Refactor -r pylock.toml, support release control and dependency cooldown - #14165
Refactor -r pylock.toml, support release control and dependency cooldown#14165sbidoul wants to merge 6 commits into
Conversation
780e67b to
f8278c3
Compare
|
This is ready. I think it makes a lot of sense to involve the PackageFinder to provide the locked links, because a lock file can been seen both as a source of requirements, and as a sort of index page which provides the available versions and artifacts URLs. |
ichard26
left a comment
There was a problem hiding this comment.
I need to do a second pass on this, but I agree this is a much cleaner approach. Kudos!
I have two general notes:
- This seems to regress support for falling back to a sdist over a wheel when the latter is rejected by
--no-binary? I guess that's what the TODO is for. Since this needs a packaging change, it seems unlikely that this will be fixed in time for pip 26.2. Speaking as the RM, I'm fine with that since this use-case seems quite rare and pylock support is still experimental, but I do want to flag this explicitly. - It's a bit unfortunate that some of the new errors don't include which lockfile the locked link comes from. This isn't a showstopper since using multiple lockfiles is likely similarly uncommon, but it could be a source of confusion nonetheless.
I still have opinions on the overall direction and shape of our pylock installation support, but I'll save that for the issue threads (and later because I haven't thought about in a while).
|
Thanks for the review and typo-catching.
It's not a regression, rather something I noticed recently and added a TODO for it.
Same opinion here. This is probably fixable but I don't want to spend time on this right now given the unlikeliness of these errors. Regarding opinions, looking forward to reading them :) I'm aiming at a consistent UX for "lock files as just another requirements source" and I feel it fits well so far, both from a UX and architecture perspective. I believe this makes sense for the low level pip commands we have. This does not preclude the creation of higher level commands such as |
Are you sure? This pylock file with a wheel and source distribution for lock-version = "1.0"
created-by = "pip"
[[packages]]
name = "six"
version = "1.17.0"
[[packages.wheels]]
name = "six-1.17.0-py2.py3-none-any.whl"
url = "https://files.pythonhosted.org/packages/b7/ce/149a00dd41f10bc29e5921b496af8b574d8413afcd5e30dfa0ed46c2cc5e/six-1.17.0-py2.py3-none-any.whl"
[packages.wheels.hashes]
sha256 = "4721f391ed90541fddacab5acf947aa0d3dc7d27b2e1e8eda2be8970586c3274"
[packages.sdist]
name = "six-1.17.0.tar.gz"
url = "https://files.pythonhosted.org/packages/94/e7/b2c673351809dca68a0e064b6af791aa332cf192da575fd474ed7d6f16a2/six-1.17.0.tar.gz"
[packages.sdist.hashes]
sha256 = "ff70335d468e7eb6ec65b95b99d3a2836546063f63acc5171de367e834932a81" |
|
Ah, I see #14187. |
|
@ichard26 indeed with 26.1 it would install by getting the sdist via the index instead of the locked URL (so that was a double bug). With this PR, your example gives an error: Which confirms that this PR is more robust. |
ichard26
left a comment
There was a problem hiding this comment.
I've left some broader thoughts on pylock.toml here: #13950 (comment), but in terms of implementation, this PR looks good to me.
I'll leave this open in case anyone has objections (including delaying this to after pip 26.2).
Insstead of adding a locked_link and locked_version to InstallRequirement, provide a locked link per project name to the PackageFinder. Let get_all_candidate ignore indexes and --find-link when a locked link is known for a project. This naturally supports format control, so the duplicate implementation in constructors.py can be removed. This also naturally supports release control, as well as --uploaded-prior-to (via the upload_time field of pylock.toml). All in all, this is a much simpler and natural implementation.
9590906 to
0090e00
Compare
|
I rebased to resolve the merge conflict in InstallRequirement constructor (due to #14206). I plan to merge this later today. |
Instead of adding a
locked_linkandlocked_versionto InstallRequirement, provide a locked link per project name to the PackageFinder.Let
get_all_candidatesignore indexes and--find-linkwhen a locked link is known for a project.This naturally supports format control, so the duplicate implementation in
constructors.pycan be removed. This also naturally addresses release control, as well as--uploaded-prior-to(via theupload_timefield ofpylock.toml). Revert the change that I had made toresolvelib/factory.py, which was a bit awkward.All in all, this is a much simpler and natural implementation, I think.
TODO:
closes #13950
closes #13963
closes #13968
closes #14168
PR Checklist: