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

Update Pypi.dependencies after update to package manager api #3040

Merged
merged 5 commits into from
Jan 25, 2023
Merged

Update Pypi.dependencies after update to package manager api #3040

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aa2091b
Update Pypi.dependencies after update to package manager api
papiro Jan 24, 2023
15ac575
Implement smarter parsing logic according to PEP508 specification
papiro Jan 25, 2023
ef5746a
Fix name of constant
papiro Jan 25, 2023
afb5349
Fixes to dependency specification name regex and variable renames
papiro Jan 25, 2023
4f46915
single character packages now correctly parse with blank requirements
papiro Jan 25, 2023
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
5 changes: 4 additions & 1 deletion app/models/package_manager/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,10 @@ def self.save_dependencies(mapped_project, sync_version: :all)

deps = begin
dependencies(name, db_version.number, mapped_project)
rescue StandardError
rescue StandardError => e
Rails.logger.error(
"Error while trying to get dependencies for #{db_platform}/#{name}@#{db_version.number}: #{e.message}"
)
[]
end

Expand Down
6 changes: 3 additions & 3 deletions app/models/package_manager/pypi.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,14 @@ def self.known_versions(name)
&.index_by { |v| v[:number] } || {}
end

def self.dependencies(name, version, _mapped_project)
def self.dependencies(name, version, _mapped_project = nil)
Copy link
Contributor

@mikeyoung85 mikeyoung85 Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure we need this change. Is something calling this method without passing in the third argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just the test.

api_response = get("https://pypi.org/pypi/#{name}/#{version}/json")
deps = api_response.dig("info", "requires_dist")
source_info = api_response.dig("releases", version)
source_info = api_response.fetch("urls", [])
Rails.logger.warn("Pypi sdist (no deps): #{name}") unless source_info.any? { |rel| rel["packagetype"] == "bdist_wheel" }

deps.map do |dep|
name, version = dep.split
name, version = dep.split(/ /, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reason for this change?

Copy link
Contributor Author

@papiro papiro Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

TLDR: before we were saying definitively that a package had a certain number of dependencies, even if some of those dependencies only came with variants of the package. Now we include the information specifying which ones are only included with a variant.

Copy link
Contributor

@tiegz tiegz Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if it's better to split on the semi-colon? Feeling slightly nervous about splitting on a whitespace (unless pypi is really strict about whitespaces in ranges) nevermind, I misread the tests at first!

Copy link
Contributor

Choose a reason for hiding this comment

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

Allegedly (according to this StackOverflow post), these dependencies are in the PEP 508 format. I can't find an official PyPI source stating this though.

Looking at the tests in the grammar section, there are a couple of instances of valid PEP 508 dependencies where the part after a space would not specify a version, e.g.

name[quux, strange];python_version<'2.7' and platform_version=='2'
=> ["name[quux,", "strange];python_version<'2.7'"and platform_version=='2'"]

That's not the point of this change, but it's possible that the current (and this) implementation would create bad data

Copy link
Contributor

Choose a reason for hiding this comment

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

That being said we should probably file something else to address it

Copy link
Contributor Author

@papiro papiro Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

oof...

According to this, we should be able to use this regex to parse the name: ^([A-Z0-9]|[A-Z0-9][A-Z0-9._-]*[A-Z0-9]). I suppose the rest of it we could provide in requirements?

Edit: Please make sure I'm understanding that right. I think when they say "names" they're talking about package names... I'm not 100% sure though.

{
project_name: name,
requirements: version.nil? || version == ";" ? "*" : version.gsub(/\(|\)/, ""),
Expand Down
96 changes: 96 additions & 0 deletions spec/fixtures/vcr_cassettes/pypi_dependencies_requests.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions spec/models/package_manager/pypi_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,31 @@
expect(described_class.deprecation_info('foo')).to eq({is_deprecated: true, message: "Development Status :: 7 - Inactive"})
end
end

describe ".dependencies" do
it "returns the dependencies of a particular version" do
VCR.use_cassette("pypi_dependencies_requests", record: :once) do
expect(
described_class.dependencies("requests", "2.28.2")
).to match_array(
[
["charset-normalizer", "<4,>=2"],
["idna", "<4,>=2.5"],
["urllib3", "<1.27,>=1.21.1"],
["certifi", ">=2017.4.17"],
["PySocks", "!=1.5.7,>=1.5.6 ; extra == 'socks'"],
["chardet", "<6,>=3.0.2 ; extra == 'use_chardet_on_py3'"]
].map do |name, requirements|
{
project_name: name,
requirements: requirements,
kind: "runtime",
optional: false,
platform: "Pypi"
}
end
)
end
end
end
end