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

Allow setting long_description_content_type externally #1343

Merged
merged 1 commit into from May 18, 2018

Conversation

Projects
None yet
6 participants
@ianw
Contributor

ianw commented Apr 30, 2018

Some tools, such as PBR, might want to set long_description_content_type externally. Modify the extant code to take the existing value from dist.metadata if it is already set.

I believe the path for this is that _Distribution.__init__() gets called, which calls the distutils initalizer, which calls into https://git.openstack.org/cgit/openstack-dev/pbr/tree/pbr/core.py. This __init__ call has setup dist.metadata within the object.

PBR then reads setup.cfg and populates a range of fields into the dist object, including some metadata. However, it is not possible for it to set the long_description_content_type because setuptools only takes it from its own attr argument after the distutils initalizers have run.

By making setuptools check if there is already a value for it in metadata, we can modify PBR to write the value to dist.metadata and have it maintained.

This is a problem unique to this field, as it's not part of the base distutils metadata

# Some initalizers may pre-set this, otherwise use from attr
self.metadata.long_description_content_type = getattr(
self.metadata, 'long_description_content_type',
attrs.get('long_description_content_type')

This comment has been minimized.

@di

di Apr 30, 2018

Member

Does it make sense to guard against the case where self.metadata.long_description_content_type might be None but attrs.get('long_description_content_type') has some value? I'm not sure how that could happen but if it did, the actual value would be ignored with this change.

This comment has been minimized.

@ianw

ianw Apr 30, 2018

Contributor

If I'm understanding, that would imply something like PBR explicitly set self.metadata.long_description_content_type = None but setup(... long_description_content_type='foo' ..) also had a value.

It think it would be fine to always take the value from attrs, if set

@pganssle

This comment has been minimized.

Member

pganssle commented Apr 30, 2018

I don't know if I understand the meaning of "externally" in this context. Under what conditions would long_description_content_type be set other than through a call to setup?

Also, this PR needs tests.

@ianw

This comment has been minimized.

Contributor

ianw commented Apr 30, 2018

@pganssle

I don't know if I understand the meaning of "externally" in this context. Under what conditions would long_description_content_type be set other than through a call to setup?

So a PBR based installer uses simply

setuptools.setup(
    setup_requires=['pbr>=2.0.0'],
    pbr=True)

The pbr argument basically ends up in https://git.openstack.org/cgit/openstack-dev/pbr/tree/pbr/core.py ; which reads a setup.cfg and you can see how it populates various fields of the Distribution object from that.

This works for updating all the original distutils-based metadata settings, etc. But long_description_content_type is different in that setuptools unconditionally sets it after the PBR setup() keyword handler has run. Hence this change to allow the value to populate there first and not be overwritten.

I'm open to ideas on what testing you'd like to see. The tests as is ensure that this didn't regress and not set the value from attr.

@pganssle

This comment has been minimized.

Member

pganssle commented Apr 30, 2018

@ianw There's no test to ensure that this behavior isn't broken by future updates, since this change does not introduce any new breaking tests.

The tests should also address things like @di's comments - what happens if setup gets passed long_description_content_type and self.metadata is set? Does self.metadata override or does attr? Should there be a warning?

@ianw ianw force-pushed the ianw:long-descr-type branch 3 times, most recently from 3bd9a5d to 89c4908 May 1, 2018

dist.long_description_content_type = 'text/something'
return None
@patch.object(_Distribution, '__init__', autospec=True)

This comment has been minimized.

@pganssle

pganssle May 1, 2018

Member

Is this actually how third party libraries extending setuptools are expected to set metadata.long_description_type? If so, I'm not sure that's something we should be supporting. _Distribution is not a public class, so doing anything to it involves relying on implementation details.

This comment has been minimized.

@ianw

ianw May 1, 2018

Contributor

This is simulating an external project that has set/accessed values via a distutils.setup_keywords entry-point in the unit test (which happens opaquely during _Distribution.__init__() call in dist.py). I don't see it as exposing _Distribution details outside setuptools.

@@ -356,19 +356,27 @@ def __init__(self, attrs=None):
self.src_root = attrs.pop("src_root", None)
self.patch_missing_pkg_info(attrs)
self.project_urls = attrs.get('project_urls', {})
self.long_description_content_type = attrs.pop(

This comment has been minimized.

@pganssle

pganssle May 1, 2018

Member

I don't think it's a good idea to store this on the Distribution instance like this. The way this is structured now, dist.long_description_content_type and dist.metadata.long_description_content_type could be two different values, which is a very confusing API - it's not clear which one is used and why they both persist.

The best options I see here are:

  1. The attrs version overrides any pre-existing self.metadata version.
  2. The self.metadata overrides any attrs version
  3. The same as 1 or 2, except that a warning is raised if both are specified.
  4. A long_description_content_type_default can be specified in self.metadata, and that is used if and only if long_description_content_type is not specified.

I think 4 is the least ambiguous but it does expand the API so I suppose it shouldn't be done lightly.

@jaraco @benoit-pierre Any thoughts on this?

This comment has been minimized.

@ianw

ianw May 1, 2018

Contributor

A long_description_content_type_default can be specified in self.metadata, and that is used if and only if long_description_content_type is not specified.

I think 4 is the least ambiguous but it does expand the API so I suppose it shouldn't be done lightly.

I'd be happy with this. I don't know if https://git.openstack.org/cgit/openstack-dev/pbr/tree/pbr/core.py#n106 setting a whole bunch of variables mapped out in https://git.openstack.org/cgit/openstack-dev/pbr/tree/pbr/util.py#n93 is actually an API, as such ... or just something that works well enough. I suspect the latter; but so far out of all of them, this content-type is the only one that can not be set outside of setup() arguments, because it is overwritten unconditionally.

This comment has been minimized.

@benoit-pierre

benoit-pierre May 2, 2018

Member

The sequence when using PBR is this:

  • setuptools.setup is called
    • distutils.core.setup is called
      • setuptools.dist.Distribution.__init__ is called
        • distutils.dist.Distribution.__init__ is called, and the Distribution.metadata field is set and populated from attrs
          • Distribution.finalize_options is called
            • pbr.core.pbr is called, overriding part of Distribution.metadata
        • missing Distribution.metadata fields are filled-in (this is where long_description_content_type is overriden even if already set)
      • setup.cfg is loaded, overriding parts of Distribution.metadata

So setup.cfg options override PBR options which in turn override setup() options (except for long_description_content_type).

I dislike how project_urls is currently handled (setting it both at the Distribution level and later at the Distribution.metadata level), and for this reason I'm not in favor of replicating the behavior with long_description_content_type.

If we want to keep:

  • using distutils to print a warning for unsupported (by setuptools) options
  • but avoid warnings for setuptools specific options or metadata options not supported by distutils
    then this is how I would patch the current code:
 setuptools/dist.py | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git i/setuptools/dist.py w/setuptools/dist.py
index 321ab6b7..34267afc 100644
--- i/setuptools/dist.py
+++ w/setuptools/dist.py
@@ -328,6 +328,12 @@ class Distribution(Distribution_parse_config_files, _Distribution):
     distribution for the included and excluded features.
     """
 
+    _DISTUTILS_UNSUPPORTED_METADATA = {
+        'long_description_content_type': None,
+        'project_urls': dict,
+        'provides_extras': set,
+    }
+
     _patched_dist = None
 
     def patch_missing_pkg_info(self, attrs):
@@ -353,25 +359,28 @@ class Distribution(Distribution_parse_config_files, _Distribution):
         self.require_features = []
         self.features = {}
         self.dist_files = []
+        # Filter-out setuptools' specific options.
         self.src_root = attrs.pop("src_root", None)
         self.patch_missing_pkg_info(attrs)
-        self.project_urls = attrs.get('project_urls', {})
         self.dependency_links = attrs.pop('dependency_links', [])
         self.setup_requires = attrs.pop('setup_requires', [])
         for ep in pkg_resources.iter_entry_points('distutils.setup_keywords'):
             vars(self).setdefault(ep.name, None)
-        _Distribution.__init__(self, attrs)
-
-        # The project_urls attribute may not be supported in distutils, so
-        # prime it here from our value if not automatically set
-        self.metadata.project_urls = getattr(
-            self.metadata, 'project_urls', self.project_urls)
-        self.metadata.long_description_content_type = attrs.get(
-            'long_description_content_type'
-        )
-        self.metadata.provides_extras = getattr(
-            self.metadata, 'provides_extras', set()
-        )
+        _Distribution.__init__(self, {
+            k: v for k, v in attrs.items()
+            if k not in self._DISTUTILS_UNSUPPORTED_METADATA
+        })
+
+        # Fill-in missing metadata fields; not supported
+        # by distutils or set by other tools (e.g. pbr).
+        for option, default in self._DISTUTILS_UNSUPPORTED_METADATA.items():
+            for source in self.metadata.__dict__, attrs:
+                if option in source:
+                    value = source[option]
+                    break
+            else:
+                value = default() if default else None
+            setattr(self.metadata, option, value)
 
         if isinstance(self.metadata.version, numbers.Number):
             # Some people apparently take "version number" too literally :)

This comment has been minimized.

@ianw

ianw May 4, 2018

Contributor

@benoit-pierre I am happy with that if you are, and I've translated it into this patch (please re-upload with yourself as author if you wish, I'm not sure how things work around here)

+            for source in self.metadata.__dict__, attrs:
+                if option in source:
+                    value = source[option]
+                    break

One thing I wasn't sure about was above. I think the intention is for attrs to override anything set by self.metadata, so we want to walk along the attrs entry first. I swapped it and this is what the test-case enforces; if that's not the intention it will have to be reworked.

I have tested this with the pbr change https://review.openstack.org/565177 and it seems OK

This comment has been minimized.

@benoit-pierre

benoit-pierre May 4, 2018

Member

I don't think attrs should override metadata: even if I find the behavior unintuitive, settings in setup.cfg do override those passed as argument to setup.

This comment has been minimized.

@ianw

ianw May 4, 2018

Contributor

I don't think attrs should override metadata:

OK, rereading your comment I guess I misunderstood that you thought it was wrong. 59ebc0f implements metadata overwriting attrs

@ianw ianw force-pushed the ianw:long-descr-type branch 3 times, most recently from 59ebc0f to 16b5cf8 May 4, 2018

@jmbowman

This comment has been minimized.

Contributor

jmbowman commented May 16, 2018

My nitpicks aside, is there anything still preventing this from being merged? It would be nice to add a changelog.d entry now that we're using towncrier to manage the changelog.

I didn't see anything in the documentation regarding precedence of attributes in setup.cfg vs. setup(); did I just miss it? Should we add that?

@pganssle

This comment has been minimized.

Member

pganssle commented May 16, 2018

I'm personally still a bit uncomfortable with this because the precedence is both undocumented and for the other members not what I would expect.

I think it's a foregone conclusion that this will have to behave like other attributes, but I'd like us to at least agree on what the strategy is and what it should be, and preferably document it.

I also haven't merged it because I have not convinced myself that the way it's being tested is actually a supported way to do this.

Allow setting long_description_content_type externally
Some tools, such as PBR, might want to set
long_description_content_type during the parent object's
Distribution.__init__() call (during distutils setup_keywords entry
points).  However, that field is currently unconditionally overwritten
after these calls, erasing the value.

We would rather not duplicate the existing method of copying into
dist.metadata as done with project_urls.

This preserves the fields within Distribution.metadata described by
self._DISTUTIULS_UNUPPORTED_METADATA, or otherwise takes it from
arguments.

A test case that simulates setting the long description and overriding
the arguments is added.

@ianw ianw force-pushed the ianw:long-descr-type branch from 16b5cf8 to 07cd2e4 May 16, 2018

@ianw

This comment has been minimized.

Contributor

ianw commented May 16, 2018

It would be nice to add a changelog.d entry now that we're using towncrier to manage the changelog.

Done

@jaraco

This comment has been minimized.

Member

jaraco commented May 18, 2018

My initial instinct was to agree with Paul on this one. I thought this change might complicate efforts for Setuptools to adopt Distutils. Also, at first blush, this change looked like hacks on hacks.

But as I continue to stare at this change, and try to understand both the left and right sides of this change, I do see some advantage in normalizing the behavior around these three fields, whereas the current behavior is inconsistent.

Therefore, I believe this change is worthwhile and we should adopt it. It does formalize the order of initialization for these three metadata fields so that future fields might also not fail to be overridden.

@jaraco jaraco merged commit 239ff9b into pypa:master May 18, 2018

5 checks passed

codecov/patch 100% of diff hit (target 81.46%)
Details
codecov/project 81.49% (+0.02%) compared to 1252d1b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment