Change packaging toolkit to use setuptools-scm #1895

webknjaz · 2019-04-01T20:36:13Z

PR Type

Feature Pull Request

decentral1se

Fantastic!

🚀 🚀 🚀

tox.ini

molecule/__init__.py

setup.py

tox.ini

ssbarnea

This change scares the... out of me as it could break a lot and I do not see any business-case (bug) for replacing pbr with something else (like setuptools-scm + python code).

Before seeing such a change I would like to see a bug describing the issue, the proposed solution (not implementation), maybe even some discussions around what we want to fix.

Packaging is one of the most sensible areas and I personally do not want to see molecule installation failing even if someone is trying to install it on system with ancient packaging tools (pip/setuptools/...).

webknjaz · 2019-04-02T20:37:35Z

@ssbarnea pbr itself is a bug. We've been talking about it a lot already. If you want a specific case: it currently generates a totally broken version number from Git tag. And replacing it with something actually working was a public plan for months.

Back to why CI is failing now: we are hitting pypa/pip#6264 now because you've enabled --system-site-packages in tox. I'm trying to make it more resilient, though.

setuptools-scm is battle-tested and pbr is broken by design. I've used both. Does anyone here want to argue about it based on actual experience?

ssbarnea

I am not convinced about the pbr broken being broken by design but I will neither defend it forever. As long we can make molecule-with-setuptools-scm work on all platforms where molecule would be used (without having to add extra documentation to ask people to "tune" their systems) I am ok with it.

The only thing I care is to keep molecule being easily installable and consumed, as wheel or from source, with or without a virtualenv. Maybe we could use tox-docker to validate package installation on multiple platforms? (or we could use an ansible playbook too).

setup.cfg

tox.ini

gundalow · 2019-04-03T12:52:45Z

As long we can make molecule-with-setuptools-scm work on all platforms where molecule would be used (without having to add extra documentation to ask people to "tune" their systems) I am ok with it.

Do we have a full list of all platforms which we can test this PR against?

ssbarnea · 2019-04-03T13:19:33Z

Without having a full list I think this makes sense:

CentOS 7
Fedora 29 (this is base for RHEL 8/CentOS 8)
Fedora Rawhide
Ubuntu latest stable

On these it need testing with:

without virtualenv
with isolated virtualenv (not sure if it will work all the way due to missing selinux)
with virtualenv with site-packages

Checkpoints:

install passes
pip check does not report conflicts
molecule can be called and is not missing files like templates

tox.ini

tehsmyers · 2019-04-03T13:37:43Z

The multi-platform testing matrix would be useful for testing in any case, both for pbr and for setuptools-scm. That effort would probably warrant its own issue for discussion and a separate pull request.

webknjaz · 2019-04-03T14:26:46Z

As long we can make molecule-with-setuptools-scm work on all platforms

That's what I'm working on now. We'll have a shim emulating missing features, just like I did it for ansible-lint. That's why I set WIP status on this PR. I've hit a few bugs with new tox+setuptools+pip but soon I'll be able to address those.

webknjaz · 2019-04-03T14:30:56Z

The multi-platform testing matrix

If you are suggesting more test jobs, they can be added to Travis to be run on cron or we can use other resources (like GitHub Actions) just for that.

tehsmyers · 2019-04-03T14:54:25Z

If you are suggesting more test jobs, they can be added to Travis to be run on cron or we can use other resources (like GitHub Actions) just for that.

Yeah, I'm mainly suggesting that while more test jobs is a good idea, I don't think it's something that should block this pull request.

decentral1se · 2019-04-03T18:39:20Z

That effort would probably warrant its own issue for discussion and a separate pull request.

Exactly. Let's please move this discussion off this PR and focus on supporting with reviews.

feel free to test again

decentral1se

🔥

molecule/__init__.py

setup.cfg

setup.py

decentral1se · 2019-04-07T21:39:14Z

setup.py

-    setup_requires=['pbr']
-)
+
+ALL_STRING_TYPES = tuple(map(type, ('', b'', u'')))


Just to note for others, this would all be very scary and signal some major maintenance headaches but it seems to be doing just fine over at https://github.com/ansible/ansible-lint/blob/master/setup.py! So, please take that into consideration when reviewing. Maybe this shim itself needs to be packaged up ... ;)

Nope, you cannot package this shim. Just because of a chicken/egg problem :)
Of course, you could do even more nasty stuff but I urge you not to.

The right thing to do is to actually upgrade pip/setuptools/python to something better maintainable and if future you can even expect something as magical as https://twitter.com/webKnjaZ/status/1113530472808763394

** But I was thinking about making a repo with an example like this...

Oh, and this version is way more improved over the one used in ansible-lint...

tox.ini

setup.py

webknjaz · 2019-04-08T21:44:28Z

FTR: interesting things about compat — https://python3statement.org/practicalities/

decentral1se

👍

I haven't done any manual testing of this but it looks good to me now! I know that this has the possibility to resolve a number of issues for the package (and also to push forward re-release 2.20.0 -> 2.20, right!?), so am happy to see this move forward.

ssbarnea · 2019-04-09T16:46:23Z

Please ignore linter job failure. It's unrelated: yapf got updated within limits which brought us its new view on code formatting. Let's deal with it separately.

Please fix linting as it does make no sense to review code that does not pass CI.

I do not see master as failing and I was not able to replicate any yapf related issue with master code (even with latest 0.26).

Feel free to make a preparatory change related to yapf, if needed.

ssbarnea

Fails CI

tehsmyers · 2019-04-09T17:39:25Z

I do not see master as failing and I was not able to replicate any yapf related issue with master code (even with latest 0.26).

0.26 is not the latest, 0.27 is.

I was able to reproduce the failures on master:

$ git checkout master
$ git pull upstream master
$ pip install -U yapf
$ yapf -d -r molecule/ test/
<observe tons of format changes>

The fixes to this don't belong in this PR, imo.

We are not doing this here.

webknjaz · 2019-04-09T21:34:22Z

I do not see master as failing

I do:

webknjaz · 2019-04-09T21:36:57Z

Feel free to make a preparatory change related to yapf

Nope. I'm not raising the number of changed files from 12 to 100 here. Sorry. As per explanation @ #1891 I refuse to do non-atomic changes.

Also: * 🎨 include a shim to simulate setuptools' ability to read data from ``setup.cfg`` under ancient envs. * ⬆️ BSpecify project stability in metadata properly * ⬆️ Bump required tox to v3.8.4 * 🎨 Retrieve Molecule version using pkg_resources * 🐛 Whitelist find in tox in build-dists * 🎨 Enable isolated build in tox * 🔥 Drop Ansible 2.4 factor ref from tox * ⬆️ Bump setuptools req to v41.0.0 * 🎨 Wire up tox build-dists env into Travis CI * 🔒 Close ansible#1753 by using package_data Co-Authored-By: webknjaz <wk.cvs.github@sydorenko.org.ua>

tehsmyers · 2019-04-09T22:03:19Z

Nope. I'm not raising the number of changed files from 12 to 100 here. Sorry. As per explanation @ #1891 I refuse to do non-atomic changes.

Also, as #1911 demonstrates, making those changes would break lint. It's a larger problem, now captured in #1916.

webknjaz · 2019-04-09T22:50:22Z

It seems like everyone who wanted to comment on this change already did it. So I'm merging it with red CI because master is broken and this change doesn't bring it new failures.

decentral1se · 2019-04-10T06:48:12Z

Great.

Migrate to setuptools-scm and setup.py. This follows on the back of ansible/molecule#1895. There are so many different standards for specifying Python dependencies so we are just going to pick one and stick with it. All runtime dependencies should be specified in setup.cfg. All test dependencies should be specified in tox.ini. Since we are using Tox as our test execution environment we can simply pin dependencies here.

webknjaz requested review from ssbarnea, themr0c, gundalow and decentral1se April 1, 2019 20:36

This was referenced Apr 1, 2019

Remove manual version checks for Ansible/Python. Rely on dist metadata. #1890

Closed

tox.ini rework: regroup things in the testenv section #1887

Closed

Remove context-less comment from test requirements #1894

Closed

webknjaz marked this pull request as ready for review April 1, 2019 21:01

webknjaz force-pushed the packaging/setuptools-scm-declarative branch 2 times, most recently from 09602d4 to 24342ef Compare April 1, 2019 21:17

decentral1se reviewed Apr 1, 2019

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

molecule/__init__.py Outdated Show resolved Hide resolved

molecule/__init__.py Outdated Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

tox.ini Outdated Show resolved Hide resolved

webknjaz changed the title ~~Change packaging toolkit to use setuptools-scm~~ [WIP] Change packaging toolkit to use setuptools-scm Apr 1, 2019

ssbarnea requested changes Apr 2, 2019

View reviewed changes

ssbarnea previously requested changes Apr 3, 2019

View reviewed changes

setup.cfg Show resolved Hide resolved

tox.ini Show resolved Hide resolved

tox.ini Show resolved Hide resolved

tox.ini Show resolved Hide resolved

tehsmyers reviewed Apr 3, 2019

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

tox.ini Show resolved Hide resolved

decentral1se added the enhancement label Apr 4, 2019

webknjaz force-pushed the packaging/setuptools-scm-declarative branch from 4f15dcf to bf60fa0 Compare April 7, 2019 09:49

webknjaz requested a review from decentral1se April 7, 2019 09:50

webknjaz changed the title ~~[WIP] Change packaging toolkit to use setuptools-scm~~ Change packaging toolkit to use setuptools-scm Apr 7, 2019

decentral1se mentioned this pull request Apr 7, 2019

Adopt use of module invocation method for tools #1897

Merged

decentral1se reviewed Apr 7, 2019

View reviewed changes

webknjaz force-pushed the packaging/setuptools-scm-declarative branch 3 times, most recently from f1d9cda to 13667c4 Compare April 8, 2019 21:41

webknjaz requested a review from decentral1se April 8, 2019 21:57

webknjaz mentioned this pull request Apr 8, 2019

Fedora (Probably also centos/rhel) package missing some files #1753

Closed

decentral1se approved these changes Apr 9, 2019

View reviewed changes

webknjaz changed the title ~~Change packaging toolkit to use setuptools-scm~~ [WIP] Change packaging toolkit to use setuptools-scm Apr 9, 2019

This comment has been minimized.

Sign in to view

ssbarnea previously requested changes Apr 9, 2019

View reviewed changes

tehsmyers mentioned this pull request Apr 9, 2019

Update format for yapf 0.27 #1911

Closed

webknjaz force-pushed the packaging/setuptools-scm-declarative branch from 13667c4 to 33fa9eb Compare April 9, 2019 21:16

webknjaz changed the title ~~[WIP] Change packaging toolkit to use setuptools-scm~~ Change packaging toolkit to use setuptools-scm Apr 9, 2019

webknjaz force-pushed the packaging/setuptools-scm-declarative branch from 33fa9eb to cb04425 Compare April 9, 2019 21:27

webknjaz force-pushed the packaging/setuptools-scm-declarative branch 2 times, most recently from a813237 to 9a58ecc Compare April 9, 2019 21:48

webknjaz force-pushed the packaging/setuptools-scm-declarative branch from 9a58ecc to 418459f Compare April 9, 2019 21:52

webknjaz mentioned this pull request Apr 9, 2019

Pin yapf to <0.27 (0.26) #1915

Merged

webknjaz merged commit 5314ebd into ansible:master Apr 9, 2019

webknjaz mentioned this pull request Apr 10, 2019

Fixing the released version metadata #1922

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change packaging toolkit to use setuptools-scm #1895

Change packaging toolkit to use setuptools-scm #1895

webknjaz commented Apr 1, 2019

decentral1se left a comment

ssbarnea left a comment

webknjaz commented Apr 2, 2019

ssbarnea left a comment

gundalow commented Apr 3, 2019

ssbarnea commented Apr 3, 2019

tehsmyers commented Apr 3, 2019

webknjaz commented Apr 3, 2019

webknjaz commented Apr 3, 2019

tehsmyers commented Apr 3, 2019

decentral1se commented Apr 3, 2019 •

edited

Loading

decentral1se left a comment

decentral1se Apr 7, 2019 •

edited

Loading

webknjaz Apr 7, 2019

webknjaz Apr 7, 2019

webknjaz Apr 7, 2019

webknjaz commented Apr 8, 2019

decentral1se left a comment

This comment has been minimized.

ssbarnea commented Apr 9, 2019

Please ignore linter job failure. It's unrelated: yapf got updated within limits which brought us its new view on code formatting. Let's deal with it separately.

ssbarnea left a comment

tehsmyers commented Apr 9, 2019

webknjaz commented Apr 9, 2019

webknjaz commented Apr 9, 2019

tehsmyers commented Apr 9, 2019

webknjaz commented Apr 9, 2019

decentral1se commented Apr 10, 2019

Change packaging toolkit to use setuptools-scm #1895

Change packaging toolkit to use setuptools-scm #1895

Conversation

webknjaz commented Apr 1, 2019

PR Type

decentral1se left a comment

Choose a reason for hiding this comment

ssbarnea left a comment

Choose a reason for hiding this comment

webknjaz commented Apr 2, 2019

ssbarnea left a comment

Choose a reason for hiding this comment

gundalow commented Apr 3, 2019

ssbarnea commented Apr 3, 2019

tehsmyers commented Apr 3, 2019

webknjaz commented Apr 3, 2019

webknjaz commented Apr 3, 2019

tehsmyers commented Apr 3, 2019

decentral1se commented Apr 3, 2019 • edited Loading

decentral1se left a comment

Choose a reason for hiding this comment

decentral1se Apr 7, 2019 • edited Loading

Choose a reason for hiding this comment

webknjaz Apr 7, 2019

Choose a reason for hiding this comment

webknjaz Apr 7, 2019

Choose a reason for hiding this comment

webknjaz Apr 7, 2019

Choose a reason for hiding this comment

webknjaz commented Apr 8, 2019

decentral1se left a comment

Choose a reason for hiding this comment

This comment has been minimized.

ssbarnea commented Apr 9, 2019

Please ignore linter job failure. It's unrelated: yapf got updated within limits which brought us its new view on code formatting. Let's deal with it separately.

ssbarnea left a comment

Choose a reason for hiding this comment

tehsmyers commented Apr 9, 2019

webknjaz commented Apr 9, 2019

webknjaz commented Apr 9, 2019

tehsmyers commented Apr 9, 2019

webknjaz commented Apr 9, 2019

decentral1se commented Apr 10, 2019

decentral1se commented Apr 3, 2019 •

edited

Loading

decentral1se Apr 7, 2019 •

edited

Loading