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

Rework version handling in setup.py #515

Merged
merged 21 commits into from
Aug 27, 2019
Merged

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Aug 22, 2019

This PR reworks the version-handling code in setup.py, and cleans up the rest of setup.py a bit en-route.

Key points for version-related changes:

  • refactored logic for version handling, separating out the reading and writing from the decision logic about how to manage the versions
  • within setup.py, versions are passed around as packaging.version.Version instances; this makes it easy to compare versions without writing our own custom logic for doing so.
  • renamed traits/_version.py to traits/version.py
  • added tests for traits.__version__ and traits.version
  • added autodoc API documentation for traits.version contents
  • made all version strings Unicode, regardless of Python version
  • added proper copyright header to traits/version.py
  • added autodoc-friendly descriptions for the values defined in traits/version.py
  • simplified traits/version.py to include only the version (previously, "full version") and the git revision
  • for non-released versions, dev numbers are computed based on number of commits between the current commit and the initial Git commit, rather than based on recent tag - the tag-based approach is fragile without extra checking that the tag is actually a release tag
  • import traits will no longer fail if there's no version.py file. Note that if there's no version.py, that almost certainly means there's no traits.ctrait extension module, so any attempt to anything significant with traits will still fail. However, a user debugging packaging problems should still be able to rely on import traits not failing.

Other changes

  • traits/__init__.py no longer provides __requires__
  • don't include extra compile arguments when building the C extension. There's no reason to use a different optimization level than Python itself uses.
  • support for building docs through python setup.py has been removed; that's just not the way that we do things any more.
  • remove the explicit version check in favour of giving that information through trove classifiers and the python_requires keyword argument
  • trove classifiers updated a bit; don't claim to support all platforms
  • author is now "Enthought"
  • miscellaneous style fixes

Supersedes #513.
Fixes #478.

setup.py Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Aug 22, 2019

Codecov Report

Merging #515 into master will increase coverage by 0.09%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #515      +/-   ##
==========================================
+ Coverage   65.23%   65.33%   +0.09%     
==========================================
  Files          44       44              
  Lines        7056     7058       +2     
  Branches     1415     1415              
==========================================
+ Hits         4603     4611       +8     
+ Misses       2030     2024       -6     
  Partials      423      423
Impacted Files Coverage Δ
traits/__init__.py 75% <50%> (-10.72%) ⬇️
traits/traits.py 60% <0%> (-0.4%) ⬇️
traits/etsconfig/etsconfig.py 63.58% <0%> (+6.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54af509...b2cfb51. Read the comment docs.

@mdickinson
Copy link
Member Author

  • don't include extra compile arguments when building the C extension

FWIW, I've confirmed that the -O3 and -DNDEBUG flags are usually supplied when building against a release Python:

    gcc -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -g -arch x86_64 -arch x86_64 -I/Users/mdickinson/.edm/envs/test/include/python3.6m -c traits/ctraits.c -o build/temp.macosx-10.9-x86_64-3.6/traits/ctraits.o

@mdickinson
Copy link
Member Author

mdickinson commented Aug 22, 2019

Okay, I'm not so happy with the new packaging dependency. I thought packaging was a dependency of setuptools (and the setuptools egg in EDM depends on packaging), but apparently that's not true any longer.

That makes the use of packaging an unnecessarily heavy requirement: new build dependencies shouldn't be added lightly.

@mdickinson
Copy link
Member Author

I'm not so happy with the new packaging dependency.

It looks as though pkg_resources.parse_version is all I actually need. (Current pkg_resources vendorises packaging, but we don't need to know that or access packaging directly.)

setup.py Outdated
git_revision : str
The full commit hash for the current Git revision.
"""
with open(VERSION_FILE, "w") as version_file:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - maybe use io.open?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I did in a previous version, but setup.py gets picky about Unicode, so I ended up reverting to the "natural" combination of bytes on Python 2 and Unicode on Python 3 (and so plain old "open" everywhere). All the text being read and written is ASCII anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

For reference: b125c2c

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I did end up going back to io.open, and adding u'' prefixes to a handful of important strings. It's an ugly solution: I really want to use from __future__ import unicode_literals, but that breaks current setuptools: I get an error resembling the following

mirzakhani:traits mdickinson$ python etstool.py update --runtime 2.7
Re-installing in  'traits-test-2.7'
[EXECUTING] edm run -e traits-test-2.7 -- python -m pip install --no-deps .
Processing /Users/mdickinson/Enthought/ETS/traits
    Complete output from command python setup.py egg_info:
    Computed package version: (u'5.2.0.dev1233', u'b2cfb514f7d2674a24bb742b42431194d4a0f37d')
    Writing version to version file /private/var/folders/07/jbbjv8b53bs5y9xyjdyn1zgw0000gn/T/pip-req-build-mZH0B0/traits/version.py.
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/private/var/folders/07/jbbjv8b53bs5y9xyjdyn1zgw0000gn/T/pip-req-build-mZH0B0/setup.py", line 243, in <module>
        ext_modules=[setuptools.Extension("traits.ctraits", ["traits/ctraits.c"])],
      File "/Users/mdickinson/.edm/envs/traits-test-2.7/lib/python2.7/site-packages/setuptools/extension.py", line 39, in __init__
        _Extension.__init__(self, name, sources, *args, **kw)
      File "/Users/mdickinson/.edm/envs/traits-test-2.7/lib/python2.7/distutils/extension.py", line 106, in __init__
        assert type(name) is StringType, "'name' must be a string"
    AssertionError: 'name' must be a string

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM apart from a minor nit-picky comment. Thanks for redoing the work!

@mdickinson
Copy link
Member Author

PR significantly updated:

  • don't rely on packaging
  • add tests
  • rename _version.py to version.py
  • add copyright header and Traits docstrings to version.py
  • make all version-related strings Unicode on all Python versions.

I've edited the main PR description to add these points.

@mdickinson
Copy link
Member Author

One more update: fix the overengineered logic with something much simpler:

  • if there's a .git directory, then use locally computed information, and always write the version file.
  • otherwise, if there's a version file, use the information from that
  • otherwise fail (via a raise RuntimeError)

I think this should be good enough, and simpler to debug in case of failure. Moreover, it avoids someone doing a local pip install even trying to run the "git" executable.

@rahulporuri Sorry for the premature PR. I think I've actually finished messing with it now.

setup.py Outdated
# Copyright (c) 2008-2019 by Enthought, Inc.
# All rights reserved.

\"""
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes me feel a bit dirty. We're only actually escaping one of the three double quotes, but that's enough to avoid this being interpreted as the end of the triple-quoted string.

Copy link
Contributor

Choose a reason for hiding this comment

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

I usually use triple single-quotes when I want to have a string with embedded triple double-quotes. I think doing that here might improve readability and make you feel less dirty.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do that. As a bonus, black leaves that solution unchanged. (I ran the entirety of setup.py through black as part of this cleanup.)

Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

I haven't actually run it, but the refactor looks reasonably sane and adds to clarity.

@mdickinson
Copy link
Member Author

Thanks @corranwebster and @rahulporuri for reviewing! I added a couple of Unicode-related tweaks, but nothing major. Merging when the CI completes.

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.

setup.py attempts to import from traits._version
4 participants