[proxy] web.archive.org← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
/ cpython Public
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

bpo-34530: Fix distutils find_executable() #8968

Closed
wants to merge 1 commit into from

Conversation

Copy link
Member

vstinner commented Aug 28, 2018

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

Copy link
Member Author

vstinner commented Aug 28, 2018

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

Copy link
Member

serhiy-storchaka commented Aug 28, 2018

I thought distutils is frozen.

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

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 force-pushed the distutils_find_executable branch from 1ff1514 to ae3d790 Compare Aug 28, 2018
Copy link
Member Author

vstinner commented Aug 28, 2018

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 changed the title distutils: reuse shutil.which() bpo-34530: Fix distutils find_executable() Aug 28, 2018
Copy link
Member

merwok commented Aug 28, 2018

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.

Copy link
Member Author

vstinner commented Aug 30, 2018

@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?

Copy link
Member

serhiy-storchaka left a comment

Thanks. LGTM now. Added just few minor comments.

pass
os.chmod(filename, stat.S_IXUSR)

rv = find_executable(program, path=tmp_dir)
Copy link
Member

serhiy-storchaka Aug 30, 2018

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.

@@ -0,0 +1,4 @@
distutils.spawn.find_executable() has been reimplemented using
Copy link
Member

serhiy-storchaka Aug 30, 2018

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`

Copy link
Member Author

vstinner commented Sep 3, 2018

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 closed this Sep 3, 2018
vstinner deleted the distutils_find_executable branch Sep 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants