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

improve encoding handling for `setup.cfg` #1180

Merged
merged 9 commits into from Jan 27, 2019

Conversation

Projects
None yet
4 participants
@benoit-pierre
Copy link
Member

benoit-pierre commented Oct 25, 2017

Support the same mechanism as for Python sources for declaring the encoding to be used when reading setup.cfg (see PEP 263), and return the results of reading it as Unicode.

Fix #1062 and #1136.

benoit-pierre added some commits Oct 25, 2017

improve encoding handling for `setup.cfg`
Support the same mechanism as for Python sources for declaring
the encoding to be used when reading `setup.cfg` (see PEP 263),
and return the results of reading it as Unicode.

Fix #1062 and #1136.
@jaraco
Copy link
Member

jaraco left a comment

Does it make sense to defer this work until setuptools adopts distutils (#417)?

Show resolved Hide resolved setuptools/py36compat.py Outdated
@@ -33,11 +47,16 @@ def parse_config_files(self, filenames=None):
if DEBUG:
self.announce("Distribution.parse_config_files():")

parser = ConfigParser(interpolation=None)
parser = ConfigParser()

This comment has been minimized.

@jaraco

jaraco Oct 25, 2017

Member

I know interpolation causes problems when a percent appears in the setup.cfg (such as for a password). I think there are tests to protect this behavior (perhaps only in CPython).

This comment has been minimized.

@benoit-pierre

benoit-pierre Oct 25, 2017

Author Member

It's the other way round, interpolation as always been enabled, see: #889 (comment)

This compatibility code was actually not used, that's why I added a test first to make sure this behavior (interpolation enabled) did not change.

This comment has been minimized.

@jaraco

jaraco Nov 20, 2017

Member

Oh, right. Good point. I wonder then if that means we (I) need to update the code in distutils for Python 3.7 to restore interpolation before it goes live.

This comment has been minimized.

@jaraco

jaraco Nov 20, 2017

Member

Okay, reviewing the history on the CPython issue, I see that there's not yet a patch in CPython that's affected by this change.

@benoit-pierre benoit-pierre force-pushed the benoit-pierre:fix_889_and_non-ascii_in_setup.cfg_take_2 branch from e110db5 to 2c897b5 Oct 25, 2017

@benoit-pierre

This comment has been minimized.

Copy link
Member Author

benoit-pierre commented Oct 25, 2017

Reworked and slightly expanded tests.

@benoit-pierre

This comment has been minimized.

Copy link
Member Author

benoit-pierre commented Oct 25, 2017

I agree that this could wait for distutils code to be included in setuptools.

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Nov 20, 2017

We can either wait for the distutils code to be adopted by Setuptools or we can simply override the parse_config_files in Distribution (and own it, rather than working to rely on the upstream version). I'm slightly inclined toward the latter, not knowing when/if distutils will land. Would you like to work on those changes or would you rather I do it (I feel some responsibility having put the compatibility layer in place)?

@benoit-pierre benoit-pierre added the draft label Dec 22, 2017

@hmarlo hmarlo referenced this pull request Jan 24, 2018

Closed

package_dir in setup.cfg #1259

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Contributor

RonnyPfannschmidt commented Jun 21, 2018

ouch, this is still an issue :/

@c00kiemon5ter

This comment has been minimized.

Copy link

c00kiemon5ter commented Jul 3, 2018

Hi and thanks for working on this. I also got bitten by #1136 and followed the thread here. Is this planned to be merged?

c00kiemon5ter added a commit to c00kiemon5ter/pysaml2 that referenced this pull request Jul 3, 2018

Switch from setup.py to setup.cfg
A bug is blocking setuptools from working with python2 [bug]. Work is on its
way [pr]. Until that is fixed, package_dir should be defined in setup.py to
preserve compatibility of the native str type.

  [bug]: pypa/setuptools#1136
  [pr]: pypa/setuptools#1180

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jul 5, 2018

I think we do basically want to adopt this change, but as part of a refactor to have setuptools take ownership of the relevant functionality from distutils (rather than trying to contribute it upstream first, which was the intention here).

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jan 25, 2019

I'm sorry to have left this to languish for so long. I want to accept these changes with some small tweaks (mainly proper adoption of the config file parsing functionality). I'll resolve the conflicts and press forward.

@jaraco

jaraco approved these changes Jan 25, 2019

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jan 25, 2019

I believe this work is ready to go, but tests are failing due to #1644.

@jaraco jaraco closed this Jan 27, 2019

@jaraco jaraco reopened this Jan 27, 2019

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jan 27, 2019

There's still a failing test on Python 2.7. I'm not yet clear on why.

@benoit-pierre

This comment has been minimized.

Copy link
Member Author

benoit-pierre commented Jan 27, 2019

That's an issue somewhere in the bowels of distutils, when handling boolean options:

 setuptools/dist.py | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git i/setuptools/dist.py w/setuptools/dist.py
index 4cc3bdfe..6cabfb60 100644
--- i/setuptools/dist.py
+++ w/setuptools/dist.py
@@ -12,6 +12,7 @@
 import distutils.cmd
 import distutils.dist
 from distutils.errors import DistutilsOptionError
+from distutils.fancy_getopt import translate_longopt
 from distutils.util import strtobool
 from distutils.debug import DEBUG
 import itertools
@@ -560,6 +561,50 @@ def _clean_req(self, req):
         req.marker = None
         return req
 
+    def _set_command_options(self, command_obj, option_dict=None):
+        """Set the options for 'command_obj' from 'option_dict'.  Basically
+        this means copying elements of a dictionary ('option_dict') to
+        attributes of an instance ('command').
+
+        'command_obj' must be a Command instance.  If 'option_dict' is not
+        supplied, uses the standard option dictionary for this command
+        (from 'self.command_options').
+        """
+        command_name = command_obj.get_command_name()
+        if option_dict is None:
+            option_dict = self.get_option_dict(command_name)
+
+        if DEBUG:
+            self.announce("  setting options for '%s' command:" % command_name)
+        for (option, (source, value)) in option_dict.items():
+            if DEBUG:
+                self.announce("    %s = %s (from %s)" % (option, value,
+                                                         source))
+            try:
+                bool_opts = [translate_longopt(o)
+                             for o in command_obj.boolean_options]
+            except AttributeError:
+                bool_opts = []
+            try:
+                neg_opt = command_obj.negative_opt
+            except AttributeError:
+                neg_opt = {}
+
+            try:
+                is_string = isinstance(value, six.string_types)
+                if option in neg_opt and is_string:
+                    setattr(command_obj, neg_opt[option], not strtobool(value))
+                elif option in bool_opts and is_string:
+                    setattr(command_obj, option, strtobool(value))
+                elif hasattr(command_obj, option):
+                    setattr(command_obj, option, value)
+                else:
+                    raise DistutilsOptionError(
+                        "error in %s: command '%s' has no such option '%s'"
+                        % (source, command_name, option))
+            except ValueError as msg:
+                raise DistutilsOptionError(msg)
+
     def _parse_config_files(self, filenames=None):
         """
         Adapted from distutils.dist.Distribution.parse_config_files,
@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jan 27, 2019

The remaining test failure (just one) is due to an option "tag-date=0" being parsed as u'0' but on Python 2 not being interpreted as isinstance(tag_date, str) (ref).

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jan 27, 2019

@benoit-pierre Assuming tests pass (they do for me locally), would you please review the latest changes?

@benoit-pierre

This comment has been minimized.

Copy link
Member Author

benoit-pierre commented Jan 27, 2019

Well, I can't approve my own PR, but looks good to me!

@jaraco jaraco merged commit b31997d into pypa:master Jan 27, 2019

4 of 5 checks passed

codecov/patch 81.11% of diff hit (target 82.32%)
Details
codecov/project 82.51% (+0.18%) compared to 050c0cb
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

@benoit-pierre benoit-pierre deleted the benoit-pierre:fix_889_and_non-ascii_in_setup.cfg_take_2 branch Jan 27, 2019

@benoit-pierre benoit-pierre removed the draft label Jan 28, 2019

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