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

Replace optparse with argparse in setup.py #73628

Closed
yan12125 mannequin opened this issue Feb 4, 2017 · 21 comments
Closed

Replace optparse with argparse in setup.py #73628

yan12125 mannequin opened this issue Feb 4, 2017 · 21 comments
Labels
3.8 only security fixes build The build process and cross-build type-feature A feature request or enhancement

Comments

@yan12125
Copy link
Mannequin

yan12125 mannequin commented Feb 4, 2017

BPO 29442
Nosy @brettcannon, @vstinner, @skrah, @xdegaye, @serhiy-storchaka, @yan12125
PRs
  • bpo-29442: Replace optparse with argparse in setup.py #139
  • Dependencies
  • bpo-30152: Reduce the number of imports for argparse
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2018-07-11.08:49:12.934>
    created_at = <Date 2017-02-04.07:26:28.157>
    labels = ['type-feature', '3.8', 'build']
    title = 'Replace optparse with argparse in setup.py'
    updated_at = <Date 2018-07-11.10:18:53.398>
    user = 'https://github.com/yan12125'

    bugs.python.org fields:

    activity = <Date 2018-07-11.10:18:53.398>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-07-11.08:49:12.934>
    closer = 'methane'
    components = ['Build']
    creation = <Date 2017-02-04.07:26:28.157>
    creator = 'yan12125'
    dependencies = ['30152']
    files = []
    hgrepos = []
    issue_num = 29442
    keywords = ['patch']
    message_count = 21.0
    messages = ['286922', '286977', '287691', '287696', '287700', '287702', '287705', '287855', '287956', '287964', '288125', '288127', '288132', '288159', '292091', '292117', '292201', '292210', '303052', '321416', '321431']
    nosy_count = 6.0
    nosy_names = ['brett.cannon', 'vstinner', 'skrah', 'xdegaye', 'serhiy.storchaka', 'yan12125']
    pr_nums = ['139']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue29442'
    versions = ['Python 3.8']

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Feb 4, 2017

    The change is clear and self-explained. See the patch.

    Motivations:

    1. The hack "To prevent optparse from raising an exception..." works for single letter flags (-I, -L, etc.) only. I plan to add --sysroot related codes for Android builds and I don't want more hacks. Apparently argparse does not need this hack as it can parse all known flags before throwing an error
    2. optparse is deprecated

    Add the developer that introduced this hack. Also Serhiy, who sseems interested in removing optparse from the code base. (bpo-18973, bpo-18971)

    @yan12125 yan12125 mannequin added 3.7 (EOL) end of life build The build process and cross-build type-feature A feature request or enhancement labels Feb 4, 2017
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Feb 4, 2017

    I didn't look at this one, but some "hacks" are necessary in setup.py (example: usage of os.system() in some places).

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Feb 13, 2017

    I've found a simpler patch set for supporting Android builds, so I don't need this patch anymore. If you don't think it's necessary, just close it.

    usage of os.system() in some places

    I'm indeed sick with them :D

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Feb 13, 2017

    It's not a matter of *necessary*, you simply cannot use argparse in setup.py. Try to build with your patch and you'll see.

    @skrah skrah mannequin closed this as completed Feb 13, 2017
    @skrah skrah mannequin added the invalid label Feb 13, 2017
    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Feb 13, 2017

    I have used my old patch several days on Android, and it seems quite fine. Anyway that's not important anymore.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Feb 13, 2017

    On Mon, Feb 13, 2017 at 03:04:09PM +0000, Chi Hsuan Yen wrote:

    Chi Hsuan Yen added the comment:

    I have used my old patch several days on Android, and it seems quite fine. Anyway that's not important anymore.

    I find that very surprising:

    ./python -E -S -m sysconfig --generate-posix-vars ;\
            if test $? -ne 0 ; then \
                    echo "generate-posix-vars failed" ; \
                    rm -f ./pybuilddir.txt ; \
                    exit 1 ; \
            fi
    Traceback (most recent call last):
      File "./setup.py", line 4, in <module>
        import sys, os, importlib.machinery, re, argparse
      File "/home/stefan/althome/pydev/commit-master-LATEST/Lib/argparse.py", line 93, in <module>
        from gettext import gettext as _, ngettext
      File "/home/stefan/althome/pydev/commit-master-LATEST/Lib/gettext.py", line 49, in <module>
        import locale, copy, io, os, re, struct, sys
      File "/home/stefan/althome/pydev/commit-master-LATEST/Lib/struct.py", line 13, in <module>
        from _struct import *
    ModuleNotFoundError: No module named '_struct'

    Yes, it is important, because you called the original a "dirty hack", which
    some core devs do not appreciate.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Feb 13, 2017

    You're right. argparse has indirect dependency on dynamic modules so it can't be used in setup.py. Cross builds use prebuilt Python binaries so there's no problem. Sorry for those noises.

    I still think it's a dirty hack as it limits scalability and brings lots of problems when adding new features, but I can't find a good way to remove it :(

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Feb 15, 2017

    Aha the fix is simple => bpo-29567

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Feb 16, 2017

    Now I have a working patch at PR 139, and dropping optparse is still the way to go, so I reopen it.

    I'm not familiar with CPython conventions. Sorry if reopening an issue is impolite.

    @yan12125 yan12125 mannequin reopened this Feb 16, 2017
    @yan12125 yan12125 mannequin removed the invalid label Feb 16, 2017
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Feb 16, 2017

    It's okay to reopen if some conditions have changed (which is the case here).

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Feb 19, 2017

    I'm not familiar with CPython conventions. Sorry if reopening an issue is impolite.

    IMO asking if it's ok to reopen this issue by using the word 'impolite' is a clear reference to the fact that Stefan had told you previously that you are being insulting by saying "it's a dirty hack ". Not only did not you take the opportunity given by Stefan at that time to express your apologies as expected, but instead you repeated it, and here your are making fun of him by using the word 'impolite' when you could have asked if it is correct or allowed to reopen an issue.

    Few months ago you uttered the same kind of insults then directed at me in msg264605, a post addressed to Guido van Rossum in bpo-26858.

    Please do read the CPython Code of Conduct and try to remember that the people that take the time to answer your posts do it on their own free time and that you should be nice to them.

    I am nosying Brett Cannon, the CPython maintainer and core dev in charge of the respect of the CoC.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Feb 19, 2017

    I see your points. Indeed in many times I didn't think carefully before leaving a comment, and not even noticing that my comments can be offensive afterwards. I apologize for all those cases and I'll be more careful when interacting with others in the future.

    @xavier: I'm gratitude for your patience on taking so much time explaining what I've done wrong. It's an invaluable course.

    Let me change the title of this issue first. That will make it more moderate and also clearer.

    @yan12125 yan12125 mannequin changed the title Use argparse and drop dirty optparse hacks in setup.py Replace optparse with argparse in setup.py Feb 19, 2017
    @serhiy-storchaka
    Copy link
    Member

    (example: usage of os.system() in some places)

    See similar bpo-7438.

    @brettcannon
    Copy link
    Member

    I just wanted to acknowledge that I read this thread. I accept Chi's acknowledgement of his actions and I hope there aren't future missteps, but please also realize that there has been a warning about how you communicate here, Chi.

    @vstinner
    Copy link
    Member

    Did someone test the patch? Does Python still bootstrap after a distclean or not? If not, I suggest to close the issue, except if Chi Hsuan Yen finf an elegant way to fix them :-)

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Apr 22, 2017

    I guess you were asking whether the newly compiled Python binary is able to build extension modules or not? I rebased PR 139 against the latest git master and seems it's fine. Running make distclean, ./configure and then make builds all modules as expected. The result is the same on Travis CI and Appveyor.

    About "elegant": I guess the only issue regarding elegancy in my patch is the local import in GNUTranslations._parse(). For me moving setup.py away from deprecated modules is a good reason for introducing local imports. Does it sound OK?

    @serhiy-storchaka
    Copy link
    Member

    See also bpo-30152. It includes Chi's change for argparse (actually it was inspired by Chi's change) and much more.

    @vstinner
    Copy link
    Member

    I consider that the issue bpo-30152 is now a dependency of this change.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Sep 26, 2017

    Thanks to Serhiy Storchaka's work in bpo-30152, this patch can be simplified. I've rebased my branch and updated the pull request.

    @methane
    Copy link
    Member

    methane commented Jul 11, 2018

    New changeset 09b2bec by INADA Naoki (Chih-Hsuan Yen) in branch 'master':
    bpo-29442: Replace optparse with argparse in setup.py (GH-139)
    09b2bec

    @methane methane added 3.8 only security fixes and removed 3.7 (EOL) end of life labels Jul 11, 2018
    @methane methane closed this as completed Jul 11, 2018
    @vstinner
    Copy link
    Member

    bpo-29442: Replace optparse with argparse in setup.py (GH-139)

    I'm surprised that it works, but I tested manually and I confirm that it works :-)

    $ git clean -fdx
    $ ./configure --with-pydebug
    $ make
    $ ./python -c "import select"

    No compilation error ;-)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes build The build process and cross-build type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants