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

Expose os.posix_spawnp() #79855

Closed
vstinner opened this issue Jan 7, 2019 · 11 comments
Closed

Expose os.posix_spawnp() #79855

vstinner opened this issue Jan 7, 2019 · 11 comments
Labels
3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@vstinner
Copy link
Member

vstinner commented Jan 7, 2019

BPO 35674
Nosy @vstinner, @serhiy-storchaka, @izbyshev, @pablogsal, @nanjekyejoannah
PRs
  • bpo-35674: Expose posix_spawnp #11545
  • bpo-35674: Expose posix_spawnp #11545
  • bpo-35674: Expose posix_spawnp #11545
  • bpo-35674: Expose posix_spawnp #11545
  • bpo-35674: Expose posix_spawnp #11554
  • bpo-35674: Expose posix_spawnp #11554
  • bpo-35674: Expose posix_spawnp #11554
  • 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 2019-01-16.14:33:10.145>
    created_at = <Date 2019-01-07.00:46:08.230>
    labels = ['interpreter-core', '3.8']
    title = 'Expose os.posix_spawnp()'
    updated_at = <Date 2019-01-16.14:34:00.804>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2019-01-16.14:34:00.804>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-01-16.14:33:10.145>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2019-01-07.00:46:08.230>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 35674
    keywords = ['patch', 'patch', 'patch', 'patch']
    message_count = 11.0
    messages = ['333128', '333130', '333142', '333212', '333225', '333346', '333347', '333375', '333767', '333772', '333773']
    nosy_count = 5.0
    nosy_names = ['vstinner', 'serhiy.storchaka', 'izbyshev', 'pablogsal', 'nanjekyejoannah']
    pr_nums = ['11545', '11545', '11545', '11545', '11554', '11554', '11554']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue35674'
    versions = ['Python 3.8']

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 7, 2019

    bpo-20104 exposed os.posix_spawn(), but not os.posix_spawnp().

    os.posix_spawnp() would be useful to support executable with no directory. See bpo-35537 "use os.posix_spawn in subprocess".

    I'm not sure what is the best API:

    • Add os.posix_spawnp()? duplicate the documentation and some parts of the C code (but share most of the C code)
    • Add a new optional parameter to os.posix_spawn()? Ideas of names: 'use_path' or 'search_executable'. Internally, the glibc uses SPAWN_XFLAGS_USE_PATH flag to distinguish posix_spawn() and posix_spawnp().

    execvp() uses the PATH environment variable, or use confstr(_CS_PATH) if PATH is not set. I guess that posix_spawnp() also uses confstr(_CS_PATH) if PATH is not set.

    Currently, my favorite option is to add a new optional 'use_path' parameter to the existing os.posix_spawn() function.

    @vstinner vstinner added 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jan 7, 2019
    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 7, 2019

    Historically, Python used to mimick the libc: there are os.stat() and os.lstat() for example. But later, os.stat(*, follow_symlinks=False) keyword-only parameter has been added: if true, lstat() is used internally.

    @serhiy-storchaka
    Copy link
    Member

    To add some context, the follow_symlinks parameter was added to os.stat() after adding support of *at() functions (like openat() and fstatat()). Since both C functions stat() and lstat() are superseded by fstatat(), the latter was exposed at Python level as adding new keyword-only parameters to os.stat(). This allowed to avoid significant increasing the number of functions in the os module.

    Since there is no function that supersedes both posix_spawn() and posix_spawnp(), this may be an argument for exposing them as separate functions at Python level. Although this argument is not very strong.

    @nanjekyejoannah
    Copy link
    Member

    I would go with exposing posix_spawnp() as a separate function to rule out any arguments.

    I am working on this in this regard unless anyone has a different view.

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2019

    I'm ok to expose posix_spawnp() as os.posix_spawnp().

    Even if we expose posix_spawnp() as os.posix_spawnp(), we can still reconsider to add posix_spawnp() feature into os.posix_spawn() as an optional keyword parameter later :-) Honestly, I have no strong preference for the API. My main problem with the keyword option is the risk of name conflict if a new feature is added to posix_spawn() with a name similar to my proposed name "use_path".

    @pablogsal
    Copy link
    Member

    I also think to expose posix_spawnp() as os.posix_spawnp() seems the more consistent thing to do as the os/posix module tries to mirror glibc as much as possible, which helps with discoverability and cross-reference with man pages.

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 9, 2019

    Pablo:

    I also think to expose posix_spawnp() as os.posix_spawnp() seems the more consistent thing to do as the os/posix module tries to mirror glibc as much as possible, which helps with discoverability and cross-reference with man pages.

    Ok. Let's do that.

    @joannah Nanjekye: Don't hesitate to come to me if you need my help to implement that ;-)

    @nanjekyejoannah
    Copy link
    Member

    Now that there is consesus, I am working on a PR for this.

    @vstinner
    Copy link
    Member Author

    New changeset 92b8322 by Victor Stinner (Joannah Nanjekye) in branch 'master':
    bpo-35674: Add os.posix_spawnp() (GH-11554)
    92b8322

    @vstinner
    Copy link
    Member Author

    Well done Joannah Nanjekye :-) Thanks to Joannah for this nice enhancemenet and to all reviewers.

    @vstinner
    Copy link
    Member Author

    FYI I modified subprocess to use os.posix_spawnp() when it's available:

    New changeset 0785889 by Victor Stinner in branch 'master':
    bpo-35537: subprocess can now use os.posix_spawnp (GH-11579)
    0785889

    @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 interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants