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

Fix #112: use setuptools_scm to determine version #115

Closed
wants to merge 1 commit into from

Conversation

glx22
Copy link
Contributor

@glx22 glx22 commented May 1, 2020

No description provided.

@glx22 glx22 changed the title Fix #112: prevent setup.py regeneration for source package Fix #112: prevent __version__.py regeneration for source package May 1, 2020
Copy link
Contributor

@matthijskooijman matthijskooijman left a 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 Show resolved Hide resolved
nml/version_info.py Outdated Show resolved Hide resolved
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))
Copy link
Contributor

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.

Copy link
Member

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

Copy link
Member

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)

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))
Copy link
Member

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)

@matthijskooijman
Copy link
Contributor

@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:

  • See if there is a version.py, if so, use that.
  • Fall back to git detection otherwise.
  • And never writes version.py anymore.

Then, on setup.py build/sdist/maybe others:

  • Version detection is performed as above
  • The resulting version is written to __version__.py in the build directory (not sure where exactly), such that this is included in the built source or binary tarball or wheel.

This means that a release version always contains a __version__.py, which is always used, regardless of any git repos. The version put into the tarball is detected from git as normal, so if you make a tarball from a non-tag or even modified git repo, that will be properly reflected. Also, if you unpack a tarball and generate a new tarball from that, the version will be preserved properly.

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.

@glx22
Copy link
Contributor Author

glx22 commented May 3, 2020

Test release on https://github.com/glx22/nml/releases/tag/0.5.1-RC3
I checked the assets on my machine, and everything seems to work.

@matthijskooijman
Copy link
Contributor

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 setuptools_scm package, so that seems to clean up nicely. However, could you comment a bit (maybe also in the code) on how that works? Does setuptools_scm write or save the version anywhere? Or does it just provide tools to get the git version and nothing else?

Looking at get_nml_version, it seems that it still looks at the git version from setuptools_scm first, falling back to __version__.py later. Or does setuptools_scm` return the release version when run from an unpacked source tarball somehow?

Also, the setuptools_scm docs suggest adding it to setup_requires, but you added it to install_requires. It seems the former is dependencies for running setup.py and the latter for installing the resulting build, so I guess the former would make more sense?

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)?

@glx22
Copy link
Contributor Author

glx22 commented May 3, 2020

setuptools_scm sets the version of the package like setuptools, and it has the option to write it to a file (I enabled it in setup.py to have it in get_nml_version as fallback), but this file is not read by setuptool_scm anyway.

setuptools_scm looks for version numbers in the SCM itself (git/hg), then in .hg_archival files (mercurial archives), then in PKG-INFO. So in some cases, like the standalone executable, it can't determine the version. That's why the fallback is there. But for unpacked source tarball it relies on PKG-INFO (I unpacked the test release tar.gz on my machine and ran python setup.py --version and it returned the correct one)

setup_requires is enough when setuptools_scm is only used in setup.py, but as it's also used in nmlc itself I think it should be installed, but it's mainly useful for dev version as installed version can use the fallback. So maybe not really required :)

@glx22 glx22 changed the title Fix #112: prevent __version__.py regeneration for source package Fix #112: use setuptools_scm to determine version May 3, 2020
@matthijskooijman
Copy link
Contributor

setup_requires is enough when setuptools_scm is only used in setup.py, but as it's also used in nmlc itself I think it should be installed, but it's mainly useful for dev version as installed version can use the fallback. So maybe not really required :)

Also, I think install_requires does not imply setup_requires. E.g. from https://setuptools.readthedocs.io/en/latest/setuptools.html:

Note: projects listed in setup_requires will NOT be automatically installed on the system where the setup script is being run. They are simply downloaded to the ./.eggs directory if they’re not locally available already. If you want them to be installed, as well as being available when the setup script is run, you should add them to install_requires and setup_requires.

But also, I wonder if install_requires is (or should be) needed, since after installation, no git version detection should happen anymore, so setuptools_scm should not be required (not sure if the code is written like this now).

setuptools_scm looks for version numbers in the SCM itself (git/hg), then in .hg_archival files (mercurial archives), then in PKG-INFO.

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).

So in some cases, like the standalone executable, it can't determine the version. That's why the fallback is there.

What is the standalone executable exactly? Is there ever a usecase that is not one of:

  • Running directly from a git checkout (which can detect the git version)
  • Building from an sdist tarball (which has PKG-INFO with the version number)
  • Installing from a wheel/bdist (which should also have PKG-INFO with the version number).

If these are all cases, I think __version__.py is not even needed?

@matthijskooijman
Copy link
Contributor

matthijskooijman commented May 4, 2020

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).

Yup, seems this is indeed the case, setuptools_scm looks at SCM data first and uses PKG_INFO and friends as fallback: https://github.com/pypa/setuptools_scm/blob/197195f52755142a97fa599d32dac83a5b5deb4a/setup.py#L78-L82

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:

nml$ ./setup.py sdist -k
    creating nml-0.5.1.dev13+g99d7350
    (snipped more output)
nml$ cd nml-0.5.1.dev13+g99d7350/
nml/nml-0.5.1.dev13+g99d7350$ git init
nml/nml-0.5.1.dev13+g99d7350$ git commit --allow-empty
nml/nml-0.5.1.dev13+g99d7350$ git tag v1.2.3
nml/nml-0.5.1.dev13+g99d7350$ ./setup.py --version
    1.2.3

@LordAro
Copy link
Member

LordAro commented May 4, 2020

I originally suggested setuptools_scm because that's what black uses. How does the debian packaging for that work?

@matthijskooijman
Copy link
Contributor

I filed an issue for setuptools_scm at pypa/setuptools_scm#423, maybe that gets some useful approaches.

I originally suggested setuptools_scm because that's what black uses. How does the debian packaging for that work?

Ah, good to have an example. It seems that black upstream does something similar as this PR: It lets setuptools_scm detect the version when running setup.py and write that to _black_version.py. At runtime, it uses the version from _black_version.py exclusively (so that means running directly from a git checkout is likely not supported, unless you create a _black_version.py first (e.g. by running setup.py).

Then, the Debian package seems to import not the sdist tarball, but a plain git tarball (without PKG_INFO and _black_version). Then then Debian build rules generate a _black_version.py, containing a version based on the Debian package version instead. Also, they patch upstream to not use setuptools_scm in setup.py.

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.
I could maybe still import the sdist and use the patch to effectively disable setuptools_scm in the Debian build. I expected this to run into problems with the clean that happens before patches are applied (as already indicated in #112), but figuring out how black solves this, I discovered that this problem does not actually occur in practice (see #112 (comment)).

That would mean I can make the Debian package work with or without this PR. Might still be good to use setuptools_scm rather than handcoded git calls as current master, though I'm not entirely sure how exactly...

@TrueBrain
Copy link
Member

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 setuptools_scm, and I have limited time to read up on this. Pretty sure any of you can read up on it and make a decision :D So I am kindly going to deny this request for review ;)

@TrueBrain TrueBrain removed their request for review May 9, 2020 12:24
@FLHerne
Copy link
Contributor

FLHerne commented May 10, 2020

I tried to fix this in a different way without the extra dependency, see #136.

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.

None yet

5 participants