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.

Sign up for GitHub

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

Jump to bottom

Switch to command -v over which #8415

Closed
wants to merge 1 commit into from
Closed

Switch to command -v over which #8415

wants to merge 1 commit into from

Conversation

keith
Copy link
Member

@keith keith commented May 21, 2019

This fixes an issue on macOS where which failed to return any results
because the PATH wasn't propagated. command is more standard for
this use case as well https://github.com/koalaman/shellcheck/wiki/SC2230

Fixes #8414

@keith keith mentioned this pull request May 21, 2019
Closed
@keith
Copy link
Member Author

keith commented May 21, 2019

The alternative to this fix, as mentioned in the issue, is to add export PATH above the which calls, but because command is part of the POSIX standard and which is not I decided to go this route

brandjon
brandjon approved these changes May 21, 2019
View reviewed changes
Copy link
Member

@brandjon brandjon left a comment

Choose a reason for hiding this comment

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

I'll merge my in-flight commit (which adds a test case), then merge this on top, including a slight modification to the pywrapper unit test.

@brandjon brandjon self-assigned this May 21, 2019
@keith keith mentioned this pull request May 21, 2019
Closed
@keith keith force-pushed the ks/python-command branch from f74875a to bf176ab Compare May 21, 2019 18:35
@keith
Switch to command -v over which
c242f25 
This fixes an issue on macOS where `which` failed to return any results
because the `PATH` wasn't propagated. `command` is more standard for
this use case as well https://github.com/koalaman/shellcheck/wiki/SC2230

Fixes bazelbuild#8414
@keith keith force-pushed the ks/python-command branch from bf176ab to c242f25 Compare May 21, 2019 18:52
@brandjon
Copy link
Member

Update from offline conversation: the command -v approach doesn't check the executable bit so it's inferior to which in that regard. At Keith's suggesiton I'll use PATH=$PATH which ... instead of exporting PATH, so as not to affect the final process's environment.

@keith keith closed this May 21, 2019
@keith keith deleted the ks/python-command branch May 21, 2019 19:22
bazel-io pushed a commit that referenced this pull request May 21, 2019
@keith @copybara-github
Avoid exporting PATH unnecessarily
ddce723 
This is a modification of PR #8415, which changed `which` to `command -v` so it works when PATH isn't exported. `command` is more standard for this kind of use case (https://github.com/koalaman/shellcheck/wiki/SC2230). Unfortunately, `command -v` doesn't check the executable bit, so it's not as useful for us.

The previous fix for this issue (7f49531) was based on exporting PATH, but this changes the environment seen by the exec'd payload Python code.

The solution in this commit is to not export PATH but rather explicitly pass it to each invocation of `which`.

Fixes #8414. See also bazelbuild/continuous-integration#578. Closes #8415.

PiperOrigin-RevId: 249334274
aehlig pushed a commit that referenced this pull request May 23, 2019
@keith @aehlig
Avoid exporting PATH unnecessarily
1114f06 
This is a modification of PR #8415, which changed `which` to `command -v` so it works when PATH isn't exported. `command` is more standard for this kind of use case (https://github.com/koalaman/shellcheck/wiki/SC2230). Unfortunately, `command -v` doesn't check the executable bit, so it's not as useful for us.

The previous fix for this issue (7f49531) was based on exporting PATH, but this changes the environment seen by the exec'd payload Python code.

The solution in this commit is to not export PATH but rather explicitly pass it to each invocation of `which`.

Fixes #8414. See also bazelbuild/continuous-integration#578. Closes #8415.

PiperOrigin-RevId: 249334274
aehlig pushed a commit that referenced this pull request May 23, 2019
@keith @aehlig
Avoid exporting PATH unnecessarily
c2cf0e2 
This is a modification of PR #8415, which changed `which` to `command -v` so it works when PATH isn't exported. `command` is more standard for this kind of use case (https://github.com/koalaman/shellcheck/wiki/SC2230). Unfortunately, `command -v` doesn't check the executable bit, so it's not as useful for us.

The previous fix for this issue (7f49531) was based on exporting PATH, but this changes the environment seen by the exec'd payload Python code.

The solution in this commit is to not export PATH but rather explicitly pass it to each invocation of `which`.

Fixes #8414. See also bazelbuild/continuous-integration#578. Closes #8415.

PiperOrigin-RevId: 249334274
aehlig pushed a commit that referenced this pull request May 24, 2019
@keith @aehlig
Avoid exporting PATH unnecessarily
ab8d616 
This is a modification of PR #8415, which changed `which` to `command -v` so it works when PATH isn't exported. `command` is more standard for this kind of use case (https://github.com/koalaman/shellcheck/wiki/SC2230). Unfortunately, `command -v` doesn't check the executable bit, so it's not as useful for us.

The previous fix for this issue (7f49531) was based on exporting PATH, but this changes the environment seen by the exec'd payload Python code.

The solution in this commit is to not export PATH but rather explicitly pass it to each invocation of `which`.

Fixes #8414. See also bazelbuild/continuous-integration#578. Closes #8415.

PiperOrigin-RevId: 249334274
irengrig pushed a commit to irengrig/bazel that referenced this pull request Jun 18, 2019
@keith @irengrig
Avoid exporting PATH unnecessarily
980f7a5 
This is a modification of PR bazelbuild#8415, which changed `which` to `command -v` so it works when PATH isn't exported. `command` is more standard for this kind of use case (https://github.com/koalaman/shellcheck/wiki/SC2230). Unfortunately, `command -v` doesn't check the executable bit, so it's not as useful for us.

The previous fix for this issue (bazelbuild@7f49531) was based on exporting PATH, but this changes the environment seen by the exec'd payload Python code.

The solution in this commit is to not export PATH but rather explicitly pass it to each invocation of `which`.

Fixes bazelbuild#8414. See also bazelbuild/continuous-integration#578. Closes bazelbuild#8415.

PiperOrigin-RevId: 249334274
@keith keith mentioned this pull request Oct 11, 2019
Closed
@jzinn
Copy link

jzinn commented Aug 28, 2020

My bazel build failed because which was not installed.

Why not use command -v and then test for the executable bit using [ -x "$PYTHON_BIN" ]?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandjon brandjon brandjon approved these changes

Assignees

@brandjon brandjon

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build failure with --incompatible_use_python_toolchains
4 participants
@keith @brandjon @jzinn @googlebot