bpo-28441: Ensure .exe suffix in sys.executable on MinGW and Cygwin#4348
Merged
methane merged 3 commits intopython:masterfrom Oct 5, 2018
Merged
bpo-28441: Ensure .exe suffix in sys.executable on MinGW and Cygwin#4348methane merged 3 commits intopython:masterfrom
.exe suffix in sys.executable on MinGW and Cygwin#4348methane merged 3 commits intopython:masterfrom
Conversation
ma8ma
reviewed
Nov 10, 2017
Doc/library/sys.rst
Outdated
Contributor
There was a problem hiding this comment.
I'd suggest adding a note which the suffix won't be appended when the path will become an invalid link (e.g. symlink can be made without the executable suffix even in cygwin).
Contributor
Author
There was a problem hiding this comment.
Right--I wasn't sure whether or not to include that point. To me it seems "obvious" that that should be the case, but maybe that's worth clarifying.
As for adding a news entry I agree this change probably merits one, though it still only impacts a small number of platforms so I'll let the core team make that determination...
899120d to
343cfe0
Compare
Contributor
Author
|
Finally got around to updating this. Hopefully we can have it merged soon. |
77a57f0 to
d8b868b
Compare
Contributor
Author
|
I rebased this--it would have been good to have for 3.7. I'm not sure if this can be done for a bugfix release. I think it's low-enough impact on most platforms that it could be... |
Member
|
I feel this solution is ugly... |
Member
Contributor
Author
|
This is a generic, cross-platform solution that makes perfect sense, and some version or other of this patch has already been in use for years. I and others have just been maintaining it from Python version to Python version... |
Member
"EXE_SUFFIX is trimmed from sys.exectable" is non-generic, platform-specific issue, isn't? |
Contributor
Author
|
It's an issue on platforms that have The issue here is that the |
methane
reviewed
Jul 26, 2018
| @@ -0,0 +1,3 @@ | |||
| On platforms that have an executable suffix (such as ``.exe`` on | |||
| Windows/Cygwin), ensure that ``sys.executable`` always includes the | |||
Member
There was a problem hiding this comment.
On Windows, PC/getpathp.c is used instead of Module/getpath.c.
So Windows is not affected by this change.
Cygwin and all other platforms are affected when configure --with-suffix=.<SUFFIX> option is used.
Contributor
Author
There was a problem hiding this comment.
True, we can reword that then.
Contributor
Author
There was a problem hiding this comment.
Agreed. Would you accept a different wording?
Doc/library/sys.rst
Outdated
|
|
||
| .. versionchanged:: 3.7.1 | ||
| Included executable suffix to path, if one is defined for the | ||
| platform, unless the executable path is a symbolic link. |
Member
There was a problem hiding this comment.
EXE_SUFFIX is specified by configure option. So "defined for the platform" is bit unclear.
Especially, suffix is added for Windows build already before this change.
Contributor
Author
There was a problem hiding this comment.
Perhaps this note can be excluded entirely. I see this more as a bug fix anyways: If the .exe is part of the executable's filename it should be included in sys.executable is the file's real name.
Contributor
Author
Not if the filename with the suffix doesn't exist. There is one remote corner case where this could be incorrect, where if both What's not an unusual corner case is building/developing Python. Not having this patch leads to a lot of confusion on case-insensitive filesystems since there's both |
Member
Since Or should we add comment to ``--with-suffix` option like "This option should be used only platform |
Contributor
Author
|
Perhaps, but for example, I think it's also used when developing Python on OSX. |
Member
|
On mac, BUILDEXEEXT=.exe ( Of course, macOS doesn't omit |
Contributor
Author
|
I see, I misread that. Indeed it's not a problem for OSX since running the executable won't omit the In that case, I'm not even sure what |
Member
|
I'm not sure about what But we can abuse So using |
Contributor
Author
|
It was last added in https://bugs.python.org/issue230197 apparently for OSX in fact. |
Member
|
Do you know platforms require this patch other than Cygwin and MSYS? If this is only MSYS and Cygwin issue, how about just |
Contributor
Author
|
Perhaps it shouldn't be "abused" then, because that's not what |
Contributor
Author
|
An earlier version of the patch did use an I would suggest instead not worrying about hypothetical misuses of |
Member
Hm, BUILDEXT is added later than that. |
Member
I don't know many platforms Python runs on. But does this patch make sense even when |
Contributor
Author
|
Ok, I see. I think, while the examples you gave like There are a few possible options then:
|
Contributor
Author
|
If it would go down easier I'd happily add back in an |
Contributor
Author
|
@methane Thanks for your review--I've updated the PR description with some possible action items based on your review so far, though a few of them require clarification. I have my own opinions about this but I'm fine with whatever. |
Member
|
Sorry for late reply. I can't approve this because I don't know all platforms on the earth Python runs on. I prefer whitelisting platforms, to avoid making any unexpected effect happens on other unknown platforms. Otherwise, some other core-dev who are expert of portability and minor platforms should review this. |
Contributor
|
@embray: Status check is done, and it's a failure ❌ . |
2 similar comments
Contributor
|
@embray: Status check is done, and it's a failure ❌ . |
Contributor
|
@embray: Status check is done, and it's a failure ❌ . |
Contributor
Author
|
@methane Thank you for taking the time on this one! |
Contributor
Author
|
Is there some way I can request this change to be backported to the 3.7 branch? Until it is, no version of Python 3.7 can build on Cygwin without this patch. It can wait until 3.8, but I should note that #8712, which I also submitted primarily for Cygwin, has been backported. |
Member
|
#8712 is clearly a bug, and it's regression. It is bit different from this. This changes @ned-deily This PR is affects only MinGW and Cygwin. All code is guareded by |
Member
|
@methane, I agree with your analysis. One could argue that, since this is a user-visible change in behavior in a maintenance release, it's not appropriate. On the other hand, one could argue that the change prevents failures and the risk is mitigated by being limited to non-mainstream platforms (from a cpython perspective). Overall I'm +0 on this change so I won't oppose it. However, if you merge it, you should add a |
Contributor
Author
|
I also agree with the analysis regarding being a user-visible change. That said, as @methane wrote it's still a platform-specific user-visible change, and Python does not even build on Cygwin without this patch (the downstream python package for Cygwin has almost always included a version of this patch anyways). Should I make a PR or can it be done automatically be applying the appropriate label? Thanks to you both. |
Contributor
|
GH-12063 is a backport of this pull request to the 3.7 branch. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Feb 27, 2019
…in (pythonGH-4348) This is needed to even the run the test suite on buildbots for affected platforms; e.g.: ``` ./python.exe ./Tools/scripts/run_tests.py -j 1 -u all -W --slowest --fail-env-changed --timeout=11700 -j2 /home/embray/src/python/test-worker/3.x.test-worker/build/python -u -W default -bb -E -W error::BytesWarning -m test -r -w -j 1 -u all -W --slowest --fail-env-changed --timeout=11700 -j2 Traceback (most recent call last): File "./Tools/scripts/run_tests.py", line 56, in <module> main(sys.argv[1:]) File "./Tools/scripts/run_tests.py", line 52, in main os.execv(sys.executable, args) PermissionError: [Errno 13] Permission denied make: *** [Makefile:1073: buildbottest] Error 1 ``` (cherry picked from commit 7a7693e) Co-authored-by: E. M. Bray <erik.m.bray@gmail.com>
methane
pushed a commit
that referenced
this pull request
Feb 27, 2019
…in (GH-4348) This is needed to even the run the test suite on buildbots for affected platforms; e.g.: ``` ./python.exe ./Tools/scripts/run_tests.py -j 1 -u all -W --slowest --fail-env-changed --timeout=11700 -j2 /home/embray/src/python/test-worker/3.x.test-worker/build/python -u -W default -bb -E -W error::BytesWarning -m test -r -w -j 1 -u all -W --slowest --fail-env-changed --timeout=11700 -j2 Traceback (most recent call last): File "./Tools/scripts/run_tests.py", line 56, in <module> main(sys.argv[1:]) File "./Tools/scripts/run_tests.py", line 52, in main os.execv(sys.executable, args) PermissionError: [Errno 13] Permission denied make: *** [Makefile:1073: buildbottest] Error 1 ``` (cherry picked from commit 7a7693e) Co-authored-by: E. M. Bray <erik.m.bray@gmail.com>
|
Member
|
CIs are failing due to https://bugs.python.org/issue36131 |
|
|
|
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on original patch by @ma8ma
Note: This is needed to even the run the test suite on buildbots for affected platforms; e.g.:
https://bugs.python.org/issue28441
Possibly to do based on review of @methane :
./configure --with-suffix=; possibly disable using it to override EXEEXT#if defined(__CYGWIN__) || defined(__MINGW32__)around the add_exe_suffix stuffsys.platform