Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bazelrc: Remove inheritance of targets from bazel build in bazel cquery, bazel clean #8014

Closed
excitoon opened this issue Apr 12, 2019 · 22 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Milestone

Comments

@excitoon
Copy link
Contributor

.bazelrc:

build //some:target # This is pretty useful

With such settings one no longer use cquery and clean because targets are going to its command line which is not intended for that.

Can we just skip inheriting targets from build (but leave settings as they are useful) in these commands?

@aiuto aiuto added team-Configurability platforms, toolchains, cquery, select(), config transitions untriaged labels Apr 17, 2019
@aiuto
Copy link
Contributor

aiuto commented Apr 17, 2019

Yuch. I would propose that the request is really more like
"Only apply rc lines to the specific command named"
That would imply we fix this all around, rather than just special casing clean and cquery.

The docs (https://docs.bazel.build/versions/master/guide.html#bazelrc), OTOH, talk about rule inheritance, such that test inherits from build. That is convenient, but magical behavior, since it is not obvious it should be so. I would advocate that we should eliminate that inheritance and clarify that

  • while building targets for test runs, the build options apply. That can happen without any notion of command inheritance.

@excitoon
Copy link
Contributor Author

@aiuto I see it more like "only apply inheritance rules to options (starting from '-')".

Generally, you cquery the same option set as you build for. That is why inheritance is a good thing here.

@gregestren
Copy link
Contributor

I didn't even realize you could set targets in the bazelrc. I wonder how common that is.

@excitoon - what exactly do you do with this? Are you usually only building a single target?

@excitoon
Copy link
Contributor Author

@gregestren Well, building bunch of targets by one --config setting (and other bunch by another). This is pretty useful thing. Is that something accidental?

@gregestren
Copy link
Contributor

gregestren commented Apr 18, 2019

No, I don't think it's accidental. But I wasn't personally familiar with it, and I know people have different usage patterns.

That honestly sounds reasonable to me.

@programmablereya programmablereya added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels Apr 18, 2019
@ghost
Copy link

ghost commented Feb 18, 2020

Just ran into this with clean when trying out user-defined flags.

$ cat .bazelrc
build --no//my/project:boolean_flag
$ bazel clean
...
ERROR: Unrecognized arguments: --no//my/project:boolean_flag

A comment in CleanCommand.java suggests that its inheritance from build should go away:

// TODO(bazel-team): Remove this - we inherit a huge number of unused options.
inherits = {BuildCommand.class}

It makes sense to me why cquery inherits from build, but I don't yet know why clean should. (Maybe to inherit the output_root family of args?) I'll probably patch this out locally.

@gregestren
Copy link
Contributor

Please share what you find, Ryan. I'm also marking this to discuss later this week among some core Bazel devs to see if anyone knows the history on this.

@ghost
Copy link

ghost commented Feb 19, 2020

Okay, so I read the rest of CleanCommand.java, and the only referenced flag from build is --symlink-prefix. It'd be easy to point CleanCommand to BuildRequestOptions.class and drop inheritance from build, but then users who set --symlink_prefix via .bazelrc (everyone using --symlink_prefix?) would end up with dangling symlinks w/o also adding a clean stanza.

However, @aiuto's take in #8014 (comment) is prolly the right call for Bazel as a whole. I'm guessing it wouldn't take a whole lot of surgery to implement, but as a breaking change it'd have to wait until the next major release to ship.

void parseRcOptions(
EventHandler eventHandler, ListMultimap<String, RcChunkOfArgs> commandToRcArgs)
throws OptionsParsingException {
for (String commandToParse : getCommandNamesToParse(commandAnnotation)) {
// Get all args defined for this command (or "common"), grouped by rc chunk.
for (RcChunkOfArgs rcArgs : commandToRcArgs.get(commandToParse)) {
if (!rcArgs.args.isEmpty()) {
String inherited = commandToParse.equals(commandAnnotation.name()) ? "" : "Inherited ";
String source =
rcArgs.rcFile.equals("client")
? "Options provided by the client"
: String.format(
"Reading rc options for '%s' from %s",
commandAnnotation.name(), rcArgs.rcFile);
rcfileNotes.add(
String.format(
"%s:\n %s'%s' options: %s",
source, inherited, commandToParse, Joiner.on(' ').join(rcArgs.args)));
}
optionsParser.parse(PriorityCategory.RC_FILE, rcArgs.rcFile, rcArgs.args);
}
}
}

private static List<String> getCommandNamesToParse(Command commandAnnotation) {
List<String> result = new ArrayList<>();
result.add("common");
getCommandNamesToParseHelper(commandAnnotation, result);
return result;
}
private static void getCommandNamesToParseHelper(
Command commandAnnotation, List<String> accumulator) {
for (Class<? extends BlazeCommand> base : commandAnnotation.inherits()) {
getCommandNamesToParseHelper(base.getAnnotation(Command.class), accumulator);
}
accumulator.add(commandAnnotation.name());
}

(Delete getCommandNamesToParseHelper and replace its call in getCommandNamesToParse with result.add(commandAnnotation.name()).)

@gabroo
Copy link

gabroo commented Mar 2, 2020

What is the recommended way of specifying targets for bazel build in the .bazelrc without breaking bazel clean?

image

@gregestren
Copy link
Contributor

Thanks for this feedback.

However, @aiuto's take in #8014 (comment) is prolly the right call for Bazel as a whole. I'm guessing it wouldn't take a whole lot of surgery to implement, but as a breaking change it'd have to wait until the next major release to ship.

It's not obvious to me breaking the inheritance between test and build offers the right experience. All test is, ultimately, is a glorified build + run. The build part is already implicit, so the relationship between their flags doesn't feel arbitrary to me.

@gregestren
Copy link
Contributor

What is the recommended way of specifying targets for bazel build in the .bazelrc without breaking bazel clean?

You could always add a --config to your build. But it's not ideal since it takes away some of the convenience of having the target auto-trigger in the first place.

I think we need Bazel changes to make this use case work cleanly.

@gabroo
Copy link

gabroo commented Mar 9, 2020

Thanks for the suggestion. I'm using --config for now, and it works

@sbenedetto
Copy link

Hi, What is the status of this ticket? I also stumbled upon this issue. We have configs where we exclude targets during the build, but the list of excluded targets is inherited also by the run (along with test and clean) command and it fails miserably when executing the application as the excluded list of targets is passed to the application.

@gregestren
Copy link
Contributor

bazel clean failing with user-defined flags (#8014 (comment)) is fixed with #11506. --no//my/project:boolean_flag still fails but --//my/project:boolean_flag=0 works.

I think we could fix bazel clean failures on //some:target by a small extension to https://github.com/bazelbuild/bazel/pull/11508/files that changes lines 137-140 to a warning vs. failure.

Excluding targets differently for build vs. test vs. run isn't obvious because there's legitimate overlap for these commands. Eliminating that inheritance can just as easily break users who depend on it. For this case it's really best to break the ambiguity by creating a --config=exclude_targets mode and triggering that for the appropriate invocation.

@djmarcin
Copy link

bazel clean failing with user-defined flags (#8014 (comment)) is fixed with #11506. --no//my/project:boolean_flag still fails but --//my/project:boolean_flag=0 works.

It seems that if a string or string list flag contains the string "//" then it still fails:

build --@rules_rust//:experimental_incremental_prefixes=//foo/bar
$ bazel clean
INFO: Invocation ID: 036ac0f9-0e69-459a-91fe-9787d4cf3c9f
ERROR: Unrecognized arguments: --@rules_rust//:experimental_incremental_prefixes=//foo/bar

Changing this to just a single slash causes the warning instead:

$ bazel clean
INFO: Invocation ID: 86dbe56b-9c0f-47e5-a8c5-22009db74fbe
WARNING: Blaze clean does not support starlark options. Ignoring options: [--@rules_rust//:experimental_incremental_prefixes=/foo/bar]
INFO: Starting clean (this may take a while). Consider using --async if the clean takes more than several minutes.

@gregestren
Copy link
Contributor

@djmarcin which Bazel version were you using? Note #12808 (comment).

@djmarcin
Copy link

djmarcin commented Jun 8, 2021

It fails on 4.0.0 and 4.1.0. Maybe I need to wait for 4.2.0?

@gregestren
Copy link
Contributor

#12808 (comment) confirms the latest code will only show up in 4.2.0 (assuming the cherrypick is accepted). I think that will address your use case too. You can also try with a rolling release Bazel, which has everything: #13505.

@aiuto aiuto added this to the flags cleanup milestone Dec 11, 2021
@anthpi
Copy link
Contributor

anthpi commented Sep 2, 2022

Any update on this issue? We would like NOT to get any warning for user defined build settings when running bazel clean command.

@gregestren
Copy link
Contributor

See the final comments at #13473. I'd ask for any further clarification on that thread.

Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Nov 18, 2023
Copy link

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please post @bazelbuild/triage in a comment here and we'll take a look. Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Development

No branches or pull requests

8 participants