Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add --dry-run option to pip install #11096

Merged
merged 5 commits into from
Jun 26, 2022
Merged

Add --dry-run option to pip install #11096

merged 5 commits into from
Jun 26, 2022

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented May 7, 2022

This options simply prints what pip would install instead of actually installing.

The output is human readable and mimics what an actual pip install prints.
It is not intended to be parsed by tools: that will be added by the --report option which will produce a rich json output.

Towards #53 and #10771.

@sbidoul sbidoul force-pushed the install-dry-run-sbi branch 2 times, most recently from e57e1d2 to 2cdca0b Compare May 7, 2022 15:28
@sbidoul sbidoul mentioned this pull request May 7, 2022
Merged
@pradyunsg
Copy link
Member

My understanding from #53 was that the concensus is to put such functionality into a separate resolve command. Are you proposing that this be called install --dry-run instead?

@sbidoul
Copy link
Member Author

sbidoul commented May 8, 2022 •
edited
Loading

My understanding from #53 was that the concensus is to put such functionality into a separate resolve command. Are you proposing that this be called install --dry-run instead?

@pradyunsg It is not entirely clear to me there was a clear consensus in #53, which is a very long thread. And #10748 shows that other people understood it as an extension of pip download.

But yes I am proposing that instead of pip resolve, we allow the composition of 3 pip install options: --dry-run, --report and --ignore-installed to the same effect, and more. It is IMO easy to understand, relatively simple to implement, and a more versatile superset of what a new pip resolve command would do. See also #10771 which has more explanations and is testable today. See also my comments in #10748.

This PR is one of several I propose for ease of review (see the full plan in #10771 (comment)).

@sbidoul
Copy link
Member Author

sbidoul commented May 9, 2022

@pradyunsg to elaborate a little bit, I am not opposed to a pip resolve that would be some sort of alias to pip install --dry-run --ignore-installed --report, although I don't think myself it is necessary. If people want we can still do it after the "beta" period of the installation report. Also, I think we discussed elsewhere to have a pip lock when a lockfile format is standardized. But nevertheless the installation report remains useful for use cases such as "tell me what pip install did".

uranusjr
uranusjr reviewed May 27, 2022
View reviewed changes
news/11096.feature.rst Outdated Show resolved Hide resolved
@sbidoul
Copy link
Member Author

sbidoul commented May 30, 2022

Thanks @uranusjr. Change applied.

@sbidoul
Copy link
Member Author

sbidoul commented May 30, 2022

Hm, pip install --dry-run -e ../localdir --use-deprecated=legacy-resolver fails with AssertionError: InstallRequirement {self} has no metadata directory and no wheel: can't make a distribution..

Not sure why there is no metadata directory in that case because it did log Preparing metadata (pyproject.toml) ... done in that case.

I'd be inclined to say that some modern pip feature require the new resolver...

@sbidoul
Copy link
Member Author

sbidoul commented May 30, 2022

Hm, and now that I have fixed the assert to properly show the requirement in error, I notice the problem comes from an already installed requirement that is somehow returned by the legacy resolver. Weird, I'll need to dig into this.

@uranusjr
Copy link
Member

If it only happens on the legacy resolver (still need to make sure of this, of course), it’s probably OK to simply filter those out manually.

@sbidoul
Copy link
Member Author

sbidoul commented May 30, 2022

@uranusjr actually it looks like the legacy resolver retunrs already installed requirements, and they are filtered out later by resolver.get_installation_order.

So yeah, this discrepancy is annoying and I think I'll make --dry-run error out when used with the legacy resolver, because I don't think refactoring legacy resolver code paths is worthwhile at this point.

@sbidoul
Copy link
Member Author

sbidoul commented May 30, 2022

OTOH if we call get_installation_order when printing what would be installed it does the job even though it is not very nice. But we can drop that together with the legacy resolver so it is not a debt we'll need to carry for a long time hopefully.

uranusjr
uranusjr approved these changes May 31, 2022
View reviewed changes
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks reasonable to me.

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Jun 1, 2022
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jun 1, 2022
@sbidoul
Copy link
Member Author

sbidoul commented Jun 1, 2022 •
edited
Loading

Alright, it looks like this is not done done yet :/ I found out that get_installation_order can't be called twice because with the new resolver it modifies the result graph and the second call fails. And with the current approach we'd need to call it twice for the installation report.

So I'm considering to do to_install = resolver.get_installation_order() just after resolve() and before the build step. I'll need to check if that is safe, especially with the legacy resolver.

@pradyunsg pradyunsg requested a review from uranusjr June 1, 2022 16:27
@sbidoul
Copy link
Member Author

sbidoul commented Jun 3, 2022

Oki, now with the last commit it should be good.

sbidoul and others added 4 commits June 23, 2022 19:33
@sbidoul
Add wheel support to InstallRequirement.get_dist()
e15e519 
Before it did support only requirements that had their
metadata prepared to a local directory.
WIth wheels that does not happen so we need to
handle that case too.

get_dist() is used by the metadata property of InstallRequirement,
which in turn is useful to obtain metadata
of the RequirementSet returned by the resolver.
@sbidoul
Add a --dry-run option to pip install
3726f71
@sbidoul @uranusjr
Use metadata['name'] to get requirement name
d378007 
Co-authored-by: Tzu-ping Chung <[email protected]>
@sbidoul
Improve --dry-run with legacy resolver
f8a54ab
@sbidoul
Add requirements_to_install to RequirementSet
701a5d6 
This property is necessary because the
legacy resolver returns requirements that
need not be installed.
@sbidoul sbidoul added this to the 22.2 milestone Jun 24, 2022
@sbidoul
Copy link
Member Author

sbidoul commented Jun 24, 2022

If there are no more comments on this one I plan to merge on Sunday or so.

@sbidoul sbidoul merged commit 65680b4 into pypa:main Jun 26, 2022
@sbidoul sbidoul deleted the install-dry-run-sbi branch June 26, 2022 09:41
@sbidoul
Copy link
Member Author

sbidoul commented Jun 26, 2022

Thanks for the review @uranusjr !

@pradyunsg pradyunsg mentioned this pull request Jun 26, 2022
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@q0w q0w q0w left review comments

@uranusjr uranusjr uranusjr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
22.2
Development

Successfully merging this pull request may close these issues.
5 participants
@sbidoul @pradyunsg @uranusjr @q0w @pypa-bot