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

Preserve order of egg_info section in setup.cfg #553

Merged
merged 1 commit into from Apr 20, 2016

Conversation

Projects
None yet
3 participants
@dims
Contributor

dims commented Apr 18, 2016

egg_info is the dictionary with information that is injected
into setup.cfg. edit_config uses RawConfigParser which uses
collections.OrderedDict for all the data. Since we use a
simple dict(), when we loop through items in edit_config, we
see random behavior as a result the fields
tag_svn_revision/tag_date/tag_build are added to the setup.cfg
randomly. Let's add a OrderedDict to make it more predictable.

@dims dims force-pushed the dims:fix-order-fields-setup-cfg branch from 6706943 to f8f9df2 Apr 18, 2016

@rbtcollins

This comment has been minimized.

Contributor

rbtcollins commented Apr 18, 2016

If we're going to make this something folk depend on, it really needs a test so that future developers don't break it by mistake.

@jaraco

This comment has been minimized.

Member

jaraco commented Apr 19, 2016

Yes. I also think a test is called for. I'm not sure the implementation as written will work as I believe the keyword arguments will be ordered according to the PYTHONHASHSEED.

@dims dims force-pushed the dims:fix-order-fields-setup-cfg branch 2 times, most recently from a7dfb65 to ea99ec1 Apr 19, 2016

@dims

This comment has been minimized.

Contributor

dims commented Apr 19, 2016

@rbtcollins @jaraco i've added a test and sorted the items by key. Seems to work. Please take a look.

@jaraco

This comment has been minimized.

Member

jaraco commented Apr 19, 2016

It seems that the tests aren't capturing the new expectation in egg_info.py. If I apply the following patch to your branch, the tests still pass:

diff -r f08b20844fbd setuptools/command/egg_info.py
--- a/setuptools/command/egg_info.py    Mon Apr 18 15:08:49 2016 -0400
+++ b/setuptools/command/egg_info.py    Tue Apr 19 12:20:13 2016 -0400
@@ -2,11 +2,6 @@

 Create a distribution's .egg-info directory and contents"""

-try:
-    from collections import OrderedDict
-except ImportError:
-    OrderedDict = dict
-
 from distutils.filelist import FileList as _FileList
 from distutils.util import convert_path
 from distutils import log
@@ -72,10 +67,10 @@

     def save_version_info(self, filename):
         values = dict(
-            egg_info=OrderedDict(
+            egg_info=dict(
+                tag_svn_revision=0,
+                tag_date=0,
                 tag_build=self.tags(),
-                tag_date=0,
-                tag_svn_revision=0,
             )
         )
         edit_config(filename, values)

That leads me to believe the OrderedDict isn't required at all.

Furthermore, this new pull request alters the way that all config files are generated, which may have implications for files other than setup.cfg and sections other than egg_info. Rather than retaining the current order, it specifies a new order (sorted lexicographically).

In my experimentation, RawConfigParser will retain order of any existing options. It's only when new items are added that order is relevant (and they're always appended).

From the perspective of 'edit_config', I think I'd prefer to give control to the caller as to what order the settings are applied, rather than forcing lexicographic ordering.

Could you elaborate on what is the issue you're encountering that's inspiring this change?

@dims

This comment has been minimized.

Contributor

dims commented Apr 19, 2016

Jason, Here's the background. We were trying to verify that old tarball(s) were not tampered with by generating fresh tar.gz(s) and comparing the two. Example output : https://etherpad.openstack.org/p/lJgI00FaLw

@dims

This comment has been minimized.

Contributor

dims commented Apr 19, 2016

in the end, i'd like to be able to run md5sum on generated tar.gz(s) so we need the files generated to be deterministic.

Preserve order of egg_info section in setup.cfg
egg_info is the dictionary with information that is injected
into setup.cfg. edit_config uses RawConfigParser which uses
collections.OrderedDict for all the data. When we use a
simple dict(), when we loop through items in edit_config, we
see random behavior as a result the fields
tag_svn_revision/tag_date/tag_build are added to the setup.cfg
randomly. So if we sort the items by key when we traverse items
we will get deterministic output as RawConfigParser uses
OrderedDict internally by default.

@dims dims force-pushed the dims:fix-order-fields-setup-cfg branch from ea99ec1 to df05ebf Apr 19, 2016

@dims

This comment has been minimized.

Contributor

dims commented Apr 19, 2016

@jaraco I don't know enough to say this fix is the best way to get what i want :) Any suggestions i'd be happy to explore

@rbtcollins

This comment has been minimized.

Contributor

rbtcollins commented Apr 19, 2016

@jaraco

This comment has been minimized.

Member

jaraco commented Apr 20, 2016

We were trying to verify that old tarball(s) were not tampered with by generating fresh tar.gz(s) and comparing the two.

So updating setuptools won't support this use case for existing old tarballs, as they were already potentially generated with the nondeterministic ordering. This will only allow new tarballs, generated by users using this new functionality, to support this use case.

So I think that determinism is worth aspiring to.

Agreed, especially if the determinism doesn't also preclude giving control to the user over what fields appear in which order.

I'm slightly annoyed by how Setuptools is injecting default values into all of these tarballs. I wonder if instead Setuptools should just stop injecting these values, if that would solve the issue at hand.

@jaraco

This comment has been minimized.

Member

jaraco commented Apr 20, 2016

Setuptools is injecting default values

That characterization isn't correct. These aren't default values, but overrides. Setuptools is replacing the user-supplied values, disabling 'tag_svn_revision' and 'tag_date', and instead materializing those values in 'tag_build'.

That makes sense, now that I think about it. You don't want those values being recalculated when the sdist is installed, but you want them materialized at build time so they're fixed to the build.

@jaraco jaraco merged commit df05ebf into pypa:master Apr 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jaraco added a commit that referenced this pull request Apr 20, 2016

Use OrderedDict to retain deterministic ordering of version info in e…
…gg_info command. Remove lexicographic ordering in setopt.edit_config. Ref #553

jaraco added a commit that referenced this pull request Apr 20, 2016

Move Python 2.6 exception into specific test, capturing that the expe…
…cted order is different on Python 2.6, not because of the lack of OrderedDict, but because of different behavior in RawConfigParser. Ref #553.

jaraco added a commit that referenced this pull request Apr 20, 2016

@jaraco

This comment has been minimized.

Member

jaraco commented Apr 20, 2016

@dims: Does this latest implementation suit your needs?

@dims

This comment has been minimized.

Contributor

dims commented Apr 20, 2016

@jaraco i'll test it and get back to you later today. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment