-
-
Notifications
You must be signed in to change notification settings - Fork 201
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
Use setuptools_scm to package version number into metadata #766
Conversation
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.
Looks great. Thank you for taking the time.
I don't have the detailed knowledge of the python ecosystem to fully follow everything. I appreciate the links you provided. It gives me confidence that it is the right thing, even if I don't have the patience to read all the documents :)
Happy to see us using the pyproject.toml more.
@@ -19,7 +19,7 @@ jobs: | |||
|
|||
- name: install dependencies | |||
run: | | |||
pip install -r requirements/development.txt | |||
pip install . -r requirements/development.txt |
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.
Why the .
out of interest?
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.
conf.py now gets the version info from breathe as an installed package. That's what the dot does. This should not affect RTD as the readthedocs.yaml already tells RTD to install breathe as well.
Does this change the release process at all? Not that I'm sure what it is. |
if re.match(r"^\d+\.\d+\.\d+$", git_tag): | ||
# git_tag is a pure version number (no subsequent commits) | ||
version = "v" + git_tag | ||
release = "v" + git_tag |
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.
local testing revealed that the package metadata (used from this PR's changes) did not output a v
prefix.
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.
Presumably get_version
already strips that internally? Breathe has used the v
prefix for git tags for a long, long time now.
pinging @vermeeren because git blame says the mkrelease makefile originated from them. Looking at that file briefly, I don't think the release process should be affected at all by this PR. My only concern would be the prefixed "v" in the tag pushed (see this review comment). IMHO, the entire release process could be done via Github CI workflow, but that would be a separate issue as it would require @michaeljones (or whoever has access to the pypi account) to create a secret token on this repo's settings that can be used instead of Melvin's PGP token... local testing(venv) PS C:\Users\ytreh\Documents\GitHub\breathe> pip install .
Successfully installed breathe-4.31.1.dev41+g715746f
(venv) PS C:\Users\ytreh\Documents\GitHub\breathe> python -c "from importlib.metadata import version; print(version('breathe'))"
4.31.1.dev41+g715746f Contents of breathe/version.py (generated from setuptools_scm module)# coding: utf-8
# file generated by setuptools_scm
# don't change, don't track in version control
version = '4.31.1.dev41+g715746f'
version_tuple = (4, 31, 1, 'dev41', 'g715746f') |
if re.match(r"^\d+\.\d+\.\d+$", git_tag): | ||
# git_tag is a pure version number (no subsequent commits) | ||
version = "v" + git_tag | ||
release = "v" + git_tag |
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.
Presumably get_version
already strips that internally? Breathe has used the v
prefix for git tags for a long, long time now.
@@ -24,7 +23,6 @@ | |||
|
|||
setup( | |||
name="breathe", | |||
version=__version__, |
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.
@2bndy5 As far as I know this determines the version seen on PyPI with how the packaging scripts runs, but I am no way an expert on this. Are you sure removing this line won't cause issues?
I think you can verify by tagging a fake version locally (a to-be 4.32.0) and then running ./mkrelease pack
from repo root. That'll dump what is normally uploaded in mkrelease_tmp/dist/*.wlh
. Inside that there should be some metadata file iirc.
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 just read #714 (comment), so presumably this does work rightly with current version.
After this review round I think it's time to just test this and if it works it works.
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.
EDIT: This line is replaced by the usage of the setuptools_scm pkg at build time. Sorry I didn't realize which file this comment corresponded to.
This line has nothing to do with pypi. It was only ever a convenience for users to programmatically get the extension's version tag. According to PEP440 (link in PR description), users are encouraged to use
>>> from importlib.metadata import version
>>> version('wheel')
'0.32.3'
(snippet from pydocs - link in PR description)
I was wondering how to use the mkrelease script. I'll try this out, thanks
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.
This is also a reply to #766 (comment) below.
@2bndy5 Ah yeah, the mkrelease
utility does rely on a POSIX-compliant environment, so in practice will need some compatibility tooling on Windows.
The way you tagged it does actually seem to be correct and the script also seems to execute correctly producing exactly the expected output. So in that case the version number isn't being detected properly by the python tooling with the new method, which is probably related to its absolute removal I mentioned here.
Effectively mkrelease
is fairly simple and was added to automate some stuff I used to do manually every single time, the pack
function is actually very simple and can be fully found here https://github.com/michaeljones/breathe/blob/master/mkrelease#L29.
I think the problem is that a "git archive" tarball, either via CLI or download from a GitHub tag, does not contain actual git information (i.e. .git
directory) So when the python3 setup.py sdist bdist_wheel
is called it's called from a git-less, clean unpacked Breathe archive as-if downloaded from GitHub from the tag. git tag
does then not work from there.
With this realisation I'm not even sure if you can truly have fully automated versioning as it has to work without the git history, meaning the current version has be stored outside of git at at least one location. Linux/BSD distributions usually package based upon the source tarball + PGP signature, not the full git source (to avoid big size and for archiving), so I don't see an easy way out of this.
Could you share thoughts about this? I'm not sure how to cleanly solve this at this point.
@vermeeren I think setup.py has been stripping the "v" prefix automatically for some time. You should see a warning about this when running |
@2bndy5 As for automation, I kind of agree but it would mean you cannot securely handle the PGP signatures. With the current setup we have a two-factor security setup for distributions packaging Breathe. So if somehow someone malicious took over the Breathe repository (or if GitHub itself got hacked), such a release could not be packaged as it is not signed by my PGP key, which I only have locally on my own devices. It is definitely an inconvenience though but here I feel like it's a trade-off worth having in this case. |
Honestly, we've hit a separate issue worthy of discussion (perhaps in a new thread titled auto-publish via github CD).
I didn't think building the extension from a .git-less source is a huge priority. I do think the docs' quickstart should be updated to demonstrate using the extension from a pip install as that is the norm/standard.
If someone is building from source, then it would be best to do so from a fork/clone (not from release assets). I've never heard of this approach (using github release assets) being preferred by any CPython pkg; it feels like you're trying to ignore conveniences (like pip) provided by python itself.
This distributed software isn't linux-only, but now I understand where you're coming from using the PGP signature. TBH, I don't know much about signing/verifying PGP keys/signatures (or anything of that nature).
I also have 2FA enabled for my pypi account. When I use a github CD workflow to auto-publish to pypi (on a "created release" event), I have a pkg-specific secret token in the repo's settings. This can be setup using the pypi account (probably done by @michaeljones ), and it is the preferred method of authenticating uploaded distributions. I can give more hints (possibly another PR or augment this one), but you (or another admin) need to know about creating github secret tokens and using them within github action workflows.
I noticed you didn't mention if 2FA was enabled for all admin accounts for this repo. This is a typical requirement in github organization accounts whose members work together as a group. |
@vermeeren So, I guess you disapprove of this PR's solution? Feel free to close it. FWIW, I think the PGP signature (and indeterminate 2FA usage on this repo's admin accounts) is the main reason distributed uploads to pypi cannot be automated. |
This is something that's rather common, and also what pypa/build encourages. The package managers writ large also want these tarballs without git source. I don't really have any horses in this race, but this is why I never adopted |
yeah, using tox is a pain for pkgs that don't properly pip install. I had trouble running exhale tests on windows because of the lxml pkg trying to pip install without access to VS's |
Just trying to catch up a bit, sorry for not being more involved. My understanding from reading this is that this PR introduces However we're struggling to use the Is that a fair summary? Thanks for @2bndy5's work either way and to @vermeeren for checking it over. Our readthedocs builds are still failing in a manner related to this issue so I think it would be good to sort something out that I think I'd probably go for the less than ideal set up of just having the version number repeated in a couple of places with at least one of them easy to import by applications. |
Our readthedocs build is failing with:
Which I think is due to setup.py attempt to import |
Yes. You completely understood the dialogue here. @svenevs posted alternative options that would better suite the current release paradigm. |
Thanks for checking. Sorry this has stretched out so long. Thanks again for your efforts. |
A bit late, but I definitely liked this patchset, when I first saw it I thought it would really help with the simplification of version management. Just that upon closer look it turned out to cause an issue with the way packaging (afaik) has to be done. So really thanks for all your efforts @2bndy5 it's really appreciated. (And of course, everyone else doing this for free in spare time!) |
I recommend the use of setuptools-scm to get the version from GIT even if it requires changes to the packaging strategy. My colleagues and I make use of it as much as possible. Installing a modified version of breathe with Setuptools-scm adds a suffix to the version if there are changes compared to the GIT tag, e.g. |
resolves #714 and closes #746
setuptools_scm repo (for reference)
python docs on importlib.metadata module as that is now the preferred implementation to get version info for a specified (& installed) package.
This replaces the need to store the version number in the package's __init__.py module. Instead the version information can be found within the installed package metadata (as recommended by PEP 440). I've additionally configured the setuptools_scm module to generate a version.py file in the breathe folder for distibution.
This switch also reduces the need to use
git describe
in the docs' conf.py to get the git tag. Thus, I've simplified that process as well. While I was in the conf.py file, I noticed that themathjax_path
option was outdated (per info in sphinx-docs), so I updated that as well. See the docs' CI artifact to verify math formulas are correctly rendered (as I have already done locally).