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

pip-build-hook: enable validation of build dependencies #1431

Merged
merged 4 commits into from
May 2, 2024

Conversation

Kiskae
Copy link
Contributor

@Kiskae Kiskae commented Nov 25, 2023

Per https://pip.pypa.io/en/stable/cli/pip_wheel/#cmdoption-check-build-dependencies

It turns out that unlike pypa, pip only checks build dependencies if it is told to.

This makes the missing build dependencies a lot clearer, since it immediately lists all missing dependencies rather than fail when it can't find them.

Example output for uri-template:

Processing /build/uri-template-1.3.0
ERROR: Some build dependencies for file:///build/uri-template-1.3.0 are missing: 'setuptools_scm'.

This change turns out to have surfaced a few hidden bugs with existing test cases, additional commits fix these issues.

Contribution checklist (recommended but not always applicable/required):

  • There's an automated test for this change
  • Commit messages or code include references to related issues or PRs (including third parties)
  • Commit messages are conventional - examples from the log include "feat: add changelog files to fixup hook", "fix(contourpy): allow wheel usage", and "test: add sqlalchemy2 test"

Copy link
Contributor

@l0b0 l0b0 left a comment

Choose a reason for hiding this comment

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

Thank you! Would you be OK to include a copy of a relevant error message in the ticket, for reference?

@Kiskae
Copy link
Contributor Author

Kiskae commented Dec 5, 2023

Thank you! Would you be OK to include a copy of a relevant error message in the ticket, for reference?

Added an example to the issue body, getting it to trigger intentionally was trickier than I expected

@cpcloud
Copy link
Collaborator

cpcloud commented May 2, 2024

One difficulty here is that this check enforces version constraints, which I think might make adding the build time dependency check a non-starter without some substituteInPlace calls in the right place.

@cpcloud
Copy link
Collaborator

cpcloud commented May 2, 2024

Another issue is that this flag results in a failure for packages that are missing build time dependencies that are circular. In some cases we have to remove those build time dependencies to prevent infinite recursion.

So, for example, if we add the flag we'd need to add jupyterlab as a build time dependency to jupyterlab-widgets which would pass the build time check but then fail with an infinite recursion error later.

@cpcloud
Copy link
Collaborator

cpcloud commented May 2, 2024

I do think there's value in checking for missing build-time dependencies but I'm not sure how to surface only those and ignore the errors coming from constraint violations.

@cpcloud
Copy link
Collaborator

cpcloud commented May 2, 2024

Happy to accept the changes other than the build hook modification, merge those shortly!

@cpcloud
Copy link
Collaborator

cpcloud commented May 2, 2024

Passing locally on:

  • x86_64-darwin
  • aarch64-darwin
  • x86_64-linux

@cpcloud cpcloud merged commit 53ba796 into nix-community:master May 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants