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

extras_require in setup.cfg doesn't allow hyphen in extras name #1608

Closed
jdufresne opened this issue Dec 14, 2018 · 11 comments · Fixed by #2588
Closed

extras_require in setup.cfg doesn't allow hyphen in extras name #1608

jdufresne opened this issue Dec 14, 2018 · 11 comments · Fixed by #2588
Labels
Needs Triage Issues that need to be evaluated for severity and status.

Comments

@jdufresne
Copy link
Contributor

jdufresne commented Dec 14, 2018

I'm porting a setup.py file to declarative setup.cfg. In this package, an extra contains a hyphen. This works when defined in setup.py but does not when defined in setup.cfg. If I change the hyphen to an underscore, it works again, but this would break the extra for users.

To reproduce:

setup.py:

from setuptools import setup

setup()

setup.cfg:

[options.extras_require]
extra-a =
    pytest
extra_b =
    pytest

tox.ini (to automate the virtual env creation)

[testenv:a]
extras = extra-a

[testenv:b]
extras = extra_b

Then run:

$ tox -e a
$ tox -e b

Notice the extras are installed in the "b" environment but not the "a" environment. AFAICT, the only difference is the use of a hyphen instead of an underscore.

@jdufresne
Copy link
Contributor Author

It looks like distutils automatically converts hyphens to underscores internally when reading setup.cfg:

https://github.com/python/cpython/blob/4aa917c5feaec07a6f6db87b34185ab6180e20ee/Lib/distutils/dist.py#L414

@pganssle pganssle added the Needs Triage Issues that need to be evaluated for severity and status. label Dec 14, 2018
@pganssle
Copy link
Member

@jdufresne According to this guide, underscores, hyphens and periods are all supposed to be treated equivalently.when normalizing names, referencing PEP 508, but I don't see anything about that there or in PEP 423.

Presumably if extra-a and extra_a are actually equivalent, this should not be an issue, right? If that's the case, why are you seeing a difference in behavior?

@jdufresne
Copy link
Contributor Author

Thanks for taking a look. In that case, do you think this is an issue in pip and not setuptools? If I run the command:

pip install .[extra-a]

The extras are not installed. But if I run the command

pip install .[extra_a]

The extras are installed.

Should I file this issue with pip?

@pganssle
Copy link
Member

I'm not quite sure if there's an authoritative claim that those should be treated the same or if it was describing the behavior of a specific tool.

It could be that pip is not normalizing correctly or it could be that we should be normalizing to extra_a, or that none of us should be doing any normalizing at all.

We'll at least leave this ticket open until we get to the bottom of this. Presumably a change needs to be made in at least one of: setuptools, pip, the packaging tutorials.

@jaraco
Copy link
Member

jaraco commented Jan 8, 2019

According to this guide, underscores, hyphens and periods are all supposed to be treated equivalently when normalizing names

When I read that reference, I see explicit mention of "project" names, but not necessarily other names.

I'm strongly in favor of honoring the user's intention here and if for nothing other than feature parity, support untransformed names for extras from declarative config.

@jaraco
Copy link
Member

jaraco commented Jan 8, 2019

That distutils code isn't readily tweakable. I think the preferred solution is for setuptools not to rely on distutils to parse the options.

@jaraco
Copy link
Member

jaraco commented Jan 8, 2019

Ugh. The setuptools.config implementation is really intertwined with distutils. Fixing this bug might prove more difficult than adopting distutils and fixing it. Regardless, it's not something I'll be able to fix this afternoon as I'd hoped.

@jaraco jaraco removed their assignment Jan 8, 2019
@jaraco
Copy link
Member

jaraco commented Jan 8, 2019

Here's the start of the idea I have in mind:

diff --git a/setuptools/dist.py b/setuptools/dist.py
index 7062ae8d..e92e7779 100644
--- a/setuptools/dist.py
+++ b/setuptools/dist.py
@@ -30,7 +30,7 @@ from . import SetuptoolsDeprecationWarning
 from setuptools.depends import Require
 from setuptools import windows_support
 from setuptools.monkey import get_unpatched
-from setuptools.config import parse_configuration
+from . import config
 import pkg_resources
 from .py36compat import Distribution_parse_config_files
 
@@ -563,8 +563,10 @@ class Distribution(Distribution_parse_config_files, _Distribution):
         """
         _Distribution.parse_config_files(self, filenames=filenames)
 
-        parse_configuration(self, self.command_options,
-                            ignore_option_errors=ignore_option_errors)
+        cfg = config.read_configuration(
+            filenames=filenames, ignore_option_errors=ignore_option_errors,
+        )
+        config.apply_configuration(self, cfg)
         self._finalize_requires()
 
     def parse_command_line(self):

I'm suggesting a refactor of setuptools.config that decouples reading the config from applying that config to a distribution.

@stephen-dexda
Copy link

This breaks data_files, since its target directories are keys setup.cfg, so the following:

/some-path/to-dir =
    some/file

will install the file to /some_path/to_dir/some/file.

@jaraco
Copy link
Member

jaraco commented Mar 1, 2021

When this bug was filed, setuptools relied on distutils to parse the config files. Since then, in 24be5ab, the code was inlined, so it should be straightforward to adjust the parsing behavior. I believe it's worth seriously considering just dropping that substitution.

@melissa-kun-li
Copy link
Contributor

As Jason mentioned, since the config parsing code is now within Setuptools, I believe this line is responsible for turning hyphens to underscores: https://github.com/pypa/setuptools/blob/main/setuptools/dist.py#L601. I wonder if we can simply remove the line, but I'm not sure if there are disadvantages to doing this.

This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Triage Issues that need to be evaluated for severity and status.
Projects
None yet
5 participants