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

gh-127001: Fix PATHEXT issues in shutil.which() on Windows #127035

Merged
merged 8 commits into from
Nov 22, 2024

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Nov 19, 2024

  • Name without a PATHEXT extension is only searched if the mode does not
    include X_OK.
  • Support multi-component PATHEXT extensions (e.g. ".foo.bar").
  • Support files without extensions in PATHEXT contains dot-only extension
    (".", "..", etc).
  • Support PATHEXT extensions that end with a dot (e.g. ".foo.").

…h() on Windows

Name without a PATHEXT extension is only searched if the mode does not
include X_OK.
@zooba
Copy link
Member

zooba commented Nov 19, 2024

Ah, nice. I approve of this logic. I have a vague feeling it was discussed at some point, but I think it makes sense to require an extension from PATHEXT if X is required.

@serhiy-storchaka
Copy link
Member Author

I'll rewrite tests tomorrow.

@serhiy-storchaka serhiy-storchaka marked this pull request as ready for review November 22, 2024 08:34
@serhiy-storchaka
Copy link
Member Author

Some of existing tests did not make sense because they are grounded on wrong model. I replaced them. Also I refactored other tests to make them more uniform and reuse the same files and directories.

Fixed also cases when PATHEXT contains a multicomponent extension (e.g. .foo.bar) or dots (., .., etc).

Copy link
Member

@zooba zooba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment I'd like to see added, but otherwise looks good

Lib/shutil.py Outdated
@@ -1550,21 +1550,21 @@ def which(cmd, mode=os.F_OK | os.X_OK, path=None):
if sys.platform == "win32":
# PATHEXT is necessary to check on Windows.
pathext_source = os.getenv("PATHEXT") or _WIN_DEFAULT_PATHEXT
pathext = [ext for ext in pathext_source.split(os.pathsep) if ext]
pathext = pathext_source.split(os.pathsep)
pathext = [ext.rstrip('.') and ext for ext in pathext if ext]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really dislike using and for value selection. I trust that you've got it right here, but I can't read it easily. Can we maybe comment?

# Replace ext of '.' with empty string, but keep trailing dots on '.ext.'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think that it is better to remove dots in all cases. The behavior of cmd.exe if PATHEXT contains .BAT. and there is a.bat in the path:

  • a -- hangs
  • a.bat -- found
  • a. -- not found
  • a.bat. -- not found

which() will now return the same result, except for the first case. I think this is the best option for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me

@serhiy-storchaka serhiy-storchaka merged commit 8899e85 into python:main Nov 22, 2024
36 checks passed
@miss-islington-app
Copy link

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

@serhiy-storchaka serhiy-storchaka deleted the shutil-which branch November 22, 2024 15:52
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 22, 2024
…honGH-127035)

* Name without a PATHEXT extension is only searched if the mode does not
  include X_OK.
* Support multi-component PATHEXT extensions (e.g. ".foo.bar").
* Support files without extensions in PATHEXT contains dot-only extension
  (".", "..", etc).
* Support PATHEXT extensions that end with a dot (e.g. ".foo.").
(cherry picked from commit 8899e85)

Co-authored-by: Serhiy Storchaka <[email protected]>
@miss-islington-app
Copy link

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8899e85de100557899da05f0b37867a371a73800 3.12

@bedevere-app
Copy link

bedevere-app bot commented Nov 22, 2024

GH-127156 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Nov 22, 2024
@serhiy-storchaka serhiy-storchaka changed the title gh-127001: Fix conflict with extensionless files in shutil.which() on Windows gh-127001: Fix PATHEXT issues in shutil.which() on Windows Nov 22, 2024
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Nov 22, 2024
…ws (pythonGH-127035)

* Name without a PATHEXT extension is only searched if the mode does not
  include X_OK.
* Support multi-component PATHEXT extensions (e.g. ".foo.bar").
* Support files without extensions in PATHEXT contains dot-only extension
  (".", "..", etc).
* Support PATHEXT extensions that end with a dot (e.g. ".foo.").
(cherry picked from commit 8899e85)

Co-authored-by: Serhiy Storchaka <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Nov 22, 2024

GH-127158 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Nov 22, 2024
serhiy-storchaka added a commit that referenced this pull request Nov 22, 2024
…-127035) (GH-127156)

* Name without a PATHEXT extension is only searched if the mode does not
  include X_OK.
* Support multi-component PATHEXT extensions (e.g. ".foo.bar").
* Support files without extensions in PATHEXT contains dot-only extension
  (".", "..", etc).
* Support PATHEXT extensions that end with a dot (e.g. ".foo.").
(cherry picked from commit 8899e85)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit that referenced this pull request Nov 22, 2024
…-127035) (GH-127158)

* Name without a PATHEXT extension is only searched if the mode does not
  include X_OK.
* Support multi-component PATHEXT extensions (e.g. ".foo.bar").
* Support files without extensions in PATHEXT contains dot-only extension
  (".", "..", etc).
* Support PATHEXT extensions that end with a dot (e.g. ".foo.").
(cherry picked from commit 8899e85)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants