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

cannot pass script/binary in root of repository to --workspace_status_command with bazel 0.12 #5002

Closed
ixdy opened this issue Apr 11, 2018 · 18 comments

Comments

@ixdy
Copy link
Contributor

ixdy commented Apr 11, 2018

Description of the problem / feature request:

In our project (kubernetes/test-infra) we use a shell script to set stamping variables, and we run this on all builds by adding --workspace_status_command=./print-workspace-status.sh to our bazelrc.

Starting with bazel 0.12, this is failing with

ERROR: Process exited with status 127: Process exited with status 127
/bin/sh: print-workspace-status.sh: command not found

If I switch this to --workspace_status_command="bash ./print-workspace-status.sh" it works again.

I'm not sure whether this was an intentional regression. It's not mentioned in the release notes, at the very least.

What operating system are you running Bazel on?

Linux

What's the output of bazel info release?

$ bazel info release
release 0.12.0
@ixdy
Copy link
Contributor Author

ixdy commented Apr 11, 2018

OK, I'm pretty sure this is actually a bug. If I move the script into a subdirectory of our repo (from ./print-workspace-status.sh to ./hack/print-workspace-status.sh), then I can pass it alone.

In other words,

  • This fails: --workspace_status_command=./print-workspace-status.sh (because bazel tries to resolve it to a system binary?)
  • This works: --workspace_status_command=./hack/print-workspace-status.sh

@ixdy ixdy changed the title cannot pass shell script to --workspace_status_command with bazel 0.12 cannot pass script/binary in root of repository to --workspace_status_command with bazel 0.12 Apr 11, 2018
@mattmoor
Copy link

cc @davidstanke

google-prow-robot pushed a commit to knative/eventing that referenced this issue Apr 11, 2018
google-prow-robot pushed a commit to knative/serving that referenced this issue Apr 11, 2018
google-prow-robot pushed a commit to knative/build that referenced this issue Apr 12, 2018
@jin jin added the type: bug label Apr 12, 2018
@jin
Copy link
Member

jin commented Apr 12, 2018

cc @philwo

@philwo
Copy link
Member

philwo commented Apr 12, 2018

Pinging #4582 for reference

@ulfjack
Copy link
Contributor

ulfjack commented Apr 19, 2018

@laszlocsomor, is this due to 073ea09?

That was rolled back in 8358148. Also see #5028.

@ulfjack
Copy link
Contributor

ulfjack commented Apr 19, 2018

That change post-dates this bug report, so it seems unlikely, but maybe it was an earlier change in the same area?

@laszlocsomor
Copy link
Contributor

I can repro this, let me take a look.

@laszlocsomor laszlocsomor self-assigned this Apr 19, 2018
@laszlocsomor
Copy link
Contributor

/cc @tomlu

Culprit is a729b9b, and reason is that the PathFragment string representation no longer contains the leading "./", so Bazel cannot launch the script.

The workaround is either to use the full path of the script, e.g. --workspace_status_command="$(realpath print-workspace-status.sh)" or use it with "bash" as you did, though I plan to break that usecase in Bazel 0.14.0 because --workspace_status_command should receive a path, not a command (see #5034).

@laszlocsomor
Copy link
Contributor

Also I'm going to roll forward 073ea09 and will make sure that passing a workspace-relative path will work.

@ulfjack
Copy link
Contributor

ulfjack commented Apr 19, 2018

"bash print-workspace-status.sh" is a command (with arguments), isn't it?

@ulfjack
Copy link
Contributor

ulfjack commented Apr 19, 2018

We should consider supporting commands-with-arguments in the workspace_status_command. Actually, we might want to consider cleaning up the entire set of command-line flags that affect the workspace status command. :-/

@laszlocsomor
Copy link
Contributor

"bash print-workspace-status.sh" is a command (with arguments), isn't it?

Maybe :) Or a file name: "bash\ print-workspace-status.sh". How do you know?

@laszlocsomor
Copy link
Contributor

The point of 073ea09 was to (a) not require a shell interpreter to run the workspace status command, and (b) make the semantics unambiguous.

@laszlocsomor
Copy link
Contributor

laszlocsomor commented Apr 19, 2018

We should consider supporting commands-with-arguments in the workspace_status_command.

Why? Supporting a script path would be simpler and add just a bit of overhead -- namely having to create a script of the command you want to run (with a proper shebang line thus an absolute path to the right interpreter). It's inconvenient to have to create a script if it's a one-off --workspace_status_command you want to use, but if it's something you build with often then you should check it in as a script.

@laszlocsomor
Copy link
Contributor

I was wrong, --workspace_status_command cannot expect a path lest it would be unusable in bazelrc files if you run the build on multiple platforms. I guess @ulfjack is right and we must support an actual shell command.

@ulfjack
Copy link
Contributor

ulfjack commented Apr 19, 2018

@laszlocsomor I'm not sure I follow. Why can't we expect a path? For example, we could expect a path with forward slashes and automatically convert to the platform-specific separator? This is only an issue for checked-in bazelrc files, right? So it seems fine to restrict it to relative paths pointing at checked in workspace status command binaries.

@laszlocsomor
Copy link
Contributor

laszlocsomor commented Apr 20, 2018

@ulfjack We can expect a path, that's not the problem. But what do you pass as the flag's value, what program works on every platform? Indeed the problem is with checked-in bazelrc files. Imagine it contains --workspace_status_command=tools/foo. What language do you write tools/foo in, what extension should it have so it runs on Windows (too)? We could solve this with platform-specific bazelrc files (see #5055), but for now we don't have them.

Come to think of it, a reasonable compromise would be always running "tools/foo" through Bash except if it's an .exe or .bat file. Though I want to cut the shell dependency (see #4319), it's reasonable to say that if your workspace_status_command is written as a shell script then you need Bash.

@laszlocsomor
Copy link
Contributor

A viable workaround is to implement the workspace status command's logic as a script (e.g. in python), have the interpreter on the PATH, and use --workspace_status_command="python my_wsc.py", which works on all platforms.

vdemeester pushed a commit to vdemeester/knative-build that referenced this issue Apr 3, 2019
vdemeester pushed a commit to vdemeester/knative-build that referenced this issue Apr 3, 2019
vdemeester pushed a commit to vdemeester/knative-build that referenced this issue Apr 3, 2019
vdemeester pushed a commit to vdemeester/knative-build that referenced this issue Apr 3, 2019
vdemeester pushed a commit to vdemeester/knative-build that referenced this issue Apr 3, 2019
markusthoemmes pushed a commit to openshift/knative-serving that referenced this issue Apr 3, 2019
matzew pushed a commit to matzew/eventing that referenced this issue Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants