pypa / setuptools

Official project repository for the Setuptools build system
https://pypi.org/project/setuptools/
MIT License
2.44k stars 1.17k forks source link

distutils.FancyGetopt does not hadle repeating options with argument #2482

Open dnicolodi opened 3 years ago

dnicolodi commented 3 years ago

setuptools inherits command line parsing from distutils which uses the distutils.FancyGetopt class for setting up and performing the parsing. Documentation and comments in the code suggest that it exists a way to specify command line options that can be repeated. However, trying to use this facility for options that take arguments results in only the last specified argument to be retained. It works as expected for boolean options. I believe this is a bug that went so far unnoticed because the code base uses this feature only for the --verbose option which does not take arguments.

I know setuptools is moving away from command line invocation and toward a more declarative style. Is there any motivation to fix this? The patch for making distutils.FancyGetopt.getopt() do the right thing for repeated options with argumentsis just a few lines and I can submit a PR.

I currently monkey patch the method in my project. Some more monkey patching when it comes to setuptools does not even look that bad ;)

jaraco commented 3 years ago

I currently monkey patch the method in my project.

I would highly recommend not monkey-patching distutils. Already lots of platforms attempt to do this (numpy, debian, fedora, pypy, setuptools) and it's this monkey-patching that makes it nearly impossible to improve the system without breaking compatibility. You're of course welcome to do as you wish, but you'll immediately be fraught with challenges (which distutils to patch, stdlib or setuptools) and your package will break when setuptools moves these things around to make the world better.

Can we start with an example of why the current behavior is sub-optimal for your use-case?

dnicolodi commented 3 years ago

The current behavior is incorrect. A command defined like this

class example(Command):
    user_options = [('example', 'e', 'Example option', True)]
    boolean_options = ['example']

    def initialize_options(self):
        self.example = 0

    def finalize_options(self):
        pass

    def run(self):
        print(self.example)

behaves like this:

$ python setup.py example -e -e
running example
2

which seems reasonable. However, modifying the command like this:

class example(Command):
    user_options = [('example=', 'e', 'Example option', True)]
    boolean_options = []

    def initialize_options(self):
        self.example = None

    def finalize_options(self):
        pass

    def run(self):
        print(self.example)

results in this behavior:

$ python setup.py example -e foo -e bar
running example
bar

which is surprising, not consistent with the boolean option case, and not very useful. With my patch, the behavior is changed so that the latest example results in:

$ python setup.py example -e foo -e bar
running example
['foo', 'bar']

My use case is implementing a build_ext command that uses Meson to compile python extensions instead of distutils. The Meson way to configure the build is to have build arguments passed as a sequence of -Doption=value options. I want to emulate this command line for the build_ext command (actually for a separate config command). Of course I can come up with a different way of passing build options, but it is not trivial (without using repeated options I would need to concatenate options interleaved with a separator, but then you need a way to escape the separator, etc...) and I would like to avoid to deviate from the established Meson convention. Finally, it seems like a bug trivial to fix.

dnicolodi commented 3 years ago

I would highly recommend not monkey-patching distutils.

What other solution is there for someone that want to package Python extensions and bumps into distutils shortcomings? It seems that regular Python packages are moving to other packaging solutions that reimplement distutils and setuptools but do not provide a solution for python extensions.

In the response to the PR fixing this bug you mention that you consider only critical bug fixes for distutils, implying that this fix in behavior is not critical, thus implicitly defining a critical bug only a breakage of existing code using distutils. I thought that splitting distutils out of the standard library and vendoring it into setuptools was a way to speed up development. However, it seems that this is not the case and it seems that it will not be for a long time as the "complicated ecosystem challenges" that are blocking development now are most probably the same that have blocked development for the past decade or so.

jaraco commented 3 years ago

Well, the intention was to adopt distutils quickly, but it turns out that due to all of the monkey patching, adopting distutils is breaking those environments that did the monkey patching.

I do want to be able to evolve distutils faster, but I'm not confident setuptools will be able to reliably adopt distutils in the near term and want to limit divergence in a fork that's not yet stably merged.