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
Fix #112: use setuptools_scm to determine version #115
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.
Changes look good, I expect they will fix my problem.
I've left some minor inline comments to maybe improve things a little further.
Thanks for picking this up!
nml/version_info.py
Outdated
version = get_nml_version() | ||
if version: | ||
try: | ||
path = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) | ||
f = open(os.path.join(path, "nml", "__version__.py"), "w") | ||
f.write('# this file is autogenerated by setup.py\n') | ||
f.write('version = "{}"\n'.format(version)) | ||
f.write('release = {}\n'.format(os.getenv("GITHUB_ACTIONS") != None)) |
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.
Clever way to check whether a release is being built. I can imagine that this would not work 100%, (a to-be-written action might try multiple revisions in turn and when someone now manually runs ./setup.py sdist
or other builds, it will not be marked as a release, which might actually be good).
One alternative would be to set release = True
whenever a build is generated by setup.py
, but then make sure that it is only set inside the generated tarball or wheel, not in the original nml/__version__.py
. I'm not quite sure how to achieve that, though (other than maybe to simply remove the nml/__version__.py
automatically after installing it, since you do not really need it in that case).
However, I think that checking for GITHUB_ACTIONS
will work and is fine for the cases at hand.
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 very much disagree ;) I think it is a bad idea to use an env-variable that happens to be there during building; the variable might change, or we might change CI, and this is the only part that would depend on that. I am sure there is a better way, as when you need to do these things, often you are solving the wrong problem, or you over-complicated the problem to start with ;)
But mostly, I am missing comments. And lots of them. What is the intention of this code? What did you try to accomplish with reading this env-variable? Please please plllleeeaaaasseeee start adding more comments to your code., especially in functions like this. This will be so difficult to understand for anyone in the future. Please? :D
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.
So what you try to get done is that once a __version__.py
is generated via a GitHub Actions, it wins from auto-detection. That means this would fail if someone makes a manual release. Strengthen my comment: this sounds like the wrong solution ;)
Why not have a __version__.py
in the repo that is a dev-stub, and on build
create the __version__.py
with the right information? You can even only put that __version__.py
in the build folder, if I remember correctly, not even changing the repo itself. That means that who-ever makes the source-tarball, or the wheel, it contains the correct and same context? (honest questions, as I am dropping in blind here, trying to discover what has been done :P)
nml/version_info.py
Outdated
version = get_nml_version() | ||
if version: | ||
try: | ||
path = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) | ||
f = open(os.path.join(path, "nml", "__version__.py"), "w") | ||
f.write('# this file is autogenerated by setup.py\n') | ||
f.write('version = "{}"\n'.format(version)) | ||
f.write('release = {}\n'.format(os.getenv("GITHUB_ACTIONS") != None)) |
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.
So what you try to get done is that once a __version__.py
is generated via a GitHub Actions, it wins from auto-detection. That means this would fail if someone makes a manual release. Strengthen my comment: this sounds like the wrong solution ;)
Why not have a __version__.py
in the repo that is a dev-stub, and on build
create the __version__.py
with the right information? You can even only put that __version__.py
in the build folder, if I remember correctly, not even changing the repo itself. That means that who-ever makes the source-tarball, or the wheel, it contains the correct and same context? (honest questions, as I am dropping in blind here, trying to discover what has been done :P)
@TrueBrain I actually think we're in total agreement, at least on what the ideal solution would. I think that would be as follows. Normal version detection (e.g. when running nmlc) would:
Then, on setup.py build/sdist/maybe others:
This means that a release version always contains a This is, I think, how the end result should. I have no idea how to achieve this with distutils/setuptools, but maybe @glx22 already found out? I won't have time to look into this in detail this weekend, though, but I'm happy to review any further changes. |
Test release on https://github.com/glx22/nml/releases/tag/0.5.1-RC3 |
I had a short look at the code, here's some initial (incomplete) observations. It seems you replaced the git detection completely by using the Looking at Also, the I also wonder: Doesn't setuptools have its own builtin way to store the version number inside a release build (and/or source tarball?) At https://pypi.org/project/setuptools-scm/ under "Retrieving package version at runtime", there are some suggestions on accessing such a version (but I'm not sure if they work for source tarballs too)? |
|
Also, I think
But also, I wonder if
Also in that order? That would sound somewhat problematic, since that would have the original problem again (where the Debian git version overrides the version in the tarball).
What is the standalone executable exactly? Is there ever a usecase that is not one of:
If these are all cases, I think |
Yup, seems this is indeed the case, And if I create a git repository inside an sdist directory (which is pretty much the same as importing a sdist tarball into my Debian repo), it still uses the git repo version, which is problematic:
|
I originally suggested setuptools_scm because that's what black uses. How does the debian packaging for that work? |
I filed an issue for
Ah, good to have an example. It seems that Then, the Debian package seems to import not the sdist tarball, but a plain git tarball (without I'm not too fond of this approach, since I'd rather import the sdist tarball (as I have been doing so far) which also includes the upstream version, rather than just deriving it from the Debian version. That would mean I can make the Debian package work with or without this PR. Might still be good to use |
I am not sure why you ask me for a review. My original comment was on the weird Pythonic construction that was created. I have no knowledge of |
I tried to fix this in a different way without the extra dependency, see #136. |
No description provided.