Skip to content

Commit

Permalink
Allow blaze info to ignore starlark options
Browse files Browse the repository at this point in the history 
Fixes #11301

PiperOrigin-RevId: 312470710
  • Loading branch information
@juliexxia
juliexxia authored and Copybara-Service committed May 20, 2020
1 parent fbc43bb commit 2ec58f6
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 1 deletion.
19 changes: 19 additions & 0 deletions src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.runtime;

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.devtools.build.lib.analysis.config.CoreOptionConverters.BUILD_SETTING_CONVERTERS;
import static com.google.devtools.build.lib.packages.RuleClass.Builder.STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;
import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
Expand Down Expand Up @@ -216,6 +217,24 @@ private Target loadBuildSetting(
return buildSetting;
}

/**
* Separates out any Starlark options from the given list
*
* @param list List of strings from which to parse out starlark options
* @return Returns a pair of string lists. The first item contains the list of starlark options
* that were removed; the second contains the remaining string from the original list.
*/
public static Pair<ImmutableList<String>, ImmutableList<String>> removeStarlarkOptions(
List<String> list) {
ImmutableList<String> removed =
list.stream()
.filter(r -> r.startsWith("--//") || r.startsWith("--no//"))

This comment has been minimized.

Sign in to view

Copy link
@jin

jin May 21, 2020

Member

@juliexxia this doesn't include starlark options from external repositories, right? e.g. in rules_jvm_external, users can set --@rules_jvm_external//setting:stamp_manifest (bazelbuild/rules_jvm_external#355)

This comment has been minimized.

Sign in to view

Copy link
@juliexxia

juliexxia May 21, 2020

Author Contributor

@gregestren (Greg, I mentioned this to you in our 1:1)

@jin good point I hadn't thought of that. I can't quite tell from your comment, is ignoring external repo starlark flags a helpful behavior? I think it's a little strange because it's inconsistent but if it's helpful I think it'd be ok to leave this way.

This comment has been minimized.

Sign in to view

Copy link
@gregestren

gregestren May 21, 2020

Contributor

Is the argument that both --//foo and --@something//foo should be consistently ignored since they'd both create the same error otherwise?

Also, tangential heads up: looks like we're going to prioritize work on short-flags, which would let people map --@rules_jvm_external//setting:stamp_manifest to just --stamp_manifest or whatever other easier-to-type form they want.

This comment has been minimized.

Sign in to view

Copy link
@jin

jin May 21, 2020

Member

Yeah, users could have a Starlark option from an external repo in their .bazelrc, and that would fail with bazel info.

It's not an urgent request, just wanted to give a heads up about this. Looking forward to short flags!

This comment has been minimized.

Sign in to view

Copy link
@juliexxia

juliexxia May 26, 2020

Author Contributor

Gotcha gotcha - appreciate the heads up!

This comment has been minimized.

Sign in to view

Copy link
@juliexxia

juliexxia May 27, 2020

Author Contributor

FYI, #11510

This comment has been minimized.

Sign in to view

Copy link
@jin

jin May 28, 2020

Member

Thanks!
.collect(toImmutableList());
ImmutableList<String> kept =
list.stream().filter(r -> !removed.contains(r)).collect(toImmutableList());
return Pair.of(removed, kept);
}

@VisibleForTesting
public static StarlarkOptionsParser newStarlarkOptionsParserForTesting(
SkyframeExecutor skyframeExecutor,
Expand Down
14 changes: 13 additions & 1 deletion src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.devtools.build.lib.runtime.Command;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.runtime.InfoItem;
import com.google.devtools.build.lib.runtime.StarlarkOptionsParser;
import com.google.devtools.build.lib.runtime.commands.info.BlazeBinInfoItem;
import com.google.devtools.build.lib.runtime.commands.info.BlazeGenfilesInfoItem;
import com.google.devtools.build.lib.runtime.commands.info.BlazeTestlogsInfoItem;
Expand Down Expand Up @@ -64,6 +65,7 @@
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.ExitCode;
import com.google.devtools.build.lib.util.InterruptedFailureDetails;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.io.OutErr;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
Expand Down Expand Up @@ -198,7 +200,17 @@ public BlazeCommandResult exec(
}
}

List<String> residue = optionsParsingResult.getResidue();
Pair<ImmutableList<String>, ImmutableList<String>> starlarkOptionsAndResidue =
StarlarkOptionsParser.removeStarlarkOptions(optionsParsingResult.getResidue());
ImmutableList<String> removedStarlarkOptions = starlarkOptionsAndResidue.getFirst();
ImmutableList<String> residue = starlarkOptionsAndResidue.getSecond();
if (!removedStarlarkOptions.isEmpty()) {
env.getReporter()
.handle(
Event.warn(
"Blaze info does not support starlark options. Ignoring options: "
+ removedStarlarkOptions));
}
if (residue.size() > 1) {
String message = "at most one key may be specified";
env.getReporter().handle(Event.error(message));
Expand Down

0 comments on commit 2ec58f6

Please sign in to comment.