Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Python metric collection #11013

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+235 −17
Open

Python metric collection #11013

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8915959
Python metric collection
sachin-sandhu Nov 25, 2024
a374f1a
more fixes
sachin-sandhu Nov 26, 2024
eb854e7
more fixes
sachin-sandhu Nov 26, 2024
6e94196
more fixes
sachin-sandhu Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 90 additions & 10 deletions python/lib/dependabot/python/file_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,41 +91,109 @@ def package_manager

sig { returns(Ecosystem::VersionManager) }
def detected_package_manager
return PeotryPackageManager.new(detect_poetry_version) if poetry_lock && detect_poetry_version
setup_python_environment if Dependabot::Experiments.enabled?(:enable_file_parser_python_local)

return PeotryPackageManager.new(detect_poetry_version) if potery_files && detect_poetry_version
sachin-sandhu marked this conversation as resolved.
Show resolved Hide resolved

return PipCompilePackageManager.new(detect_pipcompile_version) if pip_compile_files && detect_pipcompile_version

return PipenvPackageManager.new(detect_pipenv_version) if pipenv_files && detect_pipenv_version

PipPackageManager.new(detect_pip_version)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure there are python package managers we don't support yet, I think this should still follow the same logic and only return pip if pip is actually found. This method should return nil if no supported package managers are found.

Copy link
Contributor

@kbukum1 kbukum1 Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure there are python package managers we don't support yet, I think this should still follow the same logic and only return pip if pip is actually found. This method should return nil if no supported package managers are found.

@pavera
When you return ecosystem object in file_parser, currently package_manager is required field. If we are unable detect package_manager I believe we shouldn't be able to hit this method since this method is getting called before starting update operation after parsing dependency files. In this situation we should return error or something for not supported package manager.

@sachin-sandhu
Is there a way to be sure that we are returning the right package manager?

end

sig { returns(T.any(String, T.untyped)) }
def detect_poetry_version
if poetry_lock
version = SharedHelpers.run_shell_command("pyenv exec poetry --version")
.to_s.split("version ").last&.split(")")&.first
if potery_files
package_manager = PeotryPackageManager::NAME

log_if_version_malformed(PeotryPackageManager.name, version)
version = package_manager_version(package_manager)
.to_s.split("version ").last&.split(")")&.first

log_if_version_malformed(package_manager, version)

# makes sure we have correct version format returned
version if version&.match?(/^\d+(?:\.\d+)*$/)
end
rescue StandardError
nil
end

sig { returns(T.any(String, T.untyped)) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why return untyped?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abdulapopoola , This is for fail safe , As we are extracting package manager version from shell command, in case the output changes or there is any issue in version extraction (very low possibility) we can make sure metric collection exception does not cause update to fail. We are checking for untyped at "return PoetryPackageManager.new(detect_poetry_version) if poetry_files && detect_poetry_version"

def detect_pipcompile_version
if pip_compile_files
package_manager = PipCompilePackageManager::NAME

version = package_manager_version(package_manager)
.to_s.split("version ").last&.split(")")&.first

log_if_version_malformed(package_manager, version)

# makes sure we have correct version format returned
version if version&.match?(/^\d+(?:\.\d+)*$/)
end
rescue StandardError
nil
end

sig { returns(T.any(String, T.untyped)) }
def detect_pipenv_version
if pipenv_files
package_manager = PipenvPackageManager::NAME

version = package_manager_version(package_manager)
.to_s.split("versions ").last&.strip

log_if_version_malformed(package_manager, version)

# makes sure we have correct version format returned
version if version&.match?(/^\d+(?:\.\d+)*$/)
end
rescue StandardError
nil
end

sig { returns(T.any(String, T.untyped)) }
def detect_pip_version
# extracts pip version from current python via executing shell command
version = SharedHelpers.run_shell_command("pyenv exec pip -V")
.split("from").first&.split("pip")&.last&.strip
package_manager = PipPackageManager::NAME

log_if_version_malformed(PipPackageManager.name, version)
version = package_manager_version(package_manager)
.split("from").first&.split("pip")&.last&.strip

log_if_version_malformed(package_manager, version)

version&.match?(/^\d+(?:\.\d+)*$/) ? version : UNDETECTED_PACKAGE_MANAGER_VERSION
rescue StandardError
nil
end

sig { params(package_manager: String).returns(T.any(String, T.untyped)) }
def package_manager_version(package_manager)
version_info = SharedHelpers.run_shell_command("pyenv exec #{package_manager} --version")

Dependabot.logger.info("Package manager #{package_manager}, Info : #{version_info}")

version_info
rescue StandardError => e
Dependabot.logger.error(e.message)
nil
end

# setup python local setup on file parser stage
sig { void }
def setup_python_environment
language_version_manager.install_required_python

SharedHelpers.run_shell_command("pyenv local #{language_version_manager.python_major_minor}")
rescue StandardError => e
Dependabot.logger.error(e.message)
nil
end

sig { params(package_manager: String, version: String).void }
def log_if_version_malformed(package_manager, version)
# logs warning if malformed version is found
return true if version&.match?(/^\d+(?:\.\d+)*$/)
return true if version.match?(/^\d+(?:\.\d+)*$/)

Dependabot.logger.warn(
"Detected #{package_manager} with malformed version #{version}"
Expand Down Expand Up @@ -256,6 +324,18 @@ def check_requirements(requirements)
end
end

def pipcompile_in_file
requirement_files.any? { |f| f.end_with?(".in") }
end

def pipenv_files
requirement_files.any?("Pipfile")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these be constants? Tagging @kbukum1 since he established a pattern in npm

Copy link
Contributor

@kbukum1 kbukum1 Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sachin-sandhu,

I recommend checking the implementation of the npm_and_yarn package manager. You'll notice that we've moved all string constants into their respective modules/classes, allowing them to be reused throughout the system. This approach ensures that the code is cleaner and easier to maintain.

For example, in the following snippet, constants are defined within their relevant ecosystem or package manager classes. This keeps the code modular and avoids hardcoding strings repeatedly across the system:

module Dependabot
  module NpmAndYarn
    ECOSYSTEM = "npm_and_yarn"
    MANIFEST_FILENAME = "package.json"
    ...
    MANIFEST_PACKAGE_MANAGER_KEY = "packageManager"
    MANIFEST_ENGINES_KEY = "engines"

    class NpmPackageManager < Ecosystem::VersionManager
      extend T::Sig
      NAME = "npm"
      RC_FILENAME = ".npmrc"
      LOCKFILE_NAME = "package-lock.json"
      SHRINKWRAP_LOCKFILE_NAME = "npm-shrinkwrap.json"
      ...
    end

    class YarnPackageManager < Ecosystem::VersionManager
      extend T::Sig
      NAME = "yarn"
      RC_FILENAME = ".yarnrc"
      RC_YML_FILENAME = ".yarnrc.yml"
      LOCKFILE_NAME = "yarn.lock"
      ...
    end

    class PNPMPackageManager < Ecosystem::VersionManager
      extend T::Sig
      NAME = "pnpm"
      LOCKFILE_NAME = "pnpm-lock.yaml"
      PNPM_WS_YML_FILENAME = "pnpm-workspace.yaml"
      ...
    end

    DEFAULT_PACKAGE_MANAGER = NpmPackageManager::NAME

    # Define a type alias for the expected class interface
    NpmAndYarnPackageManagerClassType = T.type_alias do
      T.any(
        T.class_of(Dependabot::NpmAndYarn::NpmPackageManager),
        T.class_of(Dependabot::NpmAndYarn::YarnPackageManager),
        T.class_of(Dependabot::NpmAndYarn::PNPMPackageManager)
      )
    end
  end
end

By organizing constants this way, you can easily reference them across the system while maintaining a clean and structured design. Let me know if you have any questions!

Reference: https://github.com/dependabot/dependabot-core/blob/main/npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb#L11

end

def potery_files
true if get_original_file("poetry.lock")
end

def write_temporary_dependency_files
dependency_files
.reject { |f| f.name == ".python-version" }
Expand Down
76 changes: 74 additions & 2 deletions python/lib/dependabot/python/package_manager.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,80 @@ def initialize(raw_version, requirement = nil)
super(
NAME,
Version.new(raw_version),
DEPRECATED_PYTHON_VERSIONS,
SUPPORTED_PYTHON_VERSIONS,
SUPPORTED_VERSIONS,
DEPRECATED_VERSIONS,
requirement,
)
end

sig { override.returns(T::Boolean) }
def deprecated?
false
end

sig { override.returns(T::Boolean) }
def unsupported?
false
end
end

class PipCompilePackageManager < Dependabot::Ecosystem::VersionManager
extend T::Sig

NAME = "pip-compile"

SUPPORTED_VERSIONS = T.let([].freeze, T::Array[Dependabot::Version])

DEPRECATED_VERSIONS = T.let([].freeze, T::Array[Dependabot::Version])

sig do
params(
raw_version: String,
requirement: T.nilable(Requirement)
).void
end
def initialize(raw_version, requirement = nil)
super(
NAME,
Version.new(raw_version),
SUPPORTED_VERSIONS,
DEPRECATED_VERSIONS,
requirement,
)
end

sig { override.returns(T::Boolean) }
def deprecated?
false
end

sig { override.returns(T::Boolean) }
def unsupported?
false
end
end

class PipenvPackageManager < Dependabot::Ecosystem::VersionManager
extend T::Sig

NAME = "pipenv"

SUPPORTED_VERSIONS = T.let([].freeze, T::Array[Dependabot::Version])

DEPRECATED_VERSIONS = T.let([].freeze, T::Array[Dependabot::Version])

sig do
params(
raw_version: String,
requirement: T.nilable(Requirement)
).void
end
def initialize(raw_version, requirement = nil)
super(
NAME,
Version.new(raw_version),
SUPPORTED_VERSIONS,
DEPRECATED_VERSIONS,
requirement,
)
end
Expand Down
2 changes: 1 addition & 1 deletion python/spec/dependabot/python/peotry_package_manager_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
end
end

context "when poetry version is extracted from pyenv is well formed" do
context "when poetry version extracted from pyenv is well formed" do
# If this test starts failing, you need to adjust the "detect_poetry_version" function
# to return a valid version in format x.x, x.x.x etc. examples: 3.12.5, 3.12
version = Dependabot::SharedHelpers.run_shell_command("pyenv exec poetry --version")
Expand Down
33 changes: 33 additions & 0 deletions python/spec/dependabot/python/pip_compile_package_manager_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# typed: false
# frozen_string_literal: true

require "dependabot/python/package_manager"
require "dependabot/ecosystem"
require "spec_helper"

RSpec.describe Dependabot::Python::PipCompilePackageManager do
let(:package_manager) { described_class.new("2024.0.1") }

describe "#initialize" do
context "when version is a String" do
it "sets the version correctly" do
expect(package_manager.version).to eq("2024.0.1")
end

it "sets the name correctly" do
expect(package_manager.name).to eq("pip-compile")
end
end

context "when pip-compile version extracted from pyenv is well formed" do
# If this test starts failing, you need to adjust the "detect_pipenv_version" function
# to return a valid version in format x.x, x.x.x etc. examples: 3.12.5, 3.12
version = Dependabot::SharedHelpers.run_shell_command("pyenv exec pip-compile --version")
.to_s.split("version ").last&.split(")")&.first

it "does not raise error" do
expect(version.match(/^\d+(?:\.\d+)*$/)).to be_truthy
end
end
end
end
4 changes: 2 additions & 2 deletions python/spec/dependabot/python/pip_package_manager_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@
end
end

context "when pip version is extracted from pyenv is well formed" do
context "when pip version extracted from pyenv is well formed" do
# If this test starts failing, you need to adjust the "detect_pip_version" function
# to return a valid version in format x.x, x.x.x etc. examples: 3.12.5, 3.12
version = Dependabot::SharedHelpers.run_shell_command("pyenv exec pip -V")
version = Dependabot::SharedHelpers.run_shell_command("pyenv exec pip --version")
.split("from").first&.split("pip")&.last&.strip.to_s

it "does not raise error" do
Expand Down
33 changes: 33 additions & 0 deletions python/spec/dependabot/python/pipenv_package_manager_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# typed: false
# frozen_string_literal: true

require "dependabot/python/package_manager"
require "dependabot/ecosystem"
require "spec_helper"

RSpec.describe Dependabot::Python::PipenvPackageManager do
let(:package_manager) { described_class.new("1.8.3") }

describe "#initialize" do
context "when version is a String" do
it "sets the version correctly" do
expect(package_manager.version).to eq("1.8.3")
end

it "sets the name correctly" do
expect(package_manager.name).to eq("pipenv")
end
end

context "when pipenv version extracted from pyenv is well formed" do
# If this test starts failing, you need to adjust the "detect_pipenv_version" function
# to return a valid version in format x.x, x.x.x etc. examples: 3.12.5, 3.12
version = Dependabot::SharedHelpers.run_shell_command("pyenv exec pipenv --version")
.to_s.split("version ").last&.strip

it "does not raise error" do
expect(version.match(/^\d+(?:\.\d+)*$/)).to be_truthy
end
end
end
end
Loading