Add bazel query --output_file option, which writes query results directly to a file

[keithl-stripe]

This is a proposed fix for #24293

This speeds up a fully warm bazel query ... by 23.7%, reducing wall time from 1m49s to 1m23s

$ time bazel query '...' --output=streamed_proto > queryoutput4.streamedproto

real    1m48.768s
user    0m27.410s
sys     0m19.646s

$ time bazel query '...' --output=streamed_proto --output_file=queryoutput5.streamedproto

real    1m22.920s
user    0m0.045s
sys     0m0.016s

💁‍♂️ Note: when combined with #24305, total wall time is 37s, an overall reduction of 66%.

[jin]

Should this be CommandEnvironment#getWorkspace or CommandEnvironment#getWorkingDirectory? The latter should let you store the file beneath the workspace root?

[michajlo]

We're working on channeling anything (via flags) that writes to the local filesystem outside of the output tree to the InstrumentationOutput abstraction? Could you see if that could be used without too much hardship? We're still working out the ergonomics, so I apologize if it might not be straightforward (yet).

[keithl-stripe]

We're working on channeling anything (via flags) that writes to the local filesystem outside of the output tree to the InstrumentationOutput abstraction? Could you see if that could be used without too much hardship? We're still working out the ergonomics, so I apologize if it might not be straightforward (yet).

I'm OK with trying this, but from the name and docs, InstrumentationOutput does not quite seem like a good fit. The file produced --output_file is the entire result of the bazel query command, not just instrumentation/metrics about Bazel itself.

[michajlo]

InstrumentationOutput does not quite seem like a good fit. The file produced --output_file is the entire result of the bazel query command, not just instrumentation/metrics about Bazel itself.

Agreed the name isn't ideal, but it's the same thing in spirit. I think this just might be the ~first¹ time we've had something like this, or the first we've encountered it since taking inventory and naming this concept

Footnotes

1. We actually had something like the proposed flag that we removed a while ago, as noted in https://github.com/bazelbuild/bazel/issues/24293#issuecomment-2473826800. ↩

[keithl-stripe]

InstrumentationOutput does not quite seem like a good fit. The file produced --output_file is the entire result of the bazel query command, not just instrumentation/metrics about Bazel itself.

Agreed the name isn't ideal, but it's the same thing in spirit. I think this just might be the ~first1 time we've had something like this, or the first we've encountered it since taking inventory and naming this concept

Footnotes

1. We actually had something like the proposed flag that we removed a while ago, as noted in Add option to write bazel query output directly to a file #24293 (comment). ↩

I just tried using InstrumentationOutput and it doesn't seem like a great fit in its current form:

1. Doesn't allow paths relative to the user's working directory
2. Doesn't allow introspecting the resolved path (I use the path in an error message)
3. I don't see how to integrate it with LazyFileOutputStream

[keithl-stripe]

I just tried using InstrumentationOutput and it doesn't seem like a great fit in its current form:

1. Doesn't allow paths relative to the user's working directory
2. Doesn't allow introspecting the resolved path (I use the path in an error message)
3. I don't see how to integrate it with LazyFileOutputStream

Update: I fixed 1 and 2 myself, and @jin said I don't need 3.

[yuyue730]

As the author of InstrumentationOutput, I want to sincerely thank you for adopting InstrumentationOutput.

However, please note that env.getWorkingDirectory() is not "getting the directory from which the user ran Bazel". From its implementation and JavaDoc, this method returns "working directory of the server", which could be identical to workspace, in most cases I think. I apologize for this confusion. And this is painful for us as well, which makes us making the initial assumption to conflate workspace and working directory.

As of now, we are researching about fixing this flawed assumption. So it is totally fine to opt out of adopting InstrumentationOutput in this change. I think we can do the switch after we know how to sort out this ball of mud.

[keithl-stripe]

Ok, thanks for the explanation! Note: you linked to BlazeDirectories.getWorkingDirectory, which you described accurately; but I was not calling that; I was calling CommandEnvironment.getWorkingDirectory(), which is the client working directory :)

bazel/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java

Lines 653 to 662 in 72a9d49

	/**
	* Returns the working directory of the {@code blaze} client process.
	*
	* <p>This may be equal to {@code BlazeRuntime#getWorkspace()}, or beneath it.
	*
	* @see #getWorkspace()
	*/
	public Path getWorkingDirectory() {
	return workingDirectory;
	}

[yuyue730]

Sorry, I misread on this. Then this looks good to me.

[michajlo]

i think you alluded to this in my other comment - is this the absolute path? a perhaps not-well-documented thing we like to try to do is avoid absolute paths in user-visible messaging as much as possible, this way if the local filesystem (CI etc) doesn't match the end-user's filesystem there's less confusion. also, as you may have deduced from the instrumentation-output abstraction, this path might not reflect the actual file we were operating on.

that said, logging absolute paths to info logging should be fine.

[keithl-stripe]

Hmm, okay. Personally when Bazel outputs a path, it can be difficult to understand which of Bazel's many directories it is relative to. So I would prefer Bazel only print absolute paths :)

[jin]

LGTM modulo tests.

[jin]

Your added test is failing internally. I'll attempt to fix it, but apologies if there's a delay.

[keithl-stripe]

Your added test is failing internally. I'll attempt to fix it, but apologies if there's a delay.

Thank you so much for the update! Hope they are not too onerous to fix.