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

use POSIX compliant command -v instead of which #3215

Merged
merged 2 commits into from
Jul 22, 2021

Conversation

miojonka
Copy link
Contributor

Xpra currently depends on the tool which to be installed to find its own binary on a server. The resulting error message is not helpful as it claims the Xpra binary was not found (i.e. Xpra is not installed when it clearly is), but instead it is the binary-to-find-binaries that was not found.

On Archlinux for example which is not installed by default and which in general has different output on different distributions.

Using command -v is POSIX compliant (which is not) and will work on every distribution without requiring to have which installed. It is also the endorsed way to find the path to a binary in $PATH.

This PR should fix this (I only tested it on Archlinux).

xpra/server/ssh.py Outdated Show resolved Hide resolved
@totaam totaam merged commit ecc4ab3 into Xpra-org:master Jul 22, 2021
@totaam
Copy link
Collaborator

totaam commented Jul 22, 2021

Thanks!

totaam added a commit that referenced this pull request Aug 23, 2021
@totaam
Copy link
Collaborator

totaam commented Aug 23, 2021

This had broken the builds that tried to find nvcc using the now broken which() function.

totaam added a commit that referenced this pull request Sep 18, 2021
totaam added a commit that referenced this pull request Sep 24, 2021
totaam added a commit that referenced this pull request Sep 24, 2021
totaam added a commit that referenced this pull request Sep 24, 2021
totaam added a commit that referenced this pull request Nov 11, 2021
@chewi
Copy link
Contributor

chewi commented Dec 16, 2021

This has broken the tests. Although I agree that which is bad, command is a shell built-in, so you can't call it with Popen via get_status_output like you're doing here.

@totaam
Copy link
Collaborator

totaam commented Dec 17, 2021

@chewi "like you're doing here" - where?

@miojonka
Copy link
Contributor Author

Like here I guess:

def get_status_output(*args, **kwargs):

For finding the executable maybe shutil.which might be more suited. But it requires Python 3.3 at least.

@chewi
Copy link
Contributor

chewi commented Dec 17, 2021

Yes, I was going to try shutil.which. That's okay, Xpra needs at least Python 3.6 anyway.

chewi added a commit to chewi/xpra that referenced this pull request Dec 19, 2021
"command" is a shell built-in so you cannot call it with Popen.

Bug: Xpra-org#3215
totaam pushed a commit that referenced this pull request Dec 19, 2021
"command" is a shell built-in so you cannot call it with Popen.

Bug: #3215
@totaam
Copy link
Collaborator

totaam commented Sep 9, 2022

And yet another breakage is caused by this change: #3610.

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.

None yet

3 participants