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

start using setuptools_scm #483

Merged
merged 7 commits into from
Apr 10, 2017
Merged

start using setuptools_scm #483

merged 7 commits into from
Apr 10, 2017

Conversation

obestwalter
Copy link
Member

@obestwalter obestwalter commented Mar 12, 2017

fixes #474

setuptools_scm removes the need to maintain versions numbers in the source code. This PR adds the setuptools_scm functioniality and removes all code that was needed due to manually handling version numbers.

  • remove _getdoctarget.py which was a helper to fetch the version for sphinx
  • remove SITETARGET using _getdoctarget.py as it is very likely obsolete as I can't find any usage or references to this
  • fetch version from pkg_resources instead

setuptools_scm removes the need to maintain versions numbers in the source code. This commit removes all code that was needed due to manually handling version numbers.

* remove _getdoctarget.py which was a helper to fetch the version for sphinx
* remove SITETARGET using that script as it is very likely obsolete asI can't find any usage or references to this
* fetch version from pkg_resources instead
@obestwalter
Copy link
Member Author

o.k. this needs some improvements still - I guess. Will get back to that later then.

@obestwalter obestwalter added the needs:discussion It's not quite clear if and how this should be done label Mar 12, 2017
@RonnyPfannschmidt
Copy link

@obestwalter seems like homegrown version parsing is the cause, it should suffice to use the packaging strict version lib there

@obestwalter
Copy link
Member Author

@RonnyPfannschmidt thanks, will do that.

…an digest

I feel uncomfortable to introduce a dependency on packaging just do some string splitting. It is always available inside virtualenvs, but not outside.
@obestwalter
Copy link
Member Author

About the aditional code: At the moment the vendored in _verlib.py is used and a version number like 2.6.1.dev18+ngaaa97e7 is not valid, so the minversion test fails due to that.

I just had my first journey into packaging (the module - not the activity) and it seems to be an implicit dependency in all virtualenvs but is not part of the distribution, so I used the same method that packaging uses to get the public version for the tox version which renders the exact same format that was in use for dev versions before.

@RonnyPfannschmidt
Copy link

btw, please rebase instead of merging from master

@obestwalter
Copy link
Member Author

That merge from master was done by the new (at least saw it for the first time today) web conflict resolving "tool". I wanted to try it out. It can only merge. Will not use it anymore then.

@obestwalter
Copy link
Member Author

obestwalter commented Mar 13, 2017

o.k. I definitely have to improve my git foo. This is quite different from the workflow I am used to and the results are pretty ugly. But this will all go away when I squash the commits on merge anyway :)

@RonnyPfannschmidt
Copy link

oh, i see, that one is quite a mess ^^

tox/__init__.py Outdated

from .hookspecs import hookspec, hookimpl # noqa

_version = get_distribution('tox').version
Copy link
Member

@nicoddemus nicoddemus Mar 13, 2017

Choose a reason for hiding this comment

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

Perhaps we should use the "file generation feature" instead (example here)? I really like that solution due to its simplicity and avoiding having a runtime dependency with pkg_resources, which is widely known to be really slow to import, see pypa/setuptools#926. It also poses problems to frozen applications (testing-cabal/mock#385), although I don't think people freeze tox.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, thanks @nicoddemus did not see that option yet. I will update the PR accordingly.

Choose a reason for hiding this comment

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

tox uses entrypoints for plugins, so its no loss

@obestwalter
Copy link
Member Author

obestwalter commented Mar 13, 2017

I wonder if using the write_to approach could make the lifes of packagers harder. @RonnyPfannschmidt and @warsaw - might I ask your opinion first, before I introduce any changes there? What is the best way to do this?

@nicoddemus
Copy link
Member

I wonder if using the write_to approach could make the lifes of packagers harder.

I don't see how, but let's hear other's opinion first.

@obestwalter
Copy link
Member Author

Better safe than sorry. I don't want to introduce unnecessary pain down the line :)

@warsaw
Copy link

warsaw commented Mar 13, 2017

@obestwalter Sorry, but can you explain what the "write_to approach" is?

@RonnyPfannschmidt
Copy link

@warsaw with write_to setuptools_scm writes the version string to a file that can be obtained by import or reading, thus is way more inexpensive than asking pkg_ressources for small tools

@RonnyPfannschmidt
Copy link

my personal suggestion is, if you use entrypoints in a major code-path, just use pkg_resources

@obestwalter
Copy link
Member Author

@warsaw the reason why I included you is because I read this here and I smell trouble for you, when you are packaging tox:

The file the version number is written to is bcolz/version.py. This file isn’t in the upstream code revision nor in the tarball which was released by the upstream developers, it’s always generated during build time.

In Debian there is an error if you try to build a package from a source tree which contains files which aren’t to be found in the corresponding tarball, like cruft from a previous build, or if any files have changed – therefore every new package should be test build also twice in a row in a non-chroot environment.

http://www.danielstender.com/blog/setuptools-scm/

If you haven't dealt with this yet, this might be a surprise then.

@obestwalter
Copy link
Member Author

my personal suggestion is, if you use entrypoints in a major code-path, just use pkg_resources

@RonnyPfannschmidt I don't quite get what you mean with this. Do you suggest we leave it like it is for tox then (no write_to)?

I could also imagine to still keep the version number in a file that actually ends up in the distribution to prevent packaging surprises.

My main aim here is to have one and exactly one place where the version number needs to be bumped manually and this ideally is the git tag.

@warsaw
Copy link

warsaw commented Mar 13, 2017

Yes, it will be painful for Debian if the version string is not included in the tarball. That blog post describes some workarounds which sound ugly, but could be doable. I haven't yet tried the SETUPTOOLS_SCM_PRETEND_VERSION support in dh-python, so I don't know how well that will work. I could probably do a test update from a tox branch if I can generate a tarball from it.

This is a fairly common problem that I have to deal with in my own packages and applications. I know the temptation to use git tags is very compelling, but tbh I have my own low tech approach. It still means I have to bump a text file version number on release, but I only have to do it in one place, so it's not that bad.

I'd certainly prefer not to have to implement workarounds in the Debian packaging, so including the version in the tarball would be best. But if you have a branch that generates a tarball representative of what you want to release, I can do a test update and see what happens.

@RonnyPfannschmidt
Copy link

@warsaw i do wonder if debian could start to adpat a scheme where there is just a fork of the main repo with debian version tags following the normal development

@warsaw
Copy link

warsaw commented Mar 13, 2017

@RonnyPfannschmidt There are definitely people who want to build new packages off of upstream git repos, but for now PyPI tarballs are the only acceptable workflow for Debian packaging within the Python packaging team. So much of our tooling is based on that, and it'll be fairly disruptive to change. I could move tox out of the team, but I really don't want to do that (team maintenance is a good thing). There are plenty of packages that don't use git for example. (I've only encountered one that doesn't release on PyPI, but only on GitHub, and that does break some tooling.) If more packages don't release on PyPI, it'll be something that we'll have to look at, but I still hope that doesn't happen. ;)

@RonnyPfannschmidt
Copy link

@warsaw i see, true - i jsut saw something like that happening for example for borgbackup/borg a while back and was thinking that it perhaps was a new way forward

@nicoddemus
Copy link
Member

I'd certainly prefer not to have to implement workarounds in the Debian packaging, so including the version in the tarball would be best.

The version file generated by write_to should be included in the generated tarball as it is supposed to be part of the distribution, that's why I couldn't imagine how that would be a problem for other packagers. 😄

my personal suggestion is, if you use entrypoints in a major code-path, just use pkg_resources

Ahh that's a good point. I say let's keep this PR as it is then, we are already paying the price of importing pkg_resources anyway to load the plugins. 👍

@warsaw
Copy link

warsaw commented Mar 13, 2017 via email

@obestwalter
Copy link
Member Author

oh dear ... good that we talked about this. Back to square one then 😁

As you said @nicoddemus we import pgk_resources already, so it's actually no additonal performance penalty and it just feels "safest" to me in regards to downstream packaging.

@warsaw - if you like I can create a tarball for you to check if the new versioning hotness breaks your workflow, although I am pretty sure that it won't cause problems in the way we have it now.

One fine day someone can maybe explain this "entrypoints in a major code-path"-thing, if not here I will bug you at the next sprint if the penny hasn't dropped until then.

@RonnyPfannschmidt
Copy link

@obestwalter what i meant is that tox already uses pkg_resources to iterate its installed plugins using entry-points, so its always being used and not an extra cost

@obestwalter
Copy link
Member Author

I just upoaded a 2.6.1 version built from this branch, if anyone wanst to check with their packaging workflow.

https://devpi.net/obestwalter/dev/tox/2.6.1

@obestwalter
Copy link
Member Author

@nicoddemus I am just using your cloud testing hack to test that package ... sweeeet :D

https://travis-ci.org/obestwalter/devpi-cloud-tester/builds/210661806

@nicoddemus
Copy link
Member

Hehehehe 😄

@obestwalter obestwalter removed the needs:discussion It's not quite clear if and how this should be done label Mar 13, 2017
@warsaw
Copy link

warsaw commented Mar 14, 2017

@obestwalter I did a test build with your tar.gz and after adjusting the Debian packaging for the upstream changes since 2.5.0, everything seemed to pass. The package built and passed its tests. I haven't actually tried to use it, but since that's not the focus of the setuptools-scm change, I think you're good to go with this PR.

@obestwalter
Copy link
Member Author

Great! Thank you @warsaw.

@obestwalter obestwalter self-assigned this Mar 25, 2017
@obestwalter
Copy link
Member Author

Are there any objections to merge this directly after the release of 2.7.0 then?

@obestwalter obestwalter merged commit 4cdb5c0 into tox-dev:master Apr 10, 2017
@obestwalter obestwalter deleted the 474-start-using-setuptools-scm branch April 10, 2017 20:43
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.

Use setuptools_scm for versioning
4 participants