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.

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

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

apljungquist
Copy link
Contributor

@apljungquist apljungquist commented Sep 18, 2022

Fixes #1396. Add options for including build dependencies in the output from pip-compile. This allows users to improve the reproducibility of their CI since they can ensure that the same versions of build dependencies are used to build their package every time.

Note that build dependencies of build dependencies are not included in the output. This is one reason why builds will still not be completely reproducible.

Contributor checklist
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@webknjaz
Copy link
Member

webknjaz commented Oct 2, 2022

@apljungquist could you also document the new feature?

tests/test_cli_compile.py Outdated Show resolved Hide resolved
"--no-annotate",
"--no-emit-options",
"--no-header",
"--build-system-requires",
Copy link
Member

Choose a reason for hiding this comment

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

We also need a test case showing how to only extract the build system requirements without the runtime requirements.
Oh, and how about being able to separately request sdist build requirements, wheel build requirements and editable install requirements?

Copy link
Contributor Author

@apljungquist apljungquist Oct 9, 2022

Choose a reason for hiding this comment

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

We also need a test case showing how to only extract the build system requirements without the runtime requirements.

Makes sense given that the CLI flag presents this as an option. Will fix.

Oh, and how about being able to separately request sdist build requirements, wheel build requirements and editable install requirements

Good catch, I had missed that build systems can state additional requirements for sdist and wheel. But I cannot find any information on editable installs, do you remember where you learned about it?

I am however unsure about the use case for locking sdist and wheel requirements separately. What do you have in mind?
If there is no clear use case, is it worth increasing the size of the UI and the test matrix?

Copy link
Member

Choose a reason for hiding this comment

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

But I cannot find any information on editable installs, do you remember where you learned about it?

https://peps.python.org/pep-0660/#get-requires-for-build-editable

I am however unsure about the use case for locking sdist and wheel requirements separately. What do you have in mind?

The fact that they are dynamically calculated suggests that they may be different or even conflicting.

Also, building a wheel from a Git checkout may be different than building a wheel from an sdist due to possible inconsistencies in what files are bundles in a wheel.
I imagine the same may happen with the editable checkout. Also, I could see a case when the editable install deps include some development helpers that further restrict the constraints.

If there is no clear use case, is it worth increasing the size of the UI and the test matrix?

It's probably worth thinking about edge cases. One case that I just realized is that the build deps can have env markers (both direct and transitive). This means that there could be different constraints produced depending on which OS/arch/Python version is used to generate them. This is an interesting case to explore. I currently experiment with having a matrix of constraint files per env combo, for runtime. Having that for build deps will probably be useful for non pure Python projects...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also need a test case showing how to only extract the build system requirements without the runtime requirements.

Makes sense given that the CLI flag presents this as an option. Will fix.

Now that I read the code again I am not sure what I was referring to. Instead it seems to me that we would have to add another flag to accomplish this e.g. --no-install-requires. This opens up questions like how such an option should interact with the --extra argument and I am starting to think that this should be treated as a separate issue.

I am also thinking about what would be a good interface for telling pip-compile what build requirements to include. I'm leaning towards replacing the previously proposed --build-system-requires flag with an --all-build flag and a --build parameter that works similarly to the --all-extra flag and --extra parameter respectively but takes editable, sdist or wheel as arguments.

Whatever the shape of the interface I think I need to spend some time exploring how the hooks work and how they can be tested. I will try to make some time for this over the coming weeks.

Copy link
Member

Choose a reason for hiding this comment

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

Extras are tightly coupled with the normal runtime deps. So if they aren't used, they shouldn't be pulled in...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have implemented support for this but I am not sure I like it because it adds complexity to both the public interface and the implementation and it is not clear to me that anyone will actually use the feature.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Looks like there's a number of things to address after all. I'm blocking this for now, see the previously posted requests.

@webknjaz
Copy link
Member

@apljungquist would you mind addressing the comments?

@apljungquist
Copy link
Contributor Author

apljungquist commented Nov 23, 2022 via email

@chaoflow
Copy link
Contributor

chaoflow commented Dec 1, 2022

@apljungquist @webknjaz Just to let you know that I'm really looking forward to this - thanks a lot! Please let me know if there is anything I can help with.

@apljungquist
Copy link
Contributor Author

@apljungquist @webknjaz Just to let you know that I'm really looking forward to this - thanks a lot! Please let me know if there is anything I can help with.

Thanks for the encouragement. Can I ask how you plan to use it?

README.rst Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
tests/test_cli_compile.py Outdated Show resolved Hide resolved
@chaoflow
Copy link
Contributor

Thanks for the encouragement. Can I ask how you plan to use it?

@apljungquist In nix-community/dream2nix#401 we're aiming for fully reproducible (build) environments.

tests/test_cli_compile.py Outdated Show resolved Hide resolved
tests/test_cli_compile.py Outdated Show resolved Hide resolved
tests/test_cli_compile.py Outdated Show resolved Hide resolved
@webknjaz webknjaz requested a review from merwok November 1, 2023 18:19
@webknjaz webknjaz added this pull request to the merge queue Nov 1, 2023
@webknjaz
Copy link
Member

webknjaz commented Nov 1, 2023

It was confused that GH was talking about some mysterious required statuses, which is why I was pushing all the buttons to closer/reopen the PR and such. At some point, it occurred to me that it doesn't track my prior approval for some reason (eventual consistency bugs?) and I re-approved, and then I saw GH doing what it should've done earlier — the PR got queued up. It should merge this, once the testing completes.

@webknjaz
Copy link
Member

webknjaz commented Nov 1, 2023

Oh, and additionally, Jannis dropped the requirement for the branches to be up-to-date, which doesn't make sense with merge queues.

Merged via the queue into jazzband:main with commit 58e68ed Nov 1, 2023
51 of 68 checks passed
@webknjaz
Copy link
Member

webknjaz commented Nov 1, 2023

🎉 @apljungquist thanks again for such an involved and well-thought contribution that took over a year to get polished!

@apljungquist
Copy link
Contributor Author

Thanks. And thank you for patiently helping me!

@webknjaz
Copy link
Member

webknjaz commented Jul 7, 2024

@apljungquist Hm.. --build-deps-for=wheel doesn't extract wheel from setuptools' backend for some reason (in the latest pip-tools, at least). Will need to investigate.

@webknjaz
Copy link
Member

webknjaz commented Jul 7, 2024

@apljungquist so it appears that using -P 'setuptools < 70.1.0' with this feature influences the final output inconsistently (as in, setuptools==70.0.0 is in the resulting file) but the dynamic build deps discovery process uses a newer version of setuptools==70.2.0, which does not have a wheel dependency since v70.1.0 (pypa/setuptools#1386 / pypa/setuptools#4369).
This turns into the situation where an older setuptools would be pinned and have a dependency on wheel but not include it in the constraints output, because the wheel build dependency lookup is happening against an older setuptools version.

cc @chrysle @merwok — in case any of you feel like going down the rabbit hole, and tackling a rather tricky problem here.

Comment on lines +106 to +107
env.install(builder.build_system_requires)
env.install(builder.get_requires_for_build("wheel"))
Copy link
Member

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.

Copy link

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 the PIP_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:

  1. The current value of `os.environ["PIP_CONSTRAINT"] should be saved to avoid irrevocably clobbering it
  2. For each env.install step, in addition to the requirements being passed explicitly, they also need to be added to a temporary file referenced from PIP_CONSTRAINT to force the dependencies to actually be respected
  3. Once the installs are done, PIP_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.

Copy link

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.

Copy link

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

Copy link

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 and wheel set via the pip-tools CLI option and via the PIP_CONSTRAINT environment variable.

Copy link
Member

Choose a reason for hiding this comment

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

@ncoghlan

  1. For each env.install step, in addition to the requirements being passed explicitly, they also need to be added to a temporary file referenced from PIP_CONSTRAINT to force the dependencies to actually be respected

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 in PIP_CONSTRAINT around such invocations.

Yes to points (1) and (3), though.

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor

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:

"--no-build-isolation",

However, this means that _create_project_builder yields build.ProjectBuilder relying on the outside environment and the (latest) version of setuptools that was installed together with pip-tools - no doubt to improve performance - but the test naturally fails.

pip-tools/piptools/build.py

Lines 192 to 194 in 5330964

if not isolated:
yield build.ProjectBuilder(src_dir, runner=runner)
return

Using build isolation makes the test pass after setting PIP_CONSTRAINT as you suggested.

Copy link
Member

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.

@apljungquist
Copy link
Contributor Author

Nice digging, and interesting finds. I won't have time to work on this for a while though, so 🤞 chrysle or Éric are interested.

@merwok
Copy link

merwok commented Jul 10, 2024

I’m not familiar with any resolver and haven’t contributed to pip-tools’ code, so I will be declining the offer.

@chrysle
Copy link
Contributor

chrysle commented Jul 17, 2024

because the wheel build dependency lookup is happening against an older setuptools version.

You mean, against a newer version (>=v70.1.0), right?

@webknjaz
Copy link
Member

@apljungquist thanks for the heads up. Hopefully, someone else will have time, maybe. I thought that I might but never got to it.

@chrysle yeah, I suppose I made a typo.

@chrysle
Copy link
Contributor

chrysle commented Jul 18, 2024

@webknjaz I'm at it; could you take a look at my above comment, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Support pinning PEP 517 build deps in pyproject.toml
9 participants