workspace_status_command should accept program on the PATH

[alexeagle]

Currently workspace_status_command requires a path to a script in the project.

--workspace_status_command=<path> default: ""
A command invoked at the beginning of the build to provide status information about the workspace in the form of key/value pairs. See the User's Manual for the full specification.

This is hard to make portable between platforms. Making this a shell script means Windows developers on the team can't stamp binaries.

There is some discussion about this:
https://stackoverflow.com/questions/48159516/default-platform-specific-bazel-flags-in-bazel-rc
GerritCodeReview/gerrit@7a8f40f

The current solutions are workarounds.

The most convenient way to solve this IMO is to put a platform-independent script in the PATH, just like git subcommands (e.g. git-bazel-stamp). Then Bazel can always use --workspace_status_command=git-bazel-stamp and it's up to developers to install this git subcommand before stamping releases.

Thanks to @gregmagolan for pointing this out and @laszlocsomor for discussion

[laszlocsomor]

I think this is a reasonable request and I can't think of a good workaround.
Implementation shouldn't be too hard: the Bazel client should look up the --wsc's argument from the PATH.

@lfpino : can the bazel client do this -- what I just wrote in the prev. sentence -- especially considering bazelrc files?

[lfpino]

This issue reminds me of #964.

About your idea Laszlo, --workspace_status_command is parsed in the server so I don't see how the client can inspect the argument.

Maybe the solution is to bundle a platform-independent workspace_status_command in Bazel as suggested in #964 and make it the default value of the flag.

[laszlocsomor]

@lfpino :

About your idea Laszlo, --workspace_status_command is parsed in the server so I don't see how the client can inspect the argument.

Can we change that? I mean can we parse the args in the client and if the argument starts with --workspace_status_command= or it's equal to --workspace_status_command then parse the path from it / from the following arg?

Alternatively, the server needs to know clientEnv["PATH"] to look up the path. I don't know if the server's options parser has access to the clientEnv. @lfpino , do you know?

[laszlocsomor]

Luckily, all the machinery is in place:

bazel/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java

Lines 102 to 113 in 6586621

	this.getWorkspaceStatusCommand =
	options.workspaceStatusCommand.equals(PathFragment.EMPTY_FRAGMENT)
	? null
	: new CommandBuilder()
	.addArgs(options.workspaceStatusCommand.toString())
	// Pass client env, because certain SCM client(like
	// perforce, git) relies on environment variables to work
	// correctly.
	.setEnv(clientEnv)
	.setWorkingDir(workspace)
	.useShell(true)
	.build();

This is where Bazel creates the Command to run the WSC. The clientEnv is already there, so Bazel could just look up the binary from clientEnv[PATH].

[laszlocsomor]

FYI, I don't have the capacity to work on this.

[laszlocsomor]

@alexeagle , I thought more about this problem, and it's transitively closely related to #5034 which is blocking #2190 which I ultimately care about.

I think --workspace_status_command should support absolute paths and workspace-root-relative paths. This way you could check in tools/bazelrc files to the depot with --workspace_status_command flags in them, and not rely on people having the referred script on their PATH but simply check it into the SCM. The path would have to be workspace-relative rather than PWD-relative, so you can still "bazel build" from any directory under the workspace.

@alexeagle , @lberki : WDYT?

[laszlocsomor]

Bumping priority.

[lberki]

Yep, allowing --workspace_status_command to take either an absolute path or a workspace-relative one sounds reasonable.

In fact, it _almost works today, since one could say --workspace_status_command=./wsc.sh, except that PathFragmentConverter probably normalizes ./wsc.sh into wsc.sh which the shell doesn't find because the cwd is usually not on PATH

[lberki]

/proc/self/cwd/wsc.sh does work, though -- not a great workaround, but is a workaround. The cwd is guaranteed to be the workspace root when executing the workspace status command.

[alexeagle]

That workaround doesn't help much given that the OP was for a setting that would work on Windows too.

[laszlocsomor]

Sorry I dropped the ball on this bug for so long.

@alexeagle :

This is hard to make portable between platforms. Making this a shell script means Windows developers on the team can't stamp binaries.

Could you explain why they couldn't stamp binaries?

The most convenient way to solve this IMO is to put a platform-independent script in the PATH, just like git subcommands (e.g. git-bazel-stamp). Then Bazel can always use --workspace_status_command=git-bazel-stamp and it's up to developers to install this git subcommand before stamping releases.

What language would that script be written in?

Currently Bazel runs the --workspace_status_command through Bash on Unixes and through cmd.exe on Windows, which is why --workspace_status_command="echo FOO=bar" works on all platforms.

[alexeagle]

If it's a shell script I can't run it on Windows, right?
How do the git subcommands work? In our case we would probably write the workspace status command in JavaScript and run it with nodejs.
Maybe the workspace status command would just be node path/to/script.js and this would work cross-platform?

[laszlocsomor]

If it's a shell script I can't run it on Windows, right?

Right.

How do the git subcommands work?

Unfortunately I don't know. I reckon the git binary implements some of the commands, and looks up $0-${commandname} for the others.

In our case we would probably write the workspace status command in JavaScript and run it with nodejs.
Maybe the workspace status command would just be node path/to/script.js and this would work cross-platform?

Maybe it already does! Could you give it a try and comment here?

[enriched]

I was able to use node to output stamp vars with bazel v0.16.1 on windows and mac using:

.bazelrc

build --workspace_status_command "node ./build/bazel_stamp_vars.js"

./build/bazel_stamp_vars.js

#!/usr/bin/env node

const childProcess = require('child_process');
const execFile = childProcess.execFile;

function execStampVar(varName, executableFile, args) {
  return new Promise((resolve, reject) => {
    try {
      execFile(executableFile, args, (error, stdout, stderr) => {
        if (error) {
          reject(stderr);
        }
        resolve(`${varName} ${stdout}`);
      });
    } catch (e) {
      reject(e);
    }
  });
}

(async function writeStampVars() {
  const stampVarPromises = [
    execStampVar('GIT_COMMIT', 'git', ['rev-parse', 'HEAD']),
  ];

  const stampVars = await Promise.all(stampVarPromises);
  stampVars.forEach(sv => console.log(sv));
})()
  .then(() => process.exit(0))
  .catch(e => {
    console.error(`Writing stamp variables failed: ${e.message}`);
    process.exit(1);
  });

[alexeagle]

@enriched thanks for demonstrating that! @laszlocsomor I think we could lower the priority.

My only remaining issue is that it requires that node be on your path already. In theory we'll have some full-stack developers who only install Bazel and expect the toolchain to be fetched and put in the external directory. Something like --workspace_status_command "$location(nodejs/node) ./build/bazel_stamp_vars.js" could work (if it understood standard execroot location expansion).

[laszlocsomor]

Thanks @alexeagle and @enriched , I'll lower the priority.

[ulfjack]

We do pass PATH from the client environment through to the workspace action. It seems like it should look up the path on the PATH, doesn't it?

[laszlocsomor]

How do you know that the string contains the path of an executable vs. a Bash command?

[ulfjack]

Remove support for bash commands?

[laszlocsomor]

I doubt we can just remove support for Bash commands, as there are users relying on this feature who would likely miss it.

See: #5002 (comment)