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

ENH: use shutil.which() instead of external which(1) #54937

Merged
merged 3 commits into from
Sep 8, 2023

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Sep 1, 2023

Use the shutil.which() function to determine whether executables exist rather than calling the external which(1) or where program. This program is not a part of POSIX base system, and modern Linux distributions are working towards making it completely optional. The builtin shutil.which() is available since Python 3.3 and already used in the same file.

This makes it possible to remove the dependency on sys-apps/which from Gentoo systems.

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

@mgorny
Copy link
Contributor Author

mgorny commented Sep 1, 2023

I wasn't sure how to classify this: I chose "ENH" because I suppose removing unnecessary dependency is an "enhancement". However, this is quite trivial and I'd love to see it in 2.1.1.

@twoertwein
Copy link
Member

Ideally, your PR would make it into https://github.com/asweigart/pyperclip and then a PR here to update the pyperclip copy - but that repository seems rather inactive.

@mgorny
Copy link
Contributor Author

mgorny commented Sep 2, 2023

Well, there's a bit of a problem with that since technically pyperclip still supports Python 2.7, and I don't think there's really point in going over the burden of implementing backwards compatibility given that the author is unresponsive for 2 years already.

@twoertwein
Copy link
Member

An option could be to first update the copy to the "latest" version. Assuming it is the last version ever to be released, clean it (remove py2 stuff), and then use shutil.which. Can then also add type annotations later.

@mroeschke

@mroeschke
Copy link
Member

Yeah agreed since pyperclip seems unmaintained, it would be best to update our vendor to the latest version before making any changes

@mgorny
Copy link
Contributor Author

mgorny commented Sep 6, 2023

Just to be clear, by "latest" do you mean the HEAD of git repository, or the release from PyPI? I suppose I could fork the repository and make a PR with a note that it breaks py2.7 compat, if that helps. If the project is ever revived, it'll probably drop py2 support anyway.

Long term it might be a good idea to look for a maintained replacement too.

@twoertwein
Copy link
Member

I would take the latest released version from pypi as it might be more well-tested.

As the project seems inactive/dead, I personally wouldn't bother opening a PR there and instead, I would just copy the latest __init__.py from pypi, remove the compact with python2 , and then replace it with the current copy in pandas + run black/isort/ruff (this could be one or multiple commits) and then one/more commits with your changes on top of that.

Copy the changes from upstream 1.8.2 to the bundled copy of pyperclip.
The code was reformatted using black and verified using ruff.
The existing modifications from pandas were preserved.
Remove the fallback to which/where that is only necessary for Python 2
that does not feature shutil.which().  Also collapse the imports
to avoid importing shutil.which() twice.  It is now only imported
as `_executable_exists()` to minimize the changes to the original code.
@mgorny
Copy link
Contributor Author

mgorny commented Sep 8, 2023

I've copied the changes from 1.8.2 release. Incidentally, they included the use of shutil.which() with Python 2 compatibility — so I've just removed the compatibility fallback, and reformatted everything.

@twoertwein
Copy link
Member

@mgorny Do you think this fixes #51744? I'm not sure whether we can test that on the CI.

@mgorny
Copy link
Contributor Author

mgorny commented Sep 8, 2023

@mgorny Do you think this fixes #51744? I'm not sure whether we can test that on the CI.

Looking at #51744 (comment), I think not. We should probably move the WAYLAND_DISPLAY logic outside HAS_DISPLAY. I can do that if you wish but I don't have a wayland setup to test it either.

@twoertwein
Copy link
Member

Looking at #51744 (comment), I think not. We should probably move the WAYLAND_DISPLAY logic outside HAS_DISPLAY. I can do that if you wish but I don't have a wayland setup to test it either.

Probably better to keep it as-is. There is also an issue to even deprecate it #52129

@mgorny
Copy link
Contributor Author

mgorny commented Sep 8, 2023

Ok, thanks.

By the way, it just occurred to me to check pyperclip PR tracker, and indeed there's a fix proposed along the lines of what I thought: asweigart/pyperclip#201. So if you prefer, I can just include that.

@twoertwein
Copy link
Member

Ok, thanks.

By the way, it just occurred to me to check pyperclip PR tracker, and indeed there's a fix proposed along the lines of what I thought: asweigart/pyperclip#201. So if you prefer, I can just include that.

Personally, I have no preference (I honestly wouldn't mind deprecating clipboard). Since this is still mostly a 1:1 copy, it might be better to make as few changes as possible. @mroeschke

@mroeschke mroeschke added the IO Data IO issues that don't fit into a more specific label label Sep 8, 2023
@mroeschke mroeschke added this to the 2.2 milestone Sep 8, 2023
@mroeschke mroeschke merged commit 711fea0 into pandas-dev:main Sep 8, 2023
37 checks passed
@mroeschke
Copy link
Member

Thanks @mgorny. Yeah it's best to have gradual changes in these vendored files.

FWIW, I think there is a desire to change the clipboard backend #39834. At minimum, one day unvendoring pyperclip would be ideal (I'm not sure why it was vendored in the first place)

mroeschke pushed a commit to mroeschke/pandas that referenced this pull request Sep 11, 2023
* ENH: update bundled pyperclip with changes from 1.8.2 release

Copy the changes from upstream 1.8.2 to the bundled copy of pyperclip.
The code was reformatted using black and verified using ruff.
The existing modifications from pandas were preserved.

* ENH: Remove Python 2 compatibility from imported pyperclip code

Remove the fallback to which/where that is only necessary for Python 2
that does not feature shutil.which().  Also collapse the imports
to avoid importing shutil.which() twice.  It is now only imported
as `_executable_exists()` to minimize the changes to the original code.

* BUG: Fix pylint failure (redundant `pass`) in clipboard
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO Data IO issues that don't fit into a more specific label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants