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.

Sign up for GitHub

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

Jump to bottom

Do not verify destdir argument to compileall #69091

Closed
jgarver mannequin opened this issue Aug 20, 2015 · 10 comments
Closed

Do not verify destdir argument to compileall #69091

jgarver mannequin opened this issue Aug 20, 2015 · 10 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@jgarver
Copy link
Mannequin

jgarver mannequin commented Aug 20, 2015

BPO 24903
Nosy @bitdancer
Files
  • python34_compileall_ddir.diff: Proposed patch to Python 3.4
  • python34_compileall_ddir_2.diff: Updated patch, including test changes
    		•

    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 2015-12-05.04:06:27.708>
created_at = <Date 2015-08-20.17:05:40.912>
labels = ['type-bug', 'library']
title = 'Do not verify destdir argument to compileall'
updated_at = <Date 2015-12-05.04:06:27.707>
user = 'https://bugs.python.org/jgarver'

    bugs.python.org fields:

    activity = <Date 2015-12-05.04:06:27.707>
actor = 'r.david.murray'
assignee = 'none'
closed = True
closed_date = <Date 2015-12-05.04:06:27.708>
closer = 'r.david.murray'
components = ['Library (Lib)']
creation = <Date 2015-08-20.17:05:40.912>
creator = 'jgarver'
dependencies = []
files = ['40216', '40440']
hgrepos = []
issue_num = 24903
keywords = ['patch']
message_count = 10.0
messages = ['248900', '248906', '248913', '248914', '248916', '248950', '248960', '250481', '255918', '255919']
nosy_count = 3.0
nosy_names = ['r.david.murray', 'python-dev', 'jgarver']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue24903'
versions = ['Python 3.4', 'Python 3.5', 'Python 3.6']
    @jgarver
    Copy link
    Mannequin Author

    jgarver mannequin commented Aug 20, 2015

    In compileall.py's main, we verify that the provided destdir (-d) exists at build time. But destdir will commonly be used to override the build time path with a runtime path. That runtime path will usually not exist at build time.

    Note that this logic was changed when compileall.py was migrated to argparse. I think the old logic accidentally avoided the isdir() check at build time.
    11e99b0

    With the attached patch, behavior is made consistent with python 2.7, intended or otherwise.

    @jgarver jgarver mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Aug 20, 2015
    @bitdancer
    Copy link
    Member

    That line isn't checking the existence of destdir, but rather of the (single, because we have only one destdir to use) directory to compile.

    What bug are you actually seeing?

    @jgarver
    Copy link
    Mannequin Author

    jgarver mannequin commented Aug 20, 2015

    Thanks for the response and sorry for the mis-read.

    In 2.7, I did something like:
    python -m compileall -d /runtime/path foo.py

    But in 3.4, I get:
    -d destdir requires exactly one directory argument

    I'm doing this during a build. Then we package the pyc file into a runtime image.

    Let me know if I can further help (or confuse).

    @bitdancer
    Copy link
    Member

    So, given that that does something useful, it appears that the error message (which is what is in 2.7 as well) is wrong to refer to 'directory argument', and we should just eliminate the isdir check (and fix the error message).

    The patch will need a test.

    @bitdancer
    Copy link
    Member

    Actually, it looks like it is a bit more potentially complex than that. The original code would issue the error only if there were more than one argument and the first one was not a directory. So it would in fact compile more than one argument as long as the first one was a dir.

    It's not clear what purpose that check was supposed to serve.

    @jgarver
    Copy link
    Mannequin Author

    jgarver mannequin commented Aug 21, 2015

    I agree. I couldn't find a use for the check, so I removed it entirely in the provided patch. I'm running that way now with success, but of course I'm covering just one use case.

    Digging back a bit further, the isdir() check came in here:
    dc6a4c1

    And even further, near the beginning of time, we find the original check:
    995d45e

    Speculation: In the context of the original code, "directory" in the error message makes more sense and that word should have been dropped when single file support was added. However, given that we loop over the args, I don't see how -d was limited to a single arg.

    With my patch in place, I tested compileall.py with multiple args and the -d. It works as expected and contains the -d path.
    python -m compileall -d /runtime/path foo.py bar.py foobar.py

    I'm happy to craft a new patch. How would you like it?

    @bitdancer
    Copy link
    Member

    OK, so I think the goal was to prevent more than one *directory* from being specified, since as I said earlier that wouldn't make sense given we have only a single destdir path.

    However, for backward compatibility reasons we should probably not restrict it, but rather invoke the python "consenting adults" rule and let you shoot yourself in the foot that way if you want to. It only affects error messages, after all.

    So yeah, if you can build a new patch including some tests that would be great. Best way is to check out the repository, switch to the 3.4 branch, edit the files, and use 'hg diff' to generate the patch. Doing it that way makes it easy to test your changes. But whatever method of coming up with a context diff that you care to use is also fine.

    More detailed information about the normal workflow is in docs.python.org/devguide. Note that in addition to the central repository at hg.python.org, there are also bitbucket and github mirrors, which should both be mentioned in the devguide (if they aren't, that's a bug in the devguide :)

    @jgarver
    Copy link
    Mannequin Author

    jgarver mannequin commented Sep 11, 2015

    Updated patch. Same as before, but updates tests.

    Sorry I went MIA on this issue.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 5, 2015

    New changeset b63fd82a8528 by R David Murray in branch '3.4':
    bpo-24903: Remove misleading error message to fix regression.
    https://hg.python.org/cpython/rev/b63fd82a8528

    New changeset c65e135df1dc by R David Murray in branch '3.5':
    Merge: bpo-24903: Remove misleading error message to fix regression.
    https://hg.python.org/cpython/rev/c65e135df1dc

    New changeset 1e5aacddb67d by R David Murray in branch 'default':
    Merge: bpo-24903: Remove misleading error message to fix regression.
    https://hg.python.org/cpython/rev/1e5aacddb67d

    @bitdancer
    Copy link
    Member

    Thanks, Jake.

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

    No branches or pull requests
    1 participant
    @bitdancer