-
-
Notifications
You must be signed in to change notification settings - Fork 635
Support pip versions up to 25.1 #2195
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.
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
b7ed53e to
69d4e8a
Compare
|
@sirosen I think this is nice overall. And even if imperfect, we should merge it to unblock other things. But see my inline suggestions first and decide whether they need to be in the scope of this PR or postponed. |
tests/test_cli_compile.py
Outdated
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "relative_requirement_location", ("parent", "subdir", "sibling_dir") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having a lot of setup logic in the test and raw strings as params, let's try to move it out.
One cool technique that one of the pytest maintainers once shared is using dataclasses to encapsulate coupled bits of data in params.
The idea is that you define a shape like
from dataclasses import dataclass
@dataclass(frozen=True)
class Thing:
name: str
stuff: object
...
def __str__(self) -> str:
return self.nameAnd then use it like
@pytest.mark.parametrize(
'thing',
(
Thing('case-name'),
Thing('another-case'),
),
id=str,
)If you can extend that with more attributes and maybe add a method for setting up the test directory, this would remove complex logic from the test function, keeping only testing in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used a similar technique in the past -- maybe with named tuples? -- but this looks like a nice way to do it, and id=str feels clever and classically pythonic.
I'm happy to do this, and some of the other associated refactoring. I was trying to avoid introducing a bunch of new project infrastructure in the form of fixtures, but it's probably worth it. I'll start working through it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It was just that I was reading through the tests, and they seemed to do a lot of stuff, have a lot of logic and branches. It felt like we'd need tests for tests. So in my suggestion I was trying to come up with something that would keep the tests simple as the typical recommendation is to avoid any complexity in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a crack at this, trying to avoid baking in any test execution logic, and am happy with what I produced for now. As we've been discussing, I'd like to do a broader test refactor later to see where the things I did apply and where they can/should be modified.
|
@sirosen apparently, I suggested |
tests/test_cli_compile.py
Outdated
| raise NotImplementedError(relative_requirement_location) | ||
|
|
||
| # the input is given relative to the starting dir | ||
| input_path = req_in.relative_to(tmp_path).as_posix() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the one you pass into CLI shouldn't matter. Only the paths that end up written to output would be important to convert like this.
|
Windows CI is still failing on the new tests, and as seen in comments above I did get a little mixed up about applying So this is going to take a bit more time to sort out, but I think it's worth the extra time. It gives us clear information about what the pip-compile behavior really is, which we won't have otherwise, and there's a chance that I've introduced some bug in the Windows behavior which I need to fix. |
|
I've just finished working out the issues with Windows CI. I made more changes to the way that the paths are normalized/handled and documented it in the original PR comment as "Part 3: Normalize Windows Paths in 1c6da33 shows what it took to get CI passing on Windows without adding this extra normalization. The mixed-separator paths ( |
webknjaz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a few suggestions. Looks good otherwise.
webknjaz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful! Please, see if you can improve Git history before merging.
In order to support newer `pip` versions, `pip-compile` needs to
handle the fact that the `comes_from` field on a parsed requirement
may be rewritten from a relative path to an absolute one.
The change in `pip` occurred in version 24.3. `comes_from` is
populated with absolute paths.
`pip-compile` targets usage in which a relative path is supplied as
input and ought to be preserved in the annotations for its outputs. As
such, even though the relative path has been translated to an absolute
one, the original (relative) path should be rendered.
Because the data coming back from `pip` are ambiguous to some degree,
as a heuristic, this change will use whether or not the original path
was absolute to determine behavior. Therefore, the following files
will result in correct behavior:
# reqs.in
-r other-reqs.in
# other-reqs.in
some-pkg
`some-pkg` came from `-r other-reqs.in`.
By contrast, if the CWD is `/opt`, and `-r reqs.in` is used, these
data will be *incorrectly* rendered with relative paths:
# reqs.in
-r /opt/other-reqs.in
# /opt/other-reqs.in
some-pkg
`some-pkg` came from `-r /opt/other-reqs.in`, but will be annotated
with `-r other-reqs.in`
Note that the relative path which is used is, in this scenario,
correct, but not reflective of the inputs to the program.
However, it is possible, if the absolute paths are really wanted in
such a case, to ask for them using `pip-compile -r /opt/reqs.in`.
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Update `pipsupported` to pick the latest versions of `pip` and `setuptools`.[^1] For Python 3.8, a lower `pip` version is chosen, as 25.1.1 dropped support for 3.8 . [^1]: The `setuptools` bound might not be needed anymore on newer pip versions, but this needs separate assessment.
Use pathlib for relativizing paths, checking absolute paths. This replaces some os.path usage.
The new path normalization step now has explicit tests which verify and describe the behavior rigorously. The logic for absolute paths applies in newer pip versions, and therefore is entirely conditional on the pip version which is in use. These changes include some new test tooling: - The current pip version is available as a fixture, as is `pip_produces_absolute_paths`, which is just a version-comparison on that version to indicate the cut-line for the path normalization behavior. - `TestFilesCollection` is a dataclass for writing parameters. It's a stub filesystem layout + methods for putting those files into a temp dir. Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
1. As a small tweak in `pip_compat`, output relativized paths using `as_posix()` formatting. This makes output more consistent in this particular case, and therefore easier to test. 2. Update the path normalization tests for `-r`/`-c` usage to expressly handle the various cases around Windows paths correctly. These changes are aimed at getting to a state where tests are passing in CI, with no extra changes in the source tree. (Other than (1), above.)
In order to guarantee more consistent outputs, especially in the case of mixed paths with forward- and back-slashes, this changeset updates the `comes_from` handling to *always* apply a level of normalization. Paths passed back through the `parse_requirements()` wrapper will now always be formatted as posix paths.
- Minor fixups and adjustments - Update several docstrings to PEP 257 style, with an imperative description as the first line - Simplify `rewrite_comes_from` by making it always a callable Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
|
Thanks for fixing this! When do you have the next release including this planned? |
This PR is based on #2178 and therefore includes those commits.
Part 1: Relativizing Paths
Resolves #2131
This is a much simpler approach than the one which was attempted in #1650 , and it is imperfect. There is a heuristic being used here to try to produce good outputs, and that heuristic can be tricked. More on this below.
Although I did not make use of that work directly in creating this changeset, it deserves citation as inspiration, and for having demonstrated a path to more complete handling which we won't pursue -- the avenues closed off by that change proposal and review process helped to inform the choice here to do something ultra simple.
First, this changeset introduces a new conditional behavior for tweaking the
comes_fromannotation data.pip-compileneeds to handle the fact that thecomes_fromfield on a parsed requirement may be rewritten from a relative path to an absolute one.The change in
pipoccurred in version 24.3.comes_fromis populated with absolute paths. I believe this is an effect of pypa/pip#12877 , which was a change included in that release and in which paths are made absolute in an internal pip context.As far as
pip-compileis concerned, the main thing which matters is that requirements and constraints files will always appear with absolute paths on newer pip versions.pip-compiletargets usage in which a relative path is supplied as input and ought to be preserved in the annotations for its outputs. As such, even though the relative path has been translated to an absolute one, the original (relative) path should be rendered.Because the data coming back from
pipare ambiguous to some degree, as a heuristic, this change will use whether or not the original path was absolute to determine behavior. Therefore, the following files will result in correct behavior:some-pkgcame from-r other-reqs.in.By contrast, if the CWD is
/opt, and-r reqs.inis used, these data will be incorrectly rendered with relative paths:some-pkgcame from-r /opt/other-reqs.in, but will be annotated with-r other-reqs.inNote that the relative path which is used is, in this scenario, correct with respect to the original invocation directory, but not reflective of the inputs to the program.
It is possible, if the absolute paths are really wanted in such a case, to ask for them using
pip-compile -r /opt/reqs.in.It is possible to enhance this behavior in the future, potentially with a more robust approach, but this changeset tries to make a simple guess which will be right in >80% of realistic usages.
Part 2: Update the
pipsupportedtox envWith this fix in hand + #2178, it should be possible for tests to pass on newer
pipversions.Update
pipsupportedto pick the latest versions ofpipandsetuptools.1For Python 3.8, a lower
pipversion is chosen, as 25.0 dropped support for 3.8 .Part 3: Normalize Windows Paths in
comes_fromAfter finally getting the tests to work consistently with the paths produced by various
pipversions forcomes_fromdata, I've updated this changeset to also add a layer of normalization which ensures that we format paths as posix paths.The driver for this was the discovery that you can make
pipproduce paths with mixed path separators, e.g.:Contributor checklist
Maintainer checklist
backwards incompatible,feature,enhancement,deprecation,bug,dependency,docsorskip-changelogas they determine changelog listing.Footnotes
The
setuptoolsbound might not be needed anymore on newer pip versions, but this needs separate assessment. ↩