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

"python -S" should be robust against e.g. "from site import addsitedir" #55800

Closed
carljm opened this issue Mar 17, 2011 · 16 comments
Closed

"python -S" should be robust against e.g. "from site import addsitedir" #55800

carljm opened this issue Mar 17, 2011 · 16 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@carljm
Copy link
Contributor

carljm commented Mar 17, 2011

BPO 11591
Nosy @brettcannon, @carljm, @merwok
Files
  • 87df1d37c88e.diff
  • ebe5760afa08.diff
  • test_site-11591.diff
  • 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 = 'https://github.com/merwok'
    closed_at = <Date 2011-03-23.04:09:31.883>
    created_at = <Date 2011-03-17.19:47:00.007>
    labels = ['type-bug', 'library']
    title = '"python -S" should be robust against e.g. "from site import addsitedir"'
    updated_at = <Date 2011-06-09.15:26:13.570>
    user = 'https://github.com/carljm'

    bugs.python.org fields:

    activity = <Date 2011-06-09.15:26:13.570>
    actor = 'eric.araujo'
    assignee = 'eric.araujo'
    closed = True
    closed_date = <Date 2011-03-23.04:09:31.883>
    closer = 'eric.araujo'
    components = ['Library (Lib)']
    creation = <Date 2011-03-17.19:47:00.007>
    creator = 'carljm'
    dependencies = []
    files = ['21274', '21327', '22293']
    hgrepos = ['4', '5']
    issue_num = 11591
    keywords = ['patch']
    message_count = 16.0
    messages = ['131281', '131286', '131294', '131322', '131536', '131580', '131681', '131706', '131709', '131748', '131761', '131848', '131850', '134568', '136968', '137988']
    nosy_count = 4.0
    nosy_names = ['brett.cannon', 'carljm', 'eric.araujo', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue11591'
    versions = ['Python 3.3']

    @carljm
    Copy link
    Contributor Author

    carljm commented Mar 17, 2011

    If python is run with the -S flag, that declares the intent of the user to not have site-specific additions to sys.path.

    However, some code in that process may have a legitimate need for a function defined in site.py - for instance, addsitedir. But the act of importing site.py, as a side effect, adds the standard site-specific directories to sys.path.

    python -S would be more useful and reliable if it prevented importing site from automatically making the sys.path additions. There is no loss of flexibility here, as user code could still explicitly call site.main() to achieve all of the current side-effects of "import site".

    The fix is a one-liner, and is in the linked hg repository.

    @carljm carljm added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Mar 17, 2011
    @merwok
    Copy link
    Member

    merwok commented Mar 17, 2011

    Thanks. Would you mind adding tests in test_site?

    @carljm
    Copy link
    Contributor Author

    carljm commented Mar 17, 2011

    Adding a test is easier said than done. The behavior change here depends on python being run with -S. Currently test_site skips itself if the test suite is run with -S, and if I remove that skip it crashes under -S.

    Options as I see it:

    1. Declare this one-liner correct by inspection. It doesn't break any existing tests.

    2. Add a new test file (test_no_site.py?) that only runs with -S and tests that importing something from site doesn't trigger sys.path additions. This seems like the most reasonable test, but I'm not sure how useful it is, since I doubt most people ever try running the test suite with -S.

    3. Make the fix more complicated such that it uses an intermediary variable which can be mocked (unlike sys.flags.no_site, which is read-only), and then add a test which mocks this variable, temporarily removes "site" from sys.modules, tries importing it again, and checks whether main() is called. This creates a complex test which is highly coupled to the implementation in site.py, but would be run under normal conditions (without -S).

    Which option do you prefer?

    @merwok
    Copy link
    Member

    merwok commented Mar 18, 2011

    Fair argument. Brett is the author of recent changes in site, let him decide.

    Brett: Would you agree to 1)?

    @brettcannon
    Copy link
    Member

    This is what I get for trying to clean up site.py years ago. =)

    I'm fine with the change as long as there is a very clear Misc/NEWS message that the semantics on import have changed (and obviously this is not backported).

    @merwok
    Copy link
    Member

    merwok commented Mar 21, 2011

    I think this requires a note in NEWS but also in Doc/library/site.rst. Carl, would you like to make a new changeset with that edition in your repo? (It will also provide a test for the Create Patch button after.)

    @carljm
    Copy link
    Contributor Author

    carljm commented Mar 21, 2011

    Added documentation to Doc/library/site.rst and Misc/NEWS.

    @merwok
    Copy link
    Member

    merwok commented Mar 21, 2011

    Looks great, thank you. I think I’ll also add a docstring to main before committing, now that the function is publicly documented.

    Did you have to manually click “Create Patch” to make roundup generate it? Did you try first to click on the button of the existing repo before adding a new repo entry? (Still learning how to use it, thanks for experimenting along :)

    Brett: Thanks for the review. If you don’t comment negatively on the doc change, I will commit this.

    (Side concern: the module does not define __all__, even though only 4 functions and 4 constants are officially documented. I’d like to define __all__, but the recentish huge thread on public/private APIs scared me.)

    @carljm
    Copy link
    Contributor Author

    carljm commented Mar 22, 2011

    Did you have to manually click “Create Patch” to make roundup generate it?

    Yes - the first time too.

    Did you try first to click on the button of the existing repo before adding a new repo entry?

    That would probably have worked fine. The "Remote hg repo" field was just empty when I made my latest comment, so I filled it in again. Wasn't sure if it would duplicate, or be smart enough to tell they were the same repo, or what. I guess it duplicated :/

    @brettcannon brettcannon assigned brettcannon and unassigned merwok Mar 22, 2011
    @merwok
    Copy link
    Member

    merwok commented Mar 22, 2011

    The "Remote hg repo" field was just empty when I made my latest comment

    Looks like this field is always empty: its goal is to add a repo, just like the File field is always empty unless you add a file. The existing files and repositories are however listed underneath the form, so your screen just probably did not go low enough for you to see that the repo was still listed.

    @brettcannon
    Copy link
    Member

    Doc changes seem fine to me.

    @brettcannon brettcannon assigned merwok and unassigned brettcannon Mar 22, 2011
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 23, 2011

    New changeset a364719e400a by Éric Araujo in branch 'default':
    Do not touch sys.path when site is imported and python was started with -S.
    http://hg.python.org/cpython/rev/a364719e400a

    @merwok
    Copy link
    Member

    merwok commented Mar 23, 2011

    Committed with small wording changes and more docs. Thank you, and good luck for cpythonv!

    @merwok merwok closed this as completed Mar 23, 2011
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 27, 2011

    New changeset 9a1ca0062950 by Éric Araujo in branch 'default':
    Add versionchanged for a364719e400a (bpo-11591)
    http://hg.python.org/cpython/rev/9a1ca0062950

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 26, 2011

    New changeset 6ee5443773cb by Éric Araujo in branch 'default':
    Also add versionchanged directive to the function doc (bpo-11591)
    http://hg.python.org/cpython/rev/6ee5443773cb

    @merwok
    Copy link
    Member

    merwok commented Jun 9, 2011

    Now that site can be imported without side effects under -S, I think the tests could be updated: they don’t have to be all skipped under -S. See attached patch.

    @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
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants