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.

Sign up for GitHub

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

Fix label conversion for --experimental_propagate_custom_flag #25259

Closed
wants to merge 2 commits into from
+54 −11
Closed

Fix label conversion for --experimental_propagate_custom_flag #25259

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a83fe2b
Fix label conversion for `--experimental_propagate_custom_flag`
fmeum Feb 12, 2025
3785a49
Address review comments
fmeum Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,42 @@ public String getTypeDescription() {
}
}

/**
* Flag converter for canonicalizing a label (possibly with a "/..." suffix) and/or define by
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this mention --define (also the comment on line 245). It doesn't look like this converter is used except for with --experimental_propagate_custom_flag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The option accepts both Starlark flags and --defines.

* converting the label to ambiguous canonical form.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean "unambiguous canonical form", or is "ambiguous" correct here and I am misunderstanding?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really did mean "ambiguous" since the implementation used toString(), but I now changed this to getUnambiguousCanonicalForm() for clarity.

*/
public static class CustomFlagConverter implements Converter<String> {
@Override
public String convert(String input, Object conversionContext) throws OptionsParsingException {
if (!input.startsWith("//") && !input.startsWith("@")) {
// This is a --define flag.
return input;
}
// A "/..." suffix is not valid label syntax, so replace it with valid syntax and transform
// it back after conversion.
String invalidSubpackagesSuffix = "/...";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make these constants in the CustomFlagConverter class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made /... a constant as it has meaning, but :__subpackages__ is arbitrary and not used anywhere else, so defining a constant for it may, in my opinion, even hurt readability.

String validSubpackagesSuffix = ":__subpackages__";
String escapedUnconvertedLabel =
input.endsWith(invalidSubpackagesSuffix)
? input.substring(0, input.length() - invalidSubpackagesSuffix.length())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the substring call, use String.replace. Any instance of /... inside the label is invalid anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't //...super_hidden_folder:foo be a valid label? I can still do this, but I'm not sure it's theoretically correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I hope no-one is naming packages like that but it's technically possible, so leave this as-is.

+ validSubpackagesSuffix
: input;
String escapedConvertedLabel =
convertOptionsLabel(escapedUnconvertedLabel, conversionContext).toString();
if (escapedConvertedLabel.endsWith(validSubpackagesSuffix)) {
return escapedConvertedLabel.substring(
0, escapedConvertedLabel.length() - validSubpackagesSuffix.length())
+ invalidSubpackagesSuffix;
}
return escapedConvertedLabel;
}

@Override
public String getTypeDescription() {
return "an absolute label or define";
}
}

/** Values for the --strict_*_deps option */
public enum StrictDepsMode {
/** Silently allow referencing transitive dependencies. */
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
name = "experimental_propagate_custom_flag",
defaultValue = "null",
allowMultiple = true,
converter = CoreOptionConverters.CustomFlagConverter.class,
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
metadataTags = {OptionMetadataTag.EXPERIMENTAL},
Expand Down
13 changes: 6 additions & 7 deletions src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.common.options.Options;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -325,7 +324,9 @@ def _impl(ctx):
"//my_starlark_flag:other_starlark_flag",
"true"),
"--experimental_exclude_starlark_flags_from_exec_config=true",
"--experimental_propagate_custom_flag=//my_starlark_flag:starlark_flag");
// Verify that labels are parsed rather than compared as strings by specifying the
// label in non-canonical form.
"--experimental_propagate_custom_flag=@//my_starlark_flag:starlark_flag");
assertThat(
cfg.getOptions()
.getStarlarkOptions()
Expand All @@ -339,10 +340,8 @@ def _impl(ctx):
}

@Test
// TODO: b/377959266 - fix test setup bug that fails Bazel CI. This test's logic doesn't cause the
// bug: it's somehow caused by test naming. See bug for details.
@Ignore("b/377959266")
public void testExecStarlarkFlag_isPropagatedByTargetPattern() throws Exception {
scratch.file("my_starlark_flag/BUILD");
scratch.file(
"my_starlark_flag/rule_defs.bzl",
"""
Expand All @@ -357,7 +356,7 @@ public void testExecStarlarkFlag_isPropagatedByTargetPattern() throws Exception
load("//my_starlark_flag:rule_defs.bzl", "bool_flag")
bool_flag(
name = "include_me",
build_setting_default = "False",
build_setting_default = False,
)
""");
scratch.file(
Expand All @@ -366,7 +365,7 @@ public void testExecStarlarkFlag_isPropagatedByTargetPattern() throws Exception
load("//my_starlark_flag:rule_defs.bzl", "bool_flag")
bool_flag(
name = "exclude_me",
build_setting_default = "False",
build_setting_default = False,
)
""");

Expand Down