-
-
Notifications
You must be signed in to change notification settings - Fork 620
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
Add lock feature to make it easier to test if you need to run pip-compile #1070
Conversation
3cc1a1a
to
e08aadb
Compare
e08aadb
to
3396a25
Compare
Codecov Report
@@ Coverage Diff @@
## master #1070 +/- ##
==========================================
+ Coverage 99.38% 99.40% +0.01%
==========================================
Files 34 38 +4
Lines 2450 2865 +415
Branches 301 334 +33
==========================================
+ Hits 2435 2848 +413
- Misses 8 9 +1
- Partials 7 8 +1
Continue to review full report at Codecov.
|
1bd8e75
to
47ec35c
Compare
Hello @lfdebrux, Thanks for your contribution! That feature looks interesting. I'll try it locally and review it carefully soon. P.S. |
Hi @atugushev , Thank you for your message, and thank you so much for offering to review it. I totally understand what you're saying; with hindsight that seems really obvious and I feel a bit silly for not creating an issue first. Although I do think this would be a useful feature, it's completely fine if it doesn't end up getting merged in the form as I've suggested. Please feel free to make any suggestions for changes or whatever! Thanks again for taking a look, I appreciate it :) |
Hi @atugushev, I appreciate it's a weird time for everyone right now, but I was wondering if you had had a chance to look at this PR? If it would help I would be happy to create a feature request issue. |
Thanks for pinging me. I've tried it locally and it works! Though, It has a bug with files included via Ping also @vphilippon @codingjoe @AndydeCleyre and @jazzband/pip-tools. What do you think folks? Any feedback is appreciated! |
I like the general idea! But I have some additional concerns.
After lock-compiling with a header, trying to do so again without a header (to get rid of it) will not work, as the lock-check exits with "nothing to do." Same deal trying to go from no-header to header. Other output-changing options that don't rely on differing input files include Even outside the options, it's possible that repository package releases would get revoked (for security reasons), and the resolution ought to change. Maybe the answer to that one is to avoid using the But again, the flag description does clearly state: "If output file already exists and has a valid lock, resolving dependencies is skipped." So maybe that's all fine.
Anyway, it seems like a good idea. |
I have the same thoughts/reflexion that @AndydeCleyre listed. Still pondering one some of them in my head at this point. It feels weird to me to check the need to run pip-compile based on if the As @atugushev pointed out, thanks for the great contribution @lfdebrux ! |
Thanks for all the comments!
@vphilippon I will try and describe the situation that made us need something like this, but I hope the feature will be good for other use cases. The particular use case we have is that we use an automated tool (https://pyup.io) for keeping our dependencies up to date; outside of the PRs raised by that tool we don't usually update our requirements files. Unfortunately pyup doesn't run pip-compile for you, so we have it currently set to just update requirements.in, and then we have to remember to run pip-compile ourselves before merging the PR. We wanted a script that would make our tests fail if pyup had updated the requirements.in file but the requirements.txt file hadn't been updated; I wrote something in bash but someone suggested it might be better to have it in Python.
I see your point @AndydeCleyre, especially as you pointed out there are lots of flags that will change the output but not the input. This feature is not strictly necessary given
Yeah I'm not sure about "lock" either. If the nothing-to-do check were removed from pip-compile then it could be something like
Agreed, I just didn't get around to it 😁
👍 |
@atugushev what is the bug? I thought I included a test for that 🤔 |
FYI there's another tool (https://dependabot.com/) belonging to GitHub nowadays. It does run |
Yeah we will probably end up using that, it's not as nice as PyUp in some ways though. |
@lfdebrux Just curious, in what ways? |
@lfdebrux TBH pyup also has problems distributing the load: pyupio/pyup#331. |
Done |
Our team @alphagov is currently migrating our repositories away from a custom pip freeze script to using pip-tools.
One thing that we've missed though is the ability to quickly check if we need to run pip-compile (either as a GitHub check, or a pre-commit script). Currently the only way to do this that we were able to see is running pip-compile and seeing if the output file has changed. However, this can take a while, and also isn't idempotent.
This pull request adds a "lock" feature that makes it easier to see if the files that were used to generate a requirements file have changed. The workflow we'd like to see is:
pip-compile --lock requirements.in
pip-check requirements.txt
!If
requirements.in
has changed since pip-compile was run,pip-check
should print a message telling you to run pip-compile again, and exit with a non-zero status so that automated tools can pick this up.In the suggested implementation, this is done by adding a checksum for each input file (including transitively included input files) to the output file; pip-check then reads the checksum and tests whether the input files still match the checksum. This is simple enough that it could be done in a shell script, but having it integrated into pip-tools makes for a nicer experience I feel!
As an added bonus, checking is also done when running
pip-compile --lock
, so that if the checksums are still valid then pip-compile doesn't need to do any dependency resolution.Changelog-friendly one-liner: Add lock feature to make it easier to test if you need to run pip-compile
Contributor checklist