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

Don't reinstall plugin wheels on every invocation. #5506

Merged
merged 2 commits into from Feb 25, 2018

Conversation

Projects
None yet
4 participants
@benjyw
Copy link
Contributor

benjyw commented Feb 22, 2018

The wheel install dir path completely qualifies it. So, assuming
it was moved atomically to that path, we can take existence of
the directory to imply that it contains the installed wheel.

This saves ~2 seconds (70%) of the no-op ./pants invocation time
in a repo with just a handful of plugins.

Don't reinstall plugin wheels on every invocation.
The wheel install dir path completely qualifies it. So, assuming
it was moved atomically to that path, we can take existence of
the directory to imply that it contains the installed wheel.

This saves ~2 seconds (70%) of the no-op ./pants invocation time
in a repo with just a handful of plugins.

@benjyw benjyw requested review from jsirois , stuhood and kwlzn Feb 22, 2018

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Feb 22, 2018

Reviewers: Please confirm my assertion that the path is fully qualified, and will change if the plugin specification changes in any way. I think that's true, but not 100% sure of it.

@stuhood
Copy link
Member

stuhood left a comment

The wheel name should always contain the version, afaik. But would be good to hear from Kris/John to confirm.

In general though, I've always been uncomfortable with the python cache(s) being located outside of .pants.d... maybe that's in order to speed up integration tests, but I expect that we'll need to build a pex for integration tests in order to remote them. This type of fiddling would feel safer if we knew that these are under .pants.d.

@kwlzn

kwlzn approved these changes Feb 22, 2018

Copy link
Member

kwlzn left a comment

lgtm

Fix indentation error.
Damn you python.
@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Feb 25, 2018

Merged based on other reviews, but @jsirois if you notice a problem here let me know and I'll revert/fix.

@benjyw benjyw merged commit aadecb4 into pantsbuild:master Feb 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:dont_reinstall_wheels branch Feb 25, 2018

@jsirois
Copy link
Member

jsirois left a comment

Thanks @benjyw, looks great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment