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

Fail if an invalid -X option is provided #89608

Open
pablogsal opened this issue Oct 12, 2021 · 17 comments
Open

Fail if an invalid -X option is provided #89608

pablogsal opened this issue Oct 12, 2021 · 17 comments
Labels
3.13 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@pablogsal
Copy link
Member

BPO 45445
Nosy @pablogsal
PRs
  • bpo-45445: Fail if an invalid X-option is provided in the command line #28823
  • bpo-45445: Remove incorrectly commited test file #28972
  • 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 = None
    created_at = <Date 2021-10-12.13:39:46.718>
    labels = ['type-feature', '3.11']
    title = 'Fail if an invalid -X option is provided'
    updated_at = <Date 2021-10-15.21:39:44.389>
    user = 'https://github.com/pablogsal'

    bugs.python.org fields:

    activity = <Date 2021-10-15.21:39:44.389>
    actor = 'pablogsal'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = []
    creation = <Date 2021-10-12.13:39:46.718>
    creator = 'pablogsal'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 45445
    keywords = ['patch']
    message_count = 4.0
    messages = ['403733', '403854', '404003', '404052']
    nosy_count = 1.0
    nosy_names = ['pablogsal']
    pr_nums = ['28823', '28972']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue45445'
    versions = ['Python 3.11']

    @pablogsal
    Copy link
    Member Author

    We found that using -X ---- can be very confusing and annoying when you make typos, so I think having errors if you pass some unknown stuff will be outweighing any downside of making this more strict.

    @pablogsal
    Copy link
    Member Author

    New changeset db2b6a2 by Pablo Galindo Salgado in branch 'main':
    bpo-45445: Fail if an invalid X-option is provided in the command line (GH-28823)
    db2b6a2

    @pablogsal
    Copy link
    Member Author

    New changeset 79bc5e1 by Pablo Galindo Salgado in branch 'main':
    bpo-45445: Remove incorrectly commited test file (GH-28972)
    79bc5e1

    @FFY00 FFY00 added the 3.11 only security fixes label Oct 15, 2021
    @FFY00 FFY00 closed this as completed Oct 15, 2021
    @FFY00 FFY00 added the type-feature A feature request or enhancement label Oct 15, 2021
    @pablogsal
    Copy link
    Member Author

    I'm opening this again as, this is still not finished as we want to still extend the initialization API to allow a non static string to improve the error message.

    In general, the protocol is to ask if there is something else to be done before closing (and giving it some time before closing if nobody answers) :)

    @pablogsal pablogsal reopened this Oct 15, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @kovidgoyal
    Copy link

    kovidgoyal commented Jun 23, 2022

    Your documentation has stated for YEARS that arbitrary values can be passed via PyConfig.xoptions or -X. 9583cac

    Quoting your own documentation:
    "It also allows passing arbitrary values and retrieving them through the sys._xoptions dictionary."

    There is client software out there relying on this. kovidgoyal/kitty#5223

    Please do not randomnly break backwards compatibility, without even a deprecation period, because something "annoys" you.

    @pablogsal
    Copy link
    Member Author

    pablogsal commented Jun 23, 2022

    @kovidgoyal Thanks for your message.

    Please, be kind. There are much better ways to communicate your concerns than stating that we "randomly break backwards compatibility because something annoys us" or saying that "Apparently in Python-land its acceptable behavior to break backward
    compatibility with documented interfaces on a whim. Bloody joke" as you did in your workaround commit.We all try to do our best here and although I can understand that you are concerned, there is no reason to not be polite.

    This is a bit of a grey area because the _xoptions is a private attribute so we need to discuss what to do here.

    Thanks in advance

    @pablogsal
    Copy link
    Member Author

    CC: @vstinner

    @kovidgoyal
    Copy link

    I was simply quoting you, you said you made this change because "We found that using -X ---- can be very confusing and annoying when you make typo". That is really not a good enough reason to break backwards compat.

    I apologise for my tone, but you guys need to take backwards compat more seriously.

    As for this issue, I dont care personally anymore, I have already worked around it. If you insist on breaking backwards compat then I suggest you add some loud warnings to the changelog. Just think of some poor dude running a script with python -X some_typo in it and that script now failing because python went from 3.10 to 3.11.

    @vstinner
    Copy link
    Member

    vstinner commented Jun 23, 2022

    I apologise for my tone, but you guys need to take backwards compat more seriously.

    We take backward compatibility very seriously. In this case, the incompatible change was done on purpose.

    It was too easy to make typos and Python would not notice them. For example, python3.10 -X fautlhandler does not enable faulthandler and Python 3.10 doesn't say anything about the typo. There are many ways to introduce typos:

    • pycacheprefix or pycache-prefix instead of pycache_prefix
    • UTF8 or utf_8 or utf-8 instead of utf8
    • no_debug_range instead of no_debug_ranges
    • etc.

    For other command line options, Python is strict. For example, python -o fails with a fatal error, whereas python -O works as expected. Same for long options like --check-hash-based-pyc always (instead of --check-hash-based-pycs always).


    I don't see where the -X command line option is documented as it could be used by applications or 3rd party Python modules. It's documented as: Reserved for various implementation-specific options.

    https://docs.python.org/dev/using/cmdline.html#cmdoption-X

    IMO the most portable way to pass arbitrary options to Python is to use environment variables. It works on any Python versions and any operating system.

    By the way, I added sys.orig_argv to Python 3.10: https://docs.python.org/dev/library/sys.html#sys.orig_argv


    If you insist on breaking backwards compat then I suggest you add some loud warnings to the changelog.

    I'm not comfortable with your tone of your messages.

    If you are affected and you consider that the change is not properly advertized in https://docs.python.org/dev/whatsnew/3.11.html I suggest you proposing a pull request to better document the change. Python is maintained by volunteers, you're are not a customer of a paid service.

    @kovidgoyal
    Copy link

    kovidgoyal commented Jun 23, 2022 via email

    @zooba
    Copy link
    Member

    zooba commented Jun 23, 2022

    I agree - this was an unacceptable breaking change to the behaviour of the -X option. It's clearly (and still!) documented as accepting arbitrary values, so we can't change that without a deprecation period.

    image

    @pablogsal
    Copy link
    Member Author

    I am fine reverting the change in 3.11 and adding a deprecation period if we all agree that the API is considered public. I think personally I agree with @zooba and the OP, this change should be reverted (at least in 3.11) and a deprecation warning should be emitted.

    @zooba
    Copy link
    Member

    zooba commented Jun 23, 2022

    What about deprecating un-namespaced options (namespacing to be bikeshedded)? So if it doesn't start with *waves hands* then it has to match one of our options, but otherwise it's passed through.

    Clearly there are people with uses for this, and I've used it myself to inject arguments that weren't suitable for env variables for whatever reason (typically to avoid it being passed on to subprocesses but without having to control subprocess launches). Offering something to migrate to that isn't OS specific seems the least we can do here.

    @ericsnowcurrently
    Copy link
    Member

    If there's a use case for which people have been using -x then we should definitely provide something to meet that need. However, the status quo isn't viable due to the typo problem. A namespace prefix on options could work.

    @pablogsal
    Copy link
    Member Author

    For the time being, I am opening a revert

    @pablogsal
    Copy link
    Member Author

    pablogsal commented Jul 31, 2022

    I have merged the revert as the change was technically backwards incompatible and we are super close to RC1:

    #94745

    We can discuss bringing it back and how for future versions.

    @gpshead
    Copy link
    Member

    gpshead commented Aug 5, 2022

    Thanks for reverting this. I don't think making -X arbitrarything an error is a good idea. It means when we add a -X setting to a version of Python, be it a feature in a minor release or be it a security thing in a patch release, nobody can use it as a flag until they are guaranteed to never encounter specific interpreter versions that support the new flag value. That devalues the concept of -X. We've added -X things to control security fixes in the past and are likely to in the future.

    I understand the typo problem. But people should be testing that their flags are doing what they intended in some form or another.

    At most I suggest just issuing a warning about an unsupported -X flag name when in PYTHONVERBOSE mode.

    @gpshead gpshead added 3.12 bugs and security fixes and removed 3.11 only security fixes labels Aug 5, 2022
    @erlend-aasland erlend-aasland added 3.13 new features, bugs and security fixes and removed 3.12 bugs and security fixes labels Jun 19, 2023
    @iritkatriel iritkatriel added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Nov 28, 2023
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.13 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    9 participants