-
Notifications
You must be signed in to change notification settings - Fork 33
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
setuptools-scm fixes #860
setuptools-scm fixes #860
Conversation
'post-release' has been deprecated in v8.0.0 [1] which is already 2 years old. The new default 'guess-next-dev' [2] perfectly suffices our needs. [1] pypa/setuptools-scm@3bcd8c0 [2] https://setuptools-scm.readthedocs.io/en/latest/extending/#setuptools_scmversion_scheme Signed-off-by: Erik Skultety <[email protected]>
.git_archival.txt is officially documented by setuptools-scm [1] as a custom template file recognized by setuptools-scm that that git itself fills out at the archive creation time. In order for git to do that, we need to instruct it via the means of a .gitattributes file. The order of precedence how setuptools-scm tries to infer a project's version number [2] is: 1) SCM itself (byt the presence of e.g. a .git directory) 2) mercurial archive config file 3) git archive config file, i.e. .git_archival.txt 4) PKG-info, i.e. from an sdist Fortunately when GitHub creates source tarballs it DOES make use of the 'git archive' functionality and so the .git_archival.txt template is properly filled out for setuptools-scm to not fail a container build. Resolves hermetoproject#821 [1] https://setuptools-scm.readthedocs.io/en/latest/usage/#git-archives Signed-off-by: Erik Skultety <[email protected]>
A past commit introduced a .git_archival.txt file accompanied by a corresponding .gitattributes file to instruct git to fill out vital information for setuptools-scm to consume in environments where the git repository metadata simply isn't available, e.g. builds from source tarballs. However, there's one more legitimate (although purely development related) usecase where setuptools-scm unfortunately crumbles again during the build - git worktrees. Git worktrees are shallow file system checkouts which reference the true git objects in the main repository by essentially a symlink in the form of a '.git' file (not a directory!). All native git actions are worktrees aware and so everything is seamless; until one tries a containerbuild from a worktree - due to the missing git metadata which cannot be resolved inside a container build environment easily without introducing nasty hacks to the Dockerfile/Containerfile, setuptools-scm doesn't once again have means of either inferring the right version for the project. This patch introduces a distinct 'fallback_version' [1] string which would only ever be applied (due to order of precedence) during container builds when building without full git metadata or a git archive --> git worktrees. Note, however, that this change on its own isn't enough to make a container build inside a git worktree pass because when setuptools-scm uses the fallback_version configuration option, setuptools doesn't follow with its default package detection logic and doesn't actually put any source files/modules to the resulting sdist/wheel that pip ends up installing. Next commit takes care of that aspect specifically. [1] https://setuptools-scm.readthedocs.io/en/latest/config/ Signed-off-by: Erik Skultety <[email protected]>
This is solely to workaround setuptool's weird behaviour of not exercising its default package discovery logic when the setuptools-scm plugin had to fall back to a static version string during its version inference. Due to setuptools-scm order of precedence for git metadata discovery, this file should only ever be used on a rare occasion, e.g. doing development builds from a git worktree. [1] https://setuptools-scm.readthedocs.io/en/latest/usage/#file-finders-hook-makes-most-of-manifestin-unnecessary Signed-off-by: Erik Skultety <[email protected]>
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.
LGTM, I don't find the second part of the PR controversial, it addresses a corner case and does not introduce much additional logic to maintain.
The thing is, it's not a corner case, it's actually a standard workflow, e.g. I use git worktrees a lot to be able to work on several branches in parallel due to frequent interrupts. That said, adopting a MANIFEST.in file when the official setuptools docs say it's no longer needed for most cases (unless adding non-source package data) it feels like rubbing the wrong way to add it, hence I said I could be convinced to drop it. It looks like though that there might be a bug in setuptools when combined with the SCM plugin that using a manifest seems to be the only way I could find which can create a proper sdist, in turn leading to a working cachi2 install inside a development container. |
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'm fine with the approach here.
It looks like though that there might be a bug in setuptools when combined with the SCM plugin that using a manifest seems to be the only way I could find which can create a proper sdist
Is it worth filing a bug in the setuptools-scm project?
Absolutely, it's on my todo list but I need to create a small reproducer first. I also think that this falls into base setuptools rather than setuptools-scm, but I need to confirm with the reproducer, so yeah, I'll do that at some point (but looking at the setuptools issues that are related to MANIFEST it doesn't seem like something that would get fixed). |
This is an attempt to fix one version-related aspect of our container image builds. The last 2 patches are a bit controversial, so I'd be inclined to drop them if requested, but they fix a legitimate workflow issue.
See the patch descriptions for detailed info.
FWIW I used a dummy release on my fork to experiment with this, please give it a try: https://github.com/eskultety/cachi2/releases/tag/1.2.3-alpha
Maintainers will complete the following section
Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:
/ok-to-test
(as is the standard for Pipelines as Code)