Fix resolving requirements with percent encoded characters #144

fviernau · 2023-09-15T21:16:01Z

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 · 2023-09-22T11:24:34Z

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

fviernau · 2023-09-25T09:25:40Z

@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 #145.

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

fviernau · 2023-09-25T11:33:38Z

@TG1999 I guess the failing tests for macos are just flakyness. In particular since they succeeded just recently for #145. Shall we re-run these failed checks?

TG1999 · 2023-09-25T12:00:54Z

@fviernau yes, makes sense

TG1999 · 2023-09-25T12:07:10Z

@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 · 2023-09-25T13:07:57Z

@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 · 2023-09-25T14:32:24Z

tests/data/azure-devops.req-38-expected.json

@@ -4,7 +4,7 @@
    "tool_homepageurl": "https://github.com/nexB/python-inspector",
    "tool_version": "0.9.8",
    "options": [
-      "--requirement /home/frank/sources/github/nexB/python-inspector/tests/data/azure-devops.req.txt",


@TG1999 We should make the tests ignore the start of these paths for sanity

I created #150 ... @fviernau do the test still pass if you keep the original path? This seems really unrelated.

pombredanne · 2023-09-25T15:01:07Z

@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 #143 (comment) 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 · 2023-09-25T15:04:51Z

@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 · 2023-09-26T06:41:10Z

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)

pombredanne

This looks good overall ... a few minor nits before merging:

Can you avoid making changes to the hardcoded test paths? this is minor but this is also unrelated.
Can you add a few unit tests for the from_link() method which is the main one impacted? https://github.com/nexB/python-inspector/pull/144/files#diff-f69b55183727cdb44af1543579e8ad3953ef4934462f87f8f052c2b6fb64e7abR671

OR moving the decoding to the from_filename as suggested below?

pombredanne · 2023-10-10T09:07:13Z

src/python_inspector/utils_pypi.py

@@ -675,7 +676,7 @@ def from_link(cls, link: Link):
        """
        requires_python = link.python_requires
        path_or_url = link.url
-        filename = os.path.basename(path_or_url.strip("/"))
+        filename = os.path.basename(unquote(path_or_url).strip("/"))


Are you sure you want to unquote there and not after getting the basename, so this happens just on the filename and not the whole path?

What about using from_filename(filename) directly with an encoded filename? This may fail, so decoding/unquoting in from_filename(filename) instead may be a better place then.

Makes sense to me. I've changed it, hope it's in the right place (overload) of that function.

pombredanne · 2023-10-10T09:11:10Z

@TG1999 re: #144 (comment) we need to update to the latest skeleton and drop CI OS combos no longer available

TG1999 · 2023-10-10T09:12:11Z

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

@pombredanne ack!

pombredanne · 2023-10-10T09:17:49Z

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.

Update the assertions to re-align with changed dependency trees. Signed-off-by: Frank Viernau <[email protected]>

This fixes several test cases in e.g. `test_cli.py`, `test_apy.py`. Signed-off-by: Frank Viernau <[email protected]>

`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 aboutcode-org#143. Signed-off-by: Frank Viernau <[email protected]>

fviernau · 2023-10-10T11:05:40Z

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

Move the resolution to the from_filename() method in subclasses Reference: aboutcode-org#143 Signed-off-by: Philippe Ombredanne <[email protected]>

These new test were missing originally and they excercise all the corner cases of encoding. Reference: aboutcode-org#143 Signed-off-by: Philippe Ombredanne <[email protected]>

* Ensure that we honor the --generic-paths option when converting to plain mapping. * Avoid recursive imports by moving remove_test_data_dir_variable_prefix to utils.py * Simplifify tests to bypass the creation of an output file when not needed * Some tests are also updated to account for package version updates. Reference: aboutcode-org#143 Signed-off-by: Philippe Ombredanne <[email protected]>

pombredanne · 2023-10-24T20:48:36Z

@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.

fviernau force-pushed the main branch from 0dc27f7 to b1c6547 Compare September 15, 2023 21:16

fviernau force-pushed the main branch from a974c24 to 988dde4 Compare September 25, 2023 09:19

fviernau force-pushed the main branch 2 times, most recently from 9cbd2ed to 326f62d Compare September 25, 2023 10:30

fviernau force-pushed the main branch from 326f62d to 4cb0af8 Compare September 25, 2023 12:23

pombredanne reviewed Sep 25, 2023

View reviewed changes

pombredanne requested changes Oct 10, 2023

View reviewed changes

fviernau added 3 commits October 10, 2023 12:54

Fix several dependency resolution tests

2476c73

Update the assertions to re-align with changed dependency trees. Signed-off-by: Frank Viernau <[email protected]>

Re-generate test data

b2adca8

This fixes several test cases in e.g. `test_cli.py`, `test_apy.py`. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the main branch from 4cb0af8 to 8c73d8d Compare October 10, 2023 10:54

pombredanne added 4 commits October 13, 2023 12:09

Merge remote-tracking branch 'origin/main' into main

37b37c4

Resolve quoted names in requirements aboutcode-org#143

1298af8

Move the resolution to the from_filename() method in subclasses Reference: aboutcode-org#143 Signed-off-by: Philippe Ombredanne <[email protected]>

Add new tests for unquoted/quoted names aboutcode-org#143

8a3558f

These new test were missing originally and they excercise all the corner cases of encoding. Reference: aboutcode-org#143 Signed-off-by: Philippe Ombredanne <[email protected]>

pombredanne approved these changes Oct 24, 2023

View reviewed changes

pombredanne merged commit e92d55c into aboutcode-org:main Oct 30, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix resolving requirements with percent encoded characters #144

Fix resolving requirements with percent encoded characters #144

fviernau commented Sep 15, 2023 •

edited

Loading

TG1999 commented Sep 22, 2023

fviernau commented Sep 25, 2023 •

edited

Loading

fviernau commented Sep 25, 2023

TG1999 commented Sep 25, 2023

TG1999 commented Sep 25, 2023 •

edited

Loading

fviernau commented Sep 25, 2023

pombredanne Sep 25, 2023

pombredanne Oct 10, 2023

pombredanne commented Sep 25, 2023

pombredanne commented Sep 25, 2023

fviernau commented Sep 26, 2023 •

edited

Loading

pombredanne left a comment

pombredanne Oct 10, 2023 •

edited

Loading

fviernau Oct 10, 2023

pombredanne commented Oct 10, 2023

TG1999 commented Oct 10, 2023

pombredanne commented Oct 10, 2023

fviernau commented Oct 10, 2023

pombredanne commented Oct 24, 2023

Fix resolving requirements with percent encoded characters #144

Fix resolving requirements with percent encoded characters #144

Conversation

fviernau commented Sep 15, 2023 • edited Loading

TG1999 commented Sep 22, 2023

fviernau commented Sep 25, 2023 • edited Loading

fviernau commented Sep 25, 2023

TG1999 commented Sep 25, 2023

TG1999 commented Sep 25, 2023 • edited Loading

fviernau commented Sep 25, 2023

pombredanne Sep 25, 2023

Choose a reason for hiding this comment

pombredanne Oct 10, 2023

Choose a reason for hiding this comment

pombredanne commented Sep 25, 2023

pombredanne commented Sep 25, 2023

fviernau commented Sep 26, 2023 • edited Loading

pombredanne left a comment

Choose a reason for hiding this comment

pombredanne Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

fviernau Oct 10, 2023

Choose a reason for hiding this comment

pombredanne commented Oct 10, 2023

TG1999 commented Oct 10, 2023

pombredanne commented Oct 10, 2023

fviernau commented Oct 10, 2023

pombredanne commented Oct 24, 2023

fviernau commented Sep 15, 2023 •

edited

Loading

fviernau commented Sep 25, 2023 •

edited

Loading

TG1999 commented Sep 25, 2023 •

edited

Loading

fviernau commented Sep 26, 2023 •

edited

Loading

pombredanne Oct 10, 2023 •

edited

Loading