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

Cache wheels built from immutable VCS requirements #6851

Merged
merged 3 commits into from
Nov 4, 2019
Merged

Cache wheels built from immutable VCS requirements #6851

merged 3 commits into from
Nov 4, 2019

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Aug 10, 2019 •
edited
Loading

Cache wheels built from VCS requirements referring to an immutable revision (such as a git sha).

Closes #6640

TODO

@cjerdonek
Copy link
Member

See my comment on your original issue here: #6640 (comment)

I have a concern with adding a method is_immutable_rev() that uses what the revision "looks like" to know if it's immutable, because it might not necessarily be the case. pip's current code is careful to call looks_like_hash() only after it has first checked to see if it matches a ref (e.g. branch or tag):

pip/src/pip/_internal/vcs/git.py

Lines 152 to 156 in 8720d32

if not looks_like_hash(rev):
logger.warning(
"Did not find branch or tag '%s', assuming revision or ref.",
rev,
)

Also, if the Git CLI encounters a branch whose name matches a SHA, it checks out the branch -- not the SHA. So pip's current behavior matches Git's behavior.

@sbidoul sbidoul changed the title Cache wheels built from immutable VCS requirements [WIP] Cache wheels built from immutable VCS requirements Aug 10, 2019
@pradyunsg
Copy link
Member

Let's keep the discussion of "should we do this and how" in the tracking issue: #6640. :)

@sbidoul
Copy link
Member Author

sbidoul commented Aug 10, 2019

@cjerdonek I just pushed an alternative implementation that caches the wheel if the revision in the URL is the commit hash that was checked out (using the existing is_commit_id_equal()).

@sbidoul sbidoul force-pushed the pip6640-sbi branch 3 times, most recently from 91129be to c463d96 Compare August 10, 2019 15:41
@sbidoul

This comment has been minimized.

Sign in to view
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Aug 12, 2019
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 12, 2019
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Aug 14, 2019
This was referenced Aug 15, 2019
Merged
Closed
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 19, 2019
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 5, 2019
@pradyunsg pradyunsg added the C: vcs pip's interaction with version control systems like git, svn and bzr label Sep 7, 2019
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Nov 1, 2019
@sbidoul sbidoul mentioned this pull request Nov 1, 2019
Merged
@sbidoul sbidoul force-pushed the pip6640-sbi branch 4 times, most recently from 447c56a to c86a988 Compare November 2, 2019 08:22
@sbidoul
Copy link
Member Author

sbidoul commented Nov 2, 2019 •
edited
Loading

I still need to write a couple of tests before removing the WIP status, but this PR is otherwise in good shape now.

@sbidoul sbidoul force-pushed the pip6640-sbi branch 2 times, most recently from 7be4be3 to 9086820 Compare November 2, 2019 14:23
@sbidoul sbidoul changed the title [WIP] Cache wheels built from immutable VCS requirements Cache wheels built from immutable VCS requirements Nov 2, 2019
@sbidoul
Copy link
Member Author

sbidoul commented Nov 2, 2019

This one is ready for review. All concerns expressed so far have been addressed.

The mechanism is actually quite straightforward: it decides to cache after cloning, by checking if the cloned commit hash is the one specified as revision in the URL. The implementation is also quite simple, now that the preliminary refactoring is done.

pradyunsg
pradyunsg reviewed Nov 2, 2019
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Seems like a good, scoped PR to me. Thanks @sbidoul! ^>^

I'll defer to others for reviewing this though, since I don't think I have the bandwidth to review this properly.

docs/html/reference/pip_install.rst Outdated Show resolved Hide resolved
xavfernandez
xavfernandez approved these changes Nov 2, 2019
View reviewed changes
Copy link
Member

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

Nice and clear 👍

@sbidoul
Add tests for git sha wheel caching
f5eba4f
@sbidoul
Cache wheels built from immutable Git requirements
9cad519 
Cache wheels that are built from Git requirements
that contain an immutable revision (i.e. a sha).
@sbidoul
Be extra safe in caching Git urls
a20395d 
Do not cache in case a branch/tag has the
same name as a commit sha.
@sbidoul
Copy link
Member Author

sbidoul commented Nov 3, 2019

Rebased and verified it works as expected with pip wheel too, following #7285.

chrahunt
chrahunt approved these changes Nov 3, 2019
View reviewed changes
Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@chrahunt chrahunt merged commit e79fa0e into pypa:master Nov 4, 2019
@pradyunsg pradyunsg added C: cache Dealing with cache and files in it type: enhancement Improvements to functionality labels Nov 4, 2019
@sbidoul sbidoul deleted the pip6640-sbi branch November 4, 2019 08:53
@sbidoul
Copy link
Member Author

sbidoul commented Nov 4, 2019

Great! Thanks for the review and merge.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Dec 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cjerdonek cjerdonek cjerdonek left review comments

@pradyunsg pradyunsg pradyunsg left review comments

@xavfernandez xavfernandez xavfernandez approved these changes

@chrahunt chrahunt chrahunt approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation C: cache Dealing with cache and files in it C: vcs pip's interaction with version control systems like git, svn and bzr type: enhancement Improvements to functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC] caching VCS requirements that are considered immutables
7 participants
@sbidoul @cjerdonek @pradyunsg @BrownTruck @xavfernandez @chrahunt @pypa-bot