Add support for invoking bazel with args from file #10796

robbertvanginkel · 2020-02-15T00:24:28Z

Fixes #8609. Also see https://groups.google.com/d/msg/bazel-discuss/895-OjU2Z2A/mYlw05rpDgAJ. Allows Bazel to be invoked with all arguments coming from a file rather than using commandline arguments. For example:

$ cat args.txt
build
//...
$ bazel-bin/src/bazel @args.txt
INFO: Invocation ID: 5a582c6a-4a3c-4c16-bff1-619aba804783
...

With the current implementation: only one argfile is allowed, it should be prefixed with an @ and the first argument (argv[1]). Argfiles should be newline separated (\n), one argument per line.

TODO:

Decide param file location (tempfile? somewhere in output_base?).
Decide command_server.proto changes/semantics when using argfile.
Investigate special quoting needs of param file for JVM on Windows.
Tests
Documentation

Todo: Windows quoting seems to be different and happen in WindowsEscapeArg somewhere. Need to test.

robbertvanginkel · 2020-02-15T00:28:12Z

👋My first time contributing to the bazel client, C++ isn't my area of expertise. Although I realise the PR isn't in a completely mergeable state (missing tests/documentation), I wanted to open a PR earlier rather than later to ask for feedback (so I don't have to rewrite tests/code etc for code that might need a bit of an overhaul).

I'll leave some comments on the files I have open questions/doubts about, please give it a look!

robbertvanginkel · 2020-02-15T00:31:50Z

src/main/cpp/blaze.cc

      const std::string &command, const std::vector<std::string> &command_args,
-      const std::string &invocation_policy,
+      bool uses_param_file, const std::string &invocation_policy,


Maybe refactor Communicate to take a const OptionProcessor *option_processor, as the first three arguments are currently:

server->Communicate( option_processor.GetCommand(), option_processor.GetCommandArguments(), option_processor.UsingParamFile()

robbertvanginkel · 2020-02-15T00:33:18Z

src/main/cpp/blaze.cc

@@ -715,6 +715,20 @@ static void RunBatchMode(

  jvm_args_vector.insert(jvm_args_vector.end(), command_arguments.begin(),
                         command_arguments.end());
+  if (option_processor.UsingParamFile()) {
+    blaze_util::Path param_file =
+      blaze_util::Path(startup_options.output_base).GetRelative("params");


I'm not sure if these param files are safe to be written to output_base, or if they need unique locations to support multiple clients using the same output_base.

You'll probably want to dig around the client locking code to sort out how multiple clients would react to this.

robbertvanginkel · 2020-02-15T00:38:34Z

src/main/cpp/blaze.cc

+        content = blaze_util::Quoted(*i) :
+        content += '\n' + blaze_util::Quoted(*i);


The java argument file format takes all whitespace as separate arguments, so we need to appropriately quote each.
https://docs.oracle.com/javase/9/tools/java.htm#JSWOR-GUID-4856361B-8BFD-4964-AE84-121F5F6CF111

robbertvanginkel · 2020-02-15T00:39:24Z

src/main/cpp/util/strings.cc

+std::string Quoted(const std::string s)
+{
+  std::ostringstream ss;
+  ss << std::quoted( s.c_str() );
+  return ss.str();
+}
+


I'm not sure if there's a better way to achieve quoting that java expects the argfile to have.

robbertvanginkel · 2020-02-15T00:43:23Z

src/main/protobuf/command_server.proto

+
+  string parameter_file = 7;


I think this should be a oneof with repeated bytes arg = 2;, that way it is clear for the java code which field to parse. Right now the java code attempts to parse field 2, if its empty it attempts to parse this.

I'm not sure where this proto file is used, and if this would be backward compatible change. Any tips on how to do this best welcome.

philwo · 2020-02-19T12:52:02Z

Hi @robbertvanginkel,

thanks for your contribution! This seems like a pretty cool feature to have. I'll try to find you a reviewer for this!

laurentlb · 2020-02-21T20:02:36Z

src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java

+            args = builder.build();
+          } catch (IOException e) {
+          }
+        }


Do you really need to modify the Java code?
I think it would be simpler to open the file and do the expansion in option_processor.cc.
(also skip the change to the proto file)

Yes, after doing the expansion in the cpp client we still need to send it to the java server. As the server/client communicate over gRPC this still has a message size limit, see #8609 (comment) for some more context.

The only solution I found for that is making sure the client writes the arguments to a file and instructs the server to read that file, for which we'd need a proto change (or implicitly encode this somehow in the existing field for args).

I'm told that the gRPC limit is entirely artificial and we can just increase it.

Call .maxInboundMessageSize(1<<30) on NettyServerBuilder here:

bazel/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java

Line 439 in 636430a

NettyServerBuilder.forAddress(address).addService(this).directExecutor().build().start();

Let's keep a sensible limit on the max inbound message size.

@michajlo what would you consider a sensible limit? 10/100s of megabytes?

What I like about the file approach is that it doesn't impose another seemingly arbitrarily chosen limit. What I'd like to avoid is ending up with a choice that needs to be increased 6months or a year down the line. Maybe the limit should be configurable?

Probably 10s? Generous arbitrary limits are nice because they provide some healthy pushback to prevent folks from wandering into the minefield of less well defined limits.

pcj · 2020-02-24T15:13:41Z

I don't think we should assume that the bazel client and bazel server are always running on the same machine (and sharing the same filesystem), so changing the command_server.proto is probably not a good idea.

michajlo

Unfortunately given the client-server setup and the misc paths to starting up a server or initiating requests, param files can wind up getting tricky. This might require a larger design discussion to make sure we don't accidentally paint ourselves into any corners. I'm happy to review and help you with the design if you decide to take it on, but I'm also going to offer what might be a more direct route to solving your initial problem over on #8609.

michajlo · 2020-02-24T19:20:30Z

src/main/cpp/blaze.cc

-  for (const string &arg : arg_vector) {
-    request.add_arg(arg);
+  if (uses_param_file) {
+    blaze_util::Path param_file = output_base_.GetRelative("params");


There can definitely be races between clients here - we release the client-side lock right after sending the request, so by the time the file is being parsed on the server it could have been replaced by another file.

michajlo · 2020-02-24T19:20:40Z

src/main/cpp/blaze.cc

@@ -715,6 +715,20 @@ static void RunBatchMode(

  jvm_args_vector.insert(jvm_args_vector.end(), command_arguments.begin(),
                         command_arguments.end());
+  if (option_processor.UsingParamFile()) {
+    blaze_util::Path param_file =
+      blaze_util::Path(startup_options.output_base).GetRelative("params");


You'll probably want to dig around the client locking code to sort out how multiple clients would react to this.

michajlo · 2020-02-24T19:28:52Z

src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java

+            args = builder.build();
+          } catch (IOException e) {
+          }
+        }


Probably 10s? Generous arbitrary limits are nice because they provide some healthy pushback to prevent folks from wandering into the minefield of less well defined limits.

michajlo · 2020-02-24T19:48:49Z

src/main/cpp/option_processor.cc

+    using_param_file = true;
+    string filename = args[1].substr(1, args[1].length()-1);
+    string contents;
+    if (!blaze_util::ReadFile(filename, &contents)) {


Let's avoid io in SplitCommandLine, and generally as much as possible in options processing besides for rc handling. Can this be lifted to a different abstraction layer? This also avoids mutating args, which could get confusing.

robbertvanginkel · 2020-02-25T04:35:06Z

Thanks all for the comments! There's definitely some extra investigation to do here, I agree with @michajlo that it is probably a good idea to have a larger design discussion around it.

However, I think that might take some more time and I would like to have a reasonable resolution for #8609 soon. The alternative suggested (a --build_file option similar to the existing --query_file) seems like a pretty straightforward implementation (#10856), although a bit less generic. Please give that idea a look, I hope that can be merged a bit sooner and be a stop gap till I can spend the time working out the details for a generic bazel arguments file discussion.

An alternative to bazelbuild#10796 to fix bazelbuild#8609. As bazelbuild#8609 (comment) points out there's currently a --query_file to read a query pattern from file, but there's no analogous feature available for build/test. Generic parameter file support attempted in bazelbuild#10796 turns out to be a little more complex than expected due to bazel's client/server architecture and likely requires some design document. This PR would fix help bazelbuild#8609 without rushing into any designs, so we can spend time properly designing generic parameter files while also alleviating the immediate problem with a known/simpler pattern.

An alternative to #10796 to fix #8609. As #8609 (comment) points out there's currently a `--query_file` to read a query pattern from file, but there's no analogous feature available for build/test. Generic parameter file support attempted in #10796 turns out to be a little more complex than expected due to bazel's client/server architecture and likely requires some design document. This PR would fix help #8609 without rushing into any designs, so we can spend time properly designing generic parameter files while also alleviating the immediate problem with a known/simpler pattern. Closes #10856. PiperOrigin-RevId: 298953350

jlisee · 2020-03-11T13:10:05Z

With #10856 landed should this be closed?

robbertvanginkel · 2020-03-13T21:15:34Z

Yes #10856 fixes my immediate issue for #8609. I don't have the time right now to dive into general argfile support and writeup a proposal.

Closing out this PR to reflect I'm not actively working on it. If anyone ends up looking at argfile support in the future, I hope this points them in the right direction :).

An alternative to bazelbuild#10796 to fix bazelbuild#8609. As bazelbuild#8609 (comment) points out there's currently a `--query_file` to read a query pattern from file, but there's no analogous feature available for build/test. Generic parameter file support attempted in bazelbuild#10796 turns out to be a little more complex than expected due to bazel's client/server architecture and likely requires some design document. This PR would fix help bazelbuild#8609 without rushing into any designs, so we can spend time properly designing generic parameter files while also alleviating the immediate problem with a known/simpler pattern. Closes bazelbuild#10856. PiperOrigin-RevId: 298953350

robbertvanginkel added 3 commits February 13, 2020 13:09

Parse param file in bazel client

d297fb4

Make --batch work using java argument files

51141c2

Todo: Windows quoting seems to be different and happen in WindowsEscapeArg somewhere. Need to test.

Add argfile support to client/server mode

9ffae4a

googlebot added the cla: yes label Feb 15, 2020

robbertvanginkel commented Feb 15, 2020

View reviewed changes

jin assigned laurentlb Feb 15, 2020

aiuto added team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website untriaged labels Feb 15, 2020

aiuto requested review from philwo and jin February 15, 2020 03:22

aiuto unassigned laurentlb Feb 15, 2020

laurentlb reviewed Feb 21, 2020

View reviewed changes

michajlo reviewed Feb 24, 2020

View reviewed changes

jin removed their request for review February 24, 2020 21:16

robbertvanginkel mentioned this pull request Feb 25, 2020

Implement --target_pattern_file flag for reading patterns from file #10856

Closed

robbertvanginkel closed this Mar 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for invoking bazel with args from file #10796

Add support for invoking bazel with args from file #10796

robbertvanginkel commented Feb 15, 2020

robbertvanginkel commented Feb 15, 2020

robbertvanginkel Feb 15, 2020

robbertvanginkel Feb 15, 2020

michajlo Feb 24, 2020

robbertvanginkel Feb 15, 2020

robbertvanginkel Feb 15, 2020

robbertvanginkel Feb 15, 2020

philwo commented Feb 19, 2020

laurentlb Feb 21, 2020

robbertvanginkel Feb 21, 2020

laurentlb Feb 24, 2020

michajlo Feb 24, 2020

robbertvanginkel Feb 24, 2020

michajlo Feb 24, 2020

pcj commented Feb 24, 2020

michajlo left a comment

michajlo Feb 24, 2020

michajlo Feb 24, 2020

michajlo Feb 24, 2020

michajlo Feb 24, 2020

robbertvanginkel commented Feb 25, 2020

jlisee commented Mar 11, 2020

robbertvanginkel commented Mar 13, 2020

		content = blaze_util::Quoted(*i) :
		content += '\n' + blaze_util::Quoted(*i);

Add support for invoking bazel with args from file #10796

Add support for invoking bazel with args from file #10796

Conversation

robbertvanginkel commented Feb 15, 2020

robbertvanginkel commented Feb 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philwo commented Feb 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcj commented Feb 24, 2020

michajlo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbertvanginkel commented Feb 25, 2020

jlisee commented Mar 11, 2020

robbertvanginkel commented Mar 13, 2020