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

robbertvanginkel · 2020-02-25T04:24:49Z

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.

robbertvanginkel · 2020-02-25T04:28:37Z

@michajlo thanks for your comments in #10796 (review) and your suggestion in #8609 (comment). While would like generic parameter file support in Bazel, I would also appreciate a more immediate solution to #8609. Suggesting we go with this much simpler approach for now till I can spend the time on a generic solution to the open questions in #10796.

src/main/java/com/google/devtools/build/lib/runtime/commands/BuildCommand.java

michajlo

Thanks for the changes!

src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java

src/main/java/com/google/devtools/build/lib/runtime/commands/BuildCommand.java

src/test/shell/integration/build_file_test.sh

src/main/java/com/google/devtools/build/lib/runtime/commands/BuildCommand.java

michajlo

If you can figure out how to add a simple java unit test for TargetPatternFileSupport it'd be much appreciated - easier to get more coverage more quickly than the integration tests. Otherwise just minor comments.

src/main/java/com/google/devtools/build/lib/runtime/commands/BuildCommand.java

src/main/java/com/google/devtools/build/lib/runtime/commands/TargetPatternFileSupport.java

jin · 2020-02-28T18:11:10Z

Reviewers: once approved, please read the internal go/import-bazel-pr document for PR import instructions.

robbertvanginkel · 2020-02-29T02:46:57Z

Processed the comments but did not get to the unit test yet. I looked at making one, but standing up a CommandEnvironment to pass in is a little tricky (its constructor is not public and in a different package), and when I attempted to make a mockito mock I got an error like

Cannot mock/spy class com.google.devtools.build.lib.runtime.CommandEnvironment
Mockito cannot mock/spy because :
 - final class

I'm not sure how best to approach that, if you have any recommendation please let me know and I'll try to finish the unit test.

michajlo · 2020-03-02T18:33:02Z

CommandEnvironment should be mockable as of an hour or so ago.

michajlo · 2020-03-04T01:51:05Z

Let me know when that last test is ready, otherwise everything else lgtm.

robbertvanginkel · 2020-03-04T01:55:41Z

@michajlo the unit test should be there: https://github.com/bazelbuild/bazel/blob/20964788553921e0335d5c34cd4bf70d18c4b7dd/src/test/java/com/google/devtools/build/lib/runtime/commands/TargetPatternFileSupportTest.java

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.

robbertvanginkel · 2020-03-04T02:30:24Z

@michajlo I squashed the changes and rebased them on master to resolve a merge conflict github was showing. It seems like there was a few changes to use DetailedExitCode instead of ExitCode, I attempted to update the references.

I think all the changes, including unit and integration tests should be there. Please let me know if theres anything else!

michajlo

All lgtm, just style/naming things on this round then good to go.

michajlo · 2020-03-04T02:33:58Z

src/main/java/com/google/devtools/build/lib/runtime/commands/TargetPatternFileSupport.java

+   * list of targets based on the supplied command line options.
+   */
+  public static class TargetPatternFileSupportException extends Exception {
+    public TargetPatternFileSupportException(String message) { super(message); }


style thing, mv the super(...) to a new line:

... { super(message); }

Fixed. Does Bazel have something like google-java-format setup? I couldn't find it myself in the contribution guidelines.

src/main/java/com/google/devtools/build/lib/runtime/commands/TargetPatternFileSupport.java

src/test/java/com/google/devtools/build/lib/runtime/commands/TargetPatternFileSupportTest.java

- TargetPatternFileSupport -> TargetPatternsHelper - Add unittest for empty case - Comments/style

robbertvanginkel · 2020-03-04T03:18:46Z

All comments should be taken care of.

src/main/java/com/google/devtools/build/lib/runtime/commands/TargetPatternFileSupport.java

robbertvanginkel · 2020-03-04T23:27:30Z

Thanks @michajlo!

isaprykin · 2020-08-13T03:36:48Z

--query_file accepts space separated list of targets.
It needs to be clarified that this only accepts newline separated list of targets.
I costed me a few hours.

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

googlebot added the cla: yes label Feb 25, 2020

robbertvanginkel requested a review from michajlo February 25, 2020 04:25

robbertvanginkel mentioned this pull request Feb 25, 2020

Add support for invoking bazel with args from file #10796

Closed

5 tasks

robbertvanginkel commented Feb 25, 2020

View reviewed changes

src/main/java/com/google/devtools/build/lib/runtime/commands/BuildCommand.java Outdated Show resolved Hide resolved

michajlo reviewed Feb 26, 2020

View reviewed changes

michajlo reviewed Feb 28, 2020

View reviewed changes

src/main/java/com/google/devtools/build/lib/runtime/commands/BuildCommand.java Outdated Show resolved Hide resolved

src/main/java/com/google/devtools/build/lib/runtime/commands/TargetPatternFileSupport.java Outdated Show resolved Hide resolved

jin assigned michajlo Feb 28, 2020

robbertvanginkel force-pushed the robbert/build_file branch from b57f364 to 904fd51 Compare March 2, 2020 19:46

robbertvanginkel changed the title ~~Implement --build_file flag for reading patterns from file~~ Implement --target_pattern_file flag for reading patterns from file Mar 3, 2020

benjaminp mentioned this pull request Mar 4, 2020

Aliases don't correctly destination targets that contain a / #4205

Closed

robbertvanginkel force-pushed the robbert/build_file branch from 2096478 to 71bad6b Compare March 4, 2020 02:27

michajlo requested changes Mar 4, 2020

View reviewed changes

Adress some feedback.

cd3632b

- TargetPatternFileSupport -> TargetPatternsHelper - Add unittest for empty case - Comments/style

robbertvanginkel requested a review from michajlo March 4, 2020 20:35

michajlo approved these changes Mar 4, 2020

View reviewed changes

michajlo requested changes Mar 4, 2020

View reviewed changes

src/main/java/com/google/devtools/build/lib/runtime/commands/TargetPatternFileSupport.java Outdated Show resolved Hide resolved

Add copyright header to TargetPatternsHelper.java

6b76aef

robbertvanginkel requested a review from michajlo March 4, 2020 21:27

bazel-io closed this in bdae1c2 Mar 4, 2020

robbertvanginkel deleted the robbert/build_file branch March 4, 2020 23:27

JamyDev mentioned this pull request Dec 2, 2022

fix(gpd): Write large target patterns to file bazelbuild/rules_go#3372

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

robbertvanginkel commented Feb 25, 2020

robbertvanginkel commented Feb 25, 2020

michajlo left a comment

michajlo left a comment

jin commented Feb 28, 2020 •

edited

Loading

robbertvanginkel commented Feb 29, 2020

michajlo commented Mar 2, 2020

michajlo commented Mar 4, 2020

robbertvanginkel commented Mar 4, 2020

robbertvanginkel commented Mar 4, 2020

michajlo left a comment

michajlo Mar 4, 2020

robbertvanginkel Mar 4, 2020

robbertvanginkel commented Mar 4, 2020

robbertvanginkel commented Mar 4, 2020

isaprykin commented Aug 13, 2020

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

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

Conversation

robbertvanginkel commented Feb 25, 2020

robbertvanginkel commented Feb 25, 2020

michajlo left a comment

Choose a reason for hiding this comment

michajlo left a comment

Choose a reason for hiding this comment

jin commented Feb 28, 2020 • edited Loading

robbertvanginkel commented Feb 29, 2020

michajlo commented Mar 2, 2020

michajlo commented Mar 4, 2020

robbertvanginkel commented Mar 4, 2020

robbertvanginkel commented Mar 4, 2020

michajlo left a comment

Choose a reason for hiding this comment

michajlo Mar 4, 2020

Choose a reason for hiding this comment

robbertvanginkel Mar 4, 2020

Choose a reason for hiding this comment

robbertvanginkel commented Mar 4, 2020

robbertvanginkel commented Mar 4, 2020

isaprykin commented Aug 13, 2020

jin commented Feb 28, 2020 •

edited

Loading