Skip to content

bpo-34530: Fix distutils find_executable()#8968

Closed
vstinner wants to merge 1 commit into
python:masterfrom
vstinner:distutils_find_executable
Closed

bpo-34530: Fix distutils find_executable()#8968
vstinner wants to merge 1 commit into
python:masterfrom
vstinner:distutils_find_executable

Conversation

@vstinner

@vstinner vstinner commented Aug 28, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

distutils.spawn.find_executable() has been reimplemented using
shutil.which() to fix two issues: fallback on os.defpath if the PATH
environment variable is not set, and respect the PATHEXT environment
variable on Windows.

https://bugs.python.org/issue34530

@vstinner vstinner requested a review from zooba August 28, 2018 08:19
@vstinner

vstinner commented Aug 28, 2018

Copy link
Copy Markdown
Member Author

Without the change:

$ env -i ./python -c 'import distutils.spawn; print(distutils.spawn.find_executable("true"))'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/vstinner/prog/python/master/Lib/distutils/spawn.py", line 176, in find_executable
    path = os.environ['PATH']
  File "/home/vstinner/prog/python/master/Lib/os.py", line 672, in __getitem__
    raise KeyError(key) from None
KeyError: 'PATH'

With the change:

$ env -i ./python -c 'import distutils.spawn; print(distutils.spawn.find_executable("true"))'
/bin/true

@serhiy-storchaka

serhiy-storchaka commented Aug 28, 2018

Copy link
Copy Markdown
Member

I thought distutils is frozen.

If this change acceptable, it needs an open issue, news entry and tests.

@vstinner
bpo-34530: Fix distutils find_executable()
ae3d790 
distutils.spawn.find_executable() has been reimplemented using
shutil.which() to fix two issues: fallback on os.defpath if the PATH
environment variable is not set, and respect the PATHEXT environment
variable on Windows.
@vstinner

vstinner commented Aug 28, 2018

Copy link
Copy Markdown
Member Author

I thought distutils is frozen.

I'm not aware of that. Are you refering to the dead distutils2 project? distutils now accepts changes.

If this change acceptable, it needs an open issue, news entry and tests.

done

@vstinner vstinner changed the title distutils: reuse shutil.which() bpo-34530: Fix distutils find_executable() Aug 28, 2018
@merwok

merwok commented Aug 28, 2018

Copy link
Copy Markdown
Member

It’s not 100% frozen anymore, but still very stable. New things like wheel support happens outside of distutils and the stdlib.

For example, it would break a lot of code to use shutil.which in distutils and remove find_executable. This PR however fixes two issues but keeps the interface intact, so it is acceptable.

@vstinner

vstinner commented Aug 30, 2018

Copy link
Copy Markdown
Member Author

@serhiy-storchaka: I added a NEWS entry and a basic test. I don't want to write a long test suite, since shutil.which() is very well tested in test_shutils, and find_executable() is now really just calling which(). Would you mind to review my change?

serhiy-storchaka
serhiy-storchaka approved these changes Aug 30, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks. LGTM now. Added just few minor comments.

Comment thread Lib/distutils/tests/test_spawn.py
pass
os.chmod(filename, stat.S_IXUSR)

rv = find_executable(program, path=tmp_dir)

@serhiy-storchaka serhiy-storchaka Aug 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add also a negative test. find_executable(program) should return None.

Comment thread Misc/NEWS.d/next/Library/2018-08-28-14-48-44.bpo-34530.ZZjx45.rst
@@ -0,0 +1,4 @@
distutils.spawn.find_executable() has been reimplemented using

@serhiy-storchaka serhiy-storchaka Aug 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add links? :func:`distutils.spawn.find_executable`

@vstinner

vstinner commented Sep 3, 2018

Copy link
Copy Markdown
Member Author

I missed the fact that find_executable() always lookup in the current directory, whereas shutil.which() doesn't. I abandon this change in favor of PR #9049.

@vstinner vstinner closed this Sep 3, 2018
@vstinner vstinner deleted the distutils_find_executable branch September 3, 2018 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@zooba zooba Awaiting requested review from zooba

@doko42 doko42 Awaiting requested review from doko42

@merwok merwok Awaiting requested review from merwok

@dstufft dstufft Awaiting requested review from dstufft

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vstinner @serhiy-storchaka @merwok @the-knights-who-say-ni @bedevere-bot