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

Pin build dependencies #13113

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Pin build dependencies #13113

wants to merge 1 commit into from

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Dec 11, 2024

Coming from the suggestions in #13048, this is a little experiment to see how pinning build deps looks like and see what @pypa/pip-committers and others think about this.

Here we pin build dependencies with pip-compile, use that to create a build environment with the build dependencies and nothing else, and build using --no-isolation to make sure (hopefully?) that nothing unpinned is downloaded during the build process.

Notably, at this point nox is not pinned. I have also not investigated what nox downloads when creating a session.

cc/ @sethmlarson @webknjaz

@notatallshaw
Copy link
Member

Here we pin build dependencies with pip-compile, use that to create a build environment with the build dependencies and nothing else, and build using --no-isolation to make sure (hopefully?) that nothing unpinned is downloaded during the build process.

I've created a similiar build step in my work environment, for maximum reproducability I would reccomend --only-binary :all: for both your compile and install step, otherwise there's a chance that your build requirements will themselves be sdists and go through their own build steps that are not pinned.

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.

I think it's a good start if it works for you. Not exactly how I'd structure it, but that's inconsequential.

As a future improvement, it would be good to have a nox session wrapping whatever generation command is used.

I've been playing with comprehensive pip constraint-based "DIY lock files" to the extremes. Here's some things I've got (sharing for the record, it's much more than what's in the scope of this PR):

Copy link
Member

Choose a reason for hiding this comment

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

.txt.in? The typical convention is just .in. Dependabot will recognize pairs of files if they have the same base name.

@@ -0,0 +1,3 @@
build
twine
setuptools
Copy link
Member

Choose a reason for hiding this comment

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

So this duplicates what's in pyproject.toml. Instead, perhaps make use of --all-build-deps to retrieve it from there? https://pip-tools.readthedocs.io/en/stable/#maximizing-reproducibility.

Comment on lines +1 to +2
build
twine
Copy link
Member

Choose a reason for hiding this comment

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

These two are on the front-end side, so I'd keep them separate.

Comment on lines +1 to +2
# This file was autogenerated by uv via the following command:
# uv pip compile --only-binary :all: --generate-hashes build-requirements.txt.in
Copy link
Member

Choose a reason for hiding this comment

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

Does uv support the same mechanism of retrieving PEP 517 build deps as pip-tools? Also, can the settings be put into a config? (.pip-tools.toml, for example)

"--python",
build_python,
"install",
"-r",
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps, document why it's not a constraint?

Suggested change
"-r",
"-r", # can't be constraint dues to regression @ https://github.com/pypa/pip/issues/9243

build_python,
"-m",
"twine",
"check",
Copy link
Member

Choose a reason for hiding this comment

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

(not really related to the refactoring in this PR, but it should always be strict — would be good to put into a separate PR, I suppose)

Suggested change
"check",
"check",
"--strict",

build_python,
"-m",
"build",
"--no-isolation",
Copy link
Member

Choose a reason for hiding this comment

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

Oh, is this because of PIP_CONSTRAINT not working with hashes and resolvelib?

Copy link
Member

Choose a reason for hiding this comment

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

So if not for #9243, #4582 (comment) could be used instead.

@@ -29,6 +28,7 @@
"tests": "tests/requirements.txt",
"common-wheels": "tests/requirements-common_wheels.txt",
}
HERE = Path(__file__).parent
Copy link
Member

Choose a reason for hiding this comment

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

How about a descriptive name?

Suggested change
HERE = Path(__file__).parent
GIT_REPOSITORY_ROOT = Path(__file__).parent

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