Add setuptools.get_version(path, field='version') #1679

techtonik · 2019-02-10T06:19:19Z

Summary of changes

This allows people who use setuptools to maintain project versions
in a single place, for example as attributes in their Python source.

Closes #1316
Closes pypa/pip#6004
Closes pypa/packaging.python.org#571

Pull Request Checklist

Changes have tests
News fragment added in changelog.d. See documentation for details

This allows people who use setuptools to maintain project versions in a single place, for example as attributes in their Python source. Closes pypa#1316 Closes pypa/pip#6004 Closes pypa/packaging.python.org#571

https://travis-ci.org/pypa/setuptools/jobs/491170322#L747

pganssle · 2019-02-10T14:52:39Z

I am not sure why this function exists, it seems like it is designed to pull the version string from setup.cfg, but you can already specify the version string in setup.cfg:

[metadata]
name=foo
version=0.0.1

Why would you specify it in setup.cfg and then have a function that parses it from setup.cfg in setup.py?

In general, I think we are more interested in going the other way - we don't want to encourage people to use setup.py for things as trivial as single-sourcing their versions.

Additionally, #1316 is already closed and you can use importlib_metadata for it.

techtonik · 2019-02-10T17:42:12Z

The function is designed to pull the version string from any file including various package/__init__.py and package/version.py. This method is used by pip and by 2000+ other Python projects:

https://github.com/pypa/pip/blob/fb944ab62f755e71204020bc5b9881367803e5ec/setup.py#L18

And by a will of setuptools folks all this copy/paste could be replaced by a simple:

from setuptools import get_version, setup

It even can save you some time to update version in both setup.cfg and setup.py as well as obsolete the list of seven (import this!) ways for single sourcing https://packaging.python.org/guides/single-sourcing-package-version/ If import_metadata solves the problem, it deserves to the there. But in the meanwhile this small contribution could make some people happy (including good ones).

jaraco

I'm still on the fence with this one. On one hand, this functionality provides a convenience for packages already dependent on this behavior. In doing so, it also provides a hook where such behavior could be discouraged or deprecated.

On the other hand, we're already discouraging use of imperative code in setup.py. Most importantly, this behavior doesn't have a comparable syntax in setup.cfg. That is, one cannot specify to load the version from a field in a file through setup.cfg (except for an empty file), meaning it becomes yet another feature that's preventing the adoption of declarative config.

For me, that's the deal-breaker. I'd be much more inclined to accept a pull request that enabled the metadata.version (declarative config) to specify a path and field from which the version would be extracted.

Indeed, setuptools almost has that already with:

# setup.cfg
[metadata]
version = file:pkg/version.txt

And then in pkg/__init__.py, set __version__ = open(here / 'version.txt').read().

I seem to recall someone was working on a technique to specify an extracted version in declarative config, but I can't find that now. It's possible there were good reasons why that was not done.

So between this being a deprecated pattern, there being several other viable patterns, and because this pattern impedes the best recommended patterns, I'm strongly disinclined to accept this change. If we were to accept it, I think my first action would be to deprecate it... which suggests it shouldn't be accepted at all.

jaraco · 2019-02-17T14:32:21Z

setup.py

@@ -89,7 +89,7 @@ def pypi_link(pkg_filename):

 setup_params = dict(
    name="setuptools",
-    version="40.8.0",


Although you could use get_version here, this usage would be discouraged. Better would be to store the value in setup.cfg under metadata.version (even if in duplicate), although doing so leads to a limitation in bump2version. What's more concerning about this change is the issues it introduces if setuptools were to move to a declarative config (something that's definitely desirable)... and this speaks to the resistance to accepting this change--it's encouraging patterns that run contrary to the best practices.

So what is the ETA to introduce the best practices to pip? Why doesn't it use best practices?

Support for declarative config is relatively recent and I haven't spent much time evangelizing it, so it's probably no big surprise that some projects haven't adopted it. Even setuptools itself, which because it installs itself, would have had no minimum dependency, only adopted declarative config many months after it was available. It's just a matter of priorities and resources. Perhaps that team would accept a pull request to use declarative config; I don't know.

jaraco · 2019-02-17T14:34:20Z

setuptools/tests/test_get_version.py

+
+    def test_non_utf8_python_file(self):
+        path = os.path.join(self.tmpdir, 'russian.py')
+        with open(path, 'wb') as fp:


This should use io.open(..., encoding='cp1251').

The test is called non_ut8. Python doesn't know that it is russuan or not - filename can be any.

What I should have asked is if you could open this file as text with an encoding (cp1251) and then write unicode text to it.

jaraco · 2019-02-17T14:35:24Z

setuptools/__init__.py

+    # Using binary matching to avoid possible encoding problems
+    # when reading arbitrary text files
+    if type(field) is not bytes:
+        field = field.encode('utf-8')


This encode should happen unconditionally.

What if it is already bytes?

This function should not accept bytes on Python 3. On Python 2, if field is a str, it should be decodable through the default encoding, causing an implied decoding and explicit encoding using UTF-8.

I'm actually thinking now though that this function should only support files that are UTF-8 or where an alternate encoding is specified.

jaraco · 2019-02-17T14:35:51Z

setuptools/__init__.py

+    from "key = value" format with key matched at the beginning of
+    a line and specified by `field` parameter. Returns string.
+    """
+    version_file = os.path.abspath(path)


I suspect this line has no useful effect.

Could be. Just a safe practice, useful for debugging.

In that case, let's omit it except when debugging.

pganssle · 2019-02-17T15:26:02Z

I'm still on the fence with this one. On one hand, this functionality provides a convenience for packages already dependent on this behavior. In doing so, it also provides a hook where such behavior could be discouraged or deprecated.

I'm increasingly strongly against this change. It seems very fragile compared to many of the other single-sourcing options out there, and it encourages a pattern that I would prefer to discourage. The existing users of this pattern already have something that works for them.

I agree with you that to the extent that a change needs to be made, it should be adding more options for single-sourcing your versions in the setup.cfg file.

lmittmann · 2019-02-17T18:16:55Z

Single sourcing the package version from the variable __version__ in pkg/__init__.py is a well known and commonly used pattern. The function get_version() would finally provide a handy solution for retrieving that version in setup.py analogous to find_packages() for retrieving packages.

As of today it is not possible to use setup.cfg only to describe the package. Instead one must define different information across setup.py and setup.cfg. It seems absurd, to store the version number in version.txt only to import it from pkg/__init__.py and setup.cfg, when it could be as simple as calling get_version() from within setup.py and keep storing the version at pkg/__init__.py.

The future might be a pure setup.cfg for package description with version extraction mechanisms similar to what get_version() does. Until then the function get_version() provides a great improvement to a well known and widely accepted pattern.

pganssle · 2019-02-17T18:29:08Z

@lmittmann I don't see how this is a great improvement. Generally speaking we don't want people using the imperative build system for super common patterns because it's unnecessarily powerful and error-prone. This patch itself is very error prone and makes some very specific assumptions about how the version will be specified.

For example, if someone specifies their version like this:

VERSION_MAJOR = 1
VERSION_MINOR = 2
VERSION_PATCH = 0

__version__ = '.'.join(map(str, (VERSION_MAJOR,
                                 VERSION_MINOR,
                                 VERSION_PATCH)))

This get_version function will not work. Similarly it won't work if the version is set somewhere and then modified, possibly conditionally, like this:

__version__ = '0.2.4'

if is_dev():
    __version__ += ".dev" + get_dev_version()

I'll also note that this function doesn't even write one test using it to enable the pattern of "pull the version from __version__", instead, the test is to pull it from setup.cfg, which doesn't really make sense, since you can already specify the version in setup.cfg. That is not a good sign that it's ready to be the go-to way to fetch versions.

It is very difficult to write truly generic functions that work in a wide variety of circumstances, but it's very easy to write functions that work for your particular situation. Given that we're really hoping to get people away from using the imperative build configuration (setup.py) for simple things where they should be able to use the declarative build configuration (setup.cfg) and this kind of function is pretty easy to write if you know exactly how you are specifying your version, I don't think we should be spending our time building and maintaining such a thing.

jaraco · 2019-03-09T17:49:27Z

Thanks for the contrib, but the consensus is we will decline.

techtonik · 2019-03-20T02:52:11Z

@pganssle your story is very far-fetched. It could make sense if get_version was a default behavior, but it is not - it is just an opt-in for recommended scenario #1 from Python packaging guide https://packaging.python.org/guides/single-sourcing-package-version/

@jaraco if #1687 solves single sourcing problem, why it is not possible to remove get_version brother from pip codebase. https://github.com/pypa/pip/blob/master/setup.py#L18 ?

jaraco

I've commented on the responses above (sorry for the delayed response)

jaraco · 2019-09-07T21:43:53Z

setuptools/__init__.py

+    from "key = value" format with key matched at the beginning of
+    a line and specified by `field` parameter. Returns string.
+    """
+    version_file = os.path.abspath(path)


In that case, let's omit it except when debugging.

jaraco · 2019-09-07T21:47:35Z

setuptools/__init__.py

+    # Using binary matching to avoid possible encoding problems
+    # when reading arbitrary text files
+    if type(field) is not bytes:
+        field = field.encode('utf-8')


This function should not accept bytes on Python 3. On Python 2, if field is a str, it should be decodable through the default encoding, causing an implied decoding and explicit encoding using UTF-8.

I'm actually thinking now though that this function should only support files that are UTF-8 or where an alternate encoding is specified.

jaraco · 2019-09-08T13:29:15Z

setuptools/tests/test_get_version.py

+
+    def test_non_utf8_python_file(self):
+        path = os.path.join(self.tmpdir, 'russian.py')
+        with open(path, 'wb') as fp:


What I should have asked is if you could open this file as text with an encoding (cp1251) and then write unicode text to it.

jaraco · 2019-09-08T13:33:12Z

setup.py

@@ -89,7 +89,7 @@ def pypi_link(pkg_filename):

 setup_params = dict(
    name="setuptools",
-    version="40.8.0",


Support for declarative config is relatively recent and I haven't spent much time evangelizing it, so it's probably no big surprise that some projects haven't adopted it. Even setuptools itself, which because it installs itself, would have had no minimum dependency, only adopted declarative config many months after it was available. It's just a matter of priorities and resources. Perhaps that team would accept a pull request to use declarative config; I don't know.

techtonik added 8 commits February 10, 2019 08:35

Add setuptools.get_version(path, field='__version__')

1c007d2

This allows people who use setuptools to maintain project versions in a single place, for example as attributes in their Python source. Closes pypa#1316 Closes pypa/pip#6004 Closes pypa/packaging.python.org#571

Add news fragment

958cbc1

Tests for setuptools.get_version()

31f6131

Fix encoding and self version test failures

043573f

https://travis-ci.org/pypa/setuptools/jobs/491170322#L747

Rename get_version tests for clarity

4c964a4

Write file contents separately for each test

d00fd5b

Fix for Python 3 and add missing shutil

ded0994

Strip \r for Windows

bd651a6

techtonik mentioned this pull request Feb 10, 2019

Simplify version parsing example pypa/packaging.python.org#571

Closed

jaraco reviewed Feb 17, 2019

View reviewed changes

jaraco mentioned this pull request Feb 17, 2019

Move version to declarative config #1687

Merged

jaraco closed this Mar 9, 2019

jwodder mentioned this pull request Apr 27, 2019

Support attr directive with static evaluation #1753

Merged

2 tasks

techtonik mentioned this pull request Sep 7, 2019

Support metadata.version in setup.cfg c4urself/bump2version#83

Closed

jaraco reviewed Sep 8, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setuptools.get_version(path, field='version') #1679

Add setuptools.get_version(path, field='version') #1679

techtonik commented Feb 10, 2019 •

edited

Loading

pganssle commented Feb 10, 2019

techtonik commented Feb 10, 2019

jaraco left a comment

jaraco Feb 17, 2019

techtonik Mar 20, 2019

jaraco Sep 8, 2019

jaraco Feb 17, 2019

techtonik Mar 20, 2019

jaraco Sep 8, 2019

jaraco Feb 17, 2019

techtonik Mar 20, 2019

jaraco Sep 7, 2019

jaraco Feb 17, 2019

techtonik Mar 20, 2019

jaraco Sep 7, 2019

pganssle commented Feb 17, 2019

lmittmann commented Feb 17, 2019

pganssle commented Feb 17, 2019 •

edited

Loading

jaraco commented Mar 9, 2019

techtonik commented Mar 20, 2019

jaraco left a comment

jaraco Sep 7, 2019

jaraco Sep 7, 2019

jaraco Sep 8, 2019

jaraco Sep 8, 2019

Add setuptools.get_version(path, field='__version__') #1679

Add setuptools.get_version(path, field='__version__') #1679

Conversation

techtonik commented Feb 10, 2019 • edited Loading

Summary of changes

Pull Request Checklist

pganssle commented Feb 10, 2019

techtonik commented Feb 10, 2019

jaraco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pganssle commented Feb 17, 2019

lmittmann commented Feb 17, 2019

pganssle commented Feb 17, 2019 • edited Loading

jaraco commented Mar 9, 2019

techtonik commented Mar 20, 2019

jaraco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add setuptools.get_version(path, field='version') #1679

Add setuptools.get_version(path, field='version') #1679

techtonik commented Feb 10, 2019 •

edited

Loading

pganssle commented Feb 17, 2019 •

edited

Loading