Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 options for including build dependencies in compiled output #1681
Add options for including build dependencies in compiled output #1681
Changes from all commits
bb289c4
14009d1
973b00e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 28 in piptools/build.py
Codecov / codecov/patch
piptools/build.py#L27-L28
Check warning on line 32 in piptools/build.py
Codecov / codecov/patch
piptools/build.py#L32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apljungquist looks like these installs have to be called with
os.environ['PIP_CONSTRAINT']
set to a generated file with-P
values dumped in order to install the backend deps we desire.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webknjaz Checking I understand this comment as being related to this note in the README: "-
pip
must be used with thePIP_CONSTRAINT
environment variable to lock dependencies in build environments as documented in #8439."Rather than just installing the requirements as listed in the metadata, you're suggesting that:
env.install
step, in addition to the requirements being passed explicitly, they also need to be added to a temporary file referenced fromPIP_CONSTRAINT
to force the dependencies to actually be respectedPIP_CONSTRAINT
should be reverted back to its original value.To try to get CI going, I'm going to try just setting
PIP_CONSTRAINT
in the failing test case, which would mean this discovery still needs its own bug report.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempted workaround in bc3f17b still didn't get the affected test case passing when testing locally, but I'm trying it in CI anyway in case the local failure was specific to Python 3.12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a different workaround, specifying a known version of setuptools in the test metadata: c2bd925
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even with both attempted workarounds combined, the build dependency compilation test still fails: https://github.com/jazzband/pip-tools/pull/2105/files#diff-99b06d953a9978ee564df2e67547b8a76f3b1ab116ab7c89a9005b9239f2a904
This is with setuptools 70.0.0 explicitly requested in the test project config, and with a constraints file constraining both
setuptools
andwheel
set via thepip-tools
CLI option and via thePIP_CONSTRAINT
environment variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ncoghlan
I think that the
[build-system]requires
value is there already as an explicit install request. But the constraints coming from other places like the CLI command or other/additional input files are not taken into account. I think they should be dumped into a temporary file and that file would need to be set inPIP_CONSTRAINT
around such invocations.Yes to points (1) and (3), though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webknjaz Your suggestion to set the
PIP_CONSTRAINT
environment variable appears to work, but another problem surfaced; the failing test in question doesn't use build isolation (it succeeds when that is explicitly requested) – what do you suggest should be done in such a case?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrysle oh, I missed this. Could you clarify what you mean by requesting it? Are you talking about calling some tooling outside the test suite or making said change in the tests? Perhaps a PR would make it more obvious what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem! I was referring to the
--build-isolation
boolean flag. The failing test explicitly turned that off:pip-tools/tests/test_cli_compile.py
Line 3438 in 5330964
However, this means that
_create_project_builder
yieldsbuild.ProjectBuilder
relying on the outside environment and the (latest) version ofsetuptools
that was installed together withpip-tools
- no doubt to improve performance - but the test naturally fails.pip-tools/piptools/build.py
Lines 192 to 194 in 5330964
Using build isolation makes the test pass after setting
PIP_CONSTRAINT
as you suggested.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrysle thanks for the explanation! Yes, you're right that this was a trade-off made in the name of performance. It sounds like we need a separate regression/acceptance test checking this corner case in isolation. It'll be slower but, hopefully, one test won't make a big difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting.. The pre-commit.ci service reports
piptools/build.py:120: error: Unused "type: ignore" comment [unused-ignore]
here. And the GHA CI doesn't. This makes me think that mypy runs against different Pythons in these envs. So maybe, it's unnecessary to have this here. Or, maybe use the--python-version
mypy setting to make both test against the same interpreter version.. WDYT @apljungquist?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think specifying the language version is a good option since it makes it easier to understand what is run in CI and I have pushed a commit that I hope works.