Preserve absolute/relative paths in lockfiles#18176
Conversation
ca82125 to
c0c3e75
Compare
da28d9b to
dd390c1
Compare
|
I consider changing the lockfiles fine - we shuffled those around before in regular releases - though the |
| pub fn try_relative_to_if( | ||
| path: impl AsRef<Path>, | ||
| base: impl AsRef<Path>, | ||
| should_relativize: bool, |
There was a problem hiding this comment.
Should we pass the verbatim URL here instead so it can capture the logic about was_given_absolute?(probably needs to move to verbatim URL then)
I also noticed install_path and url: VerbatimUrl always come as a pair, do we want to bundle them?
There was a problem hiding this comment.
I think if we bundled install_path and url together then it may make sense to have that type handle this.
An initial version of this PR had try_relative_to_if take the VerbatimUrl but I didn't feel like it made sense that way. Until we merge those, I feel like it strictly makes more sense to have them split up.
But I agree that all the code basically having the same combination of install_path and url.was_given_absolute() is an indication that there's an abstraction missing here.
I'll investigate the combination option.
There was a problem hiding this comment.
I did investigate this, and the issue I am seeing is with RegistrySourceDist and RegistryBuiltWheel where there is no install_path but rather there is an IndexUrl which could be a file.
But the approach could work for PathBuiltDist, PathSourceDist, and DirectorySourceDist.
For the time being I've pushed a commit which reverts the uv add changes for this PR and created another PR which reverts the revert and is stacked. I've marked that other one breaking,
I've requested your re-review for this one just to double check I have actually reverted all the "breaking" changes.
52ea916 to
f1b84c2
Compare
Previously it would treat paths with `<drive letter>:/` as URLs pre-emptively instead of parsing the scheme like other similar code does.
f1b84c2 to
31ca0ea
Compare
8c38f1a to
666ae61
Compare
|
I think this broke a use case I have: I used to specify some dependencies that live in the same repo and provide the same python package (and are included in mutually exclusive dependency groups) as Up to now, this would generate lock files with relative paths that can be shared between systems where the repo lives in different places. This no longer works. As a workaround, using seems to do the trick, but overall this does seem to be a bit more breaking than anticipated (producing lockfiles that aren't usable on other systems than the one that produced them, from pyproject configs that worked before)? |
|
Can you describe in more detail in what case the |
File URL dependencies containing environment variable references (e.g.,
`file:///${PROJECT_ROOT}/a`) should produce relative paths in lockfiles,
since the user is parameterizing the path for portability. Previously,
`was_given_absolute()` treated all file URLs as absolute, causing these
paths to be stored as absolute in the lockfile after PR astral-sh#18176.
https://claude.ai/code/session_018jGTUmqEdwKfaWo1zQ9jWW
|
It works fine in all the cases, but it breaks locking from older |
…les as relative (#18680) ## Summary Fix a regression caused by and reported in #18176. PEP 508 doesn't actually permit variables to be specified within these URLs but we support this probably due to needing to handle it for requirements files. To avoid a breaking change in a patch release, any `VerbatimUrl` that was parsed as a PEP 508 URL that contained variables that were expanded is always treated as relative. The determination of if a `VerbatimUrl` qualifies has to be done at creation time because otherwise we would incorrectly treat a non PEP 508 URL which contained something which looks like a variable reference as a relative path in cases where this wouldn't be correct. ## Test Plan Existing test coverage covers the non-regressed case, added a test for the regressed case.
|
Thanks for the report! This should be fixed in 0.11.0 |
…les as relative (#18680) ## Summary Fix a regression caused by and reported in #18176. PEP 508 doesn't actually permit variables to be specified within these URLs but we support this probably due to needing to handle it for requirements files. To avoid a breaking change in a patch release, any `VerbatimUrl` that was parsed as a PEP 508 URL that contained variables that were expanded is always treated as relative. The determination of if a `VerbatimUrl` qualifies has to be done at creation time because otherwise we would incorrectly treat a non PEP 508 URL which contained something which looks like a variable reference as a relative path in cases where this wouldn't be correct. ## Test Plan Existing test coverage covers the non-regressed case, added a test for the regressed case.
Summary
Attempt to track and preserve relative/absolute paths when read from files.
File URLs are treated as absolute. Synthetic VerbatimUrls shouldn't have a
given, and are treated as relative.This means that paths passed as absolute will be output as absolute, although they may get normalized. Paths passed as relative will be output as relative but they may be relative to a different location (so that they continue to work going forwards). Previously in various places we'd either make things absolute unconditionally or relative unconditionally.
Cases which should now be fixed:
Also noteworthy is the bugfix for a windows misbehaviour. See the commit message for some more information.
Note: For now the
uv addside of this has been split off as a breaking change.Test Plan
Added missing tests, updated existing.
I believe all the changed tests are all now correct and were previously demonstrating buggy behaviour. Well, at least if you are on board with the idea that we should keep relative paths relative and absolute paths and / file URLs absolute.
Related Issues/PRs
find-linksorpath =causes absolute wheel path to become relative path when creating lock file #16602pylock.tomlexports #16514