aboutcode-org / python-inspector

Inspect Python code and PyPI package manifests. Resolve Python dependencies.
22 stars 19 forks source link

Fix resolving requirements with percent encoded characters #144

Closed fviernau closed 1 year ago

fviernau commented 1 year ago

Distribution.from_link() derives the version string of a package from the given (percent encoded) Link.url. That derivation lacks the decoding, so the resulting version string may also contain percent encoded characters in which case the dependency resolution fails.

Fix the resolution by URL adding the missing unquoting.

Fixes #143.

TG1999 commented 1 year ago

@fviernau thanks ++, please have a look on https://github.com/nexB/python-inspector#testing to regenerate the tests with updated data

fviernau commented 1 year ago

@fviernau thanks ++, please have a look on https://github.com/nexB/python-inspector#testing to regenerate the tests with updated data

Thanks @TG1999! I've re-generated the test data and found that there were further failing tests. So, I've decided to make a dedicated PR to fix the tests. Please see https://github.com/nexB/python-inspector/pull/145.

Note: This PR should be merged only after #145 has been merged.

fviernau commented 1 year ago

@TG1999 I guess the failing tests for macos are just flakyness. In particular since they succeeded just recently for https://github.com/nexB/python-inspector/pull/145. Shall we re-run these failed checks?

TG1999 commented 1 year ago

@fviernau yes, makes sense

TG1999 commented 1 year ago

@fviernau the new test added here for inputs with percent-encoded resolution is failing, which is an addition from #145 can you please have a look at it?

fviernau commented 1 year ago

@fviernau the new test added here for inputs with percent-encoded resolution is failing, which is an addition from #145 can you please have a look at it?

I've tried to investigate this, but unfortunately all the tests do succeed on my machine running Ubuntu. In order to investigate the above failing macos tests, I wouldn't know where to start. Do you have some pointers for me? (I do not have a machine running macos at hand)

pombredanne commented 1 year ago

@fviernau From what I can see at https://download.pytorch.org/whl/torch/ macOs does not have a version 2.0.0+cpu for Python 3.10 on macOS, therefore the resolution is not possible and fails as it should IMHO. See https://github.com/nexB/python-inspector/issues/143#issuecomment-1733827713 for the details on +.

The exact scheme chosen is largely modeled on the existing behavior of pkg_resources.parse_version and pkg_resources.parse_requirements, with the main distinction being that where pkg_resources currently always takes the suffix into account when comparing versions for exact matches, the PEP requires that the local version label of the candidate version be ignored when no local version label is present in the version specifier clause

I understand this that when you ask for torch==2.0.0+cpu you can only get this version and nothing else. And there is no version for macOS that satisfies this requirement. Hence you need to design the test differently IMHO.

@TG1999 unrelated we will need to update this repo to use the latest skeleton and drop EOL Python 3.7 and older OS versions See https://www.python.org/downloads/release/python-3717/

pombredanne commented 1 year ago

@fviernau strike this out:

2023-09-25T12:25:32.1001420Z     @pytest.mark.online
2023-09-25T12:25:32.1001760Z     def test_get_resolved_dependencies_with_url_encoded_char_plus():
2023-09-25T12:25:32.1001940Z         req = Requirement("torch==2.0.0+cpu")
2023-09-25T12:25:32.1002250Z         req.is_requirement_resolved = True
2023-09-25T12:25:32.1002420Z         _, plist = get_resolved_dependencies(
2023-09-25T12:25:32.1002570Z             requirements=[req],
2023-09-25T12:25:32.1002700Z             environment=Environment(
2023-09-25T12:25:32.1002820Z                 python_version="310",
2023-09-25T12:25:32.1003670Z                 operating_system="linux",
2023-09-25T12:25:32.1003790Z             ),
2023-09-25T12:25:32.1003910Z             repos=[
2023-09-25T12:25:32.1004070Z                 PypiSimpleRepository(index_url="https://download.pytorch.org/whl/cpu", credentials=None)
2023-09-25T12:25:32.1004240Z             ],
2023-09-25T12:25:32.1004340Z >           as_tree=False,
2023-09-25T12:25:32.1004500Z         )
2023-09-25T12:25:32.1004570Z 

does NOT resolve on macOS but on linux.

I would like to see the test run on all supported Python versions and not only on 3.7, as it could be an older bug in pip on macOS on these versions

fviernau commented 1 year ago

Hence you need to design the test differently IMHO.

I've spent some time looking for a library using local version identifier (+), which is available for all platforms without any success. Looking at [1] it's probably not even worth search pypi.org.

@pombredanne - Is your suggestion to use a single library for the test which is available to all platforms to all supported python versions? (If so, would you know such a library?)

...or rather: How do you think the test should look like?

[1] https://github.com/pypi/warehouse/issues/7989#issuecomment-632108464

pombredanne commented 1 year ago

@TG1999 re: https://github.com/nexB/python-inspector/pull/144#issuecomment-1733908420 we need to update to the latest skeleton and drop CI OS combos no longer available

TG1999 commented 1 year ago

we need to update to the latest skeleton and drop CI OS combos no longer available

@pombredanne ack!

pombredanne commented 1 year ago

Hence you need to design the test differently IMHO.

I've spent some time looking for a library using local version identifier (+), which is available for all platforms without any success. Looking at [1] it's probably not even worth search pypi.org.

@pombredanne - Is your suggestion to use a single library for the test which is available to all platforms to all supported python versions? (If so, would you know such a library?)

...or rather: How do you think the test should look like?

[1] pypi/warehouse#7989 (comment)

There is no such thing on PyPI alright. Let's get the latest cleaner CI defeinition and we may just mark this test as failing on macOS to move on as this is mysterious otherwise.

fviernau commented 1 year ago

@pombredanne I believe I've addressed all above points. The test is for now disabled on macOs.

pombredanne commented 1 year ago

@fviernau FYI, I pushed a merge of main and a few commits to add tests, and refactored the tests approach to avoid any user-specific paths.