propagated for cc_library and cc_binary #9267

ishikhman · 2019-08-28T11:42:34Z

Part of #8830

RELNOTES[NEW]: tags: use --experimental_allow_tags_propagation flag to propagate tags to the action's execution requirements from cc_library or cc_binary targets. Such tags should start with: no-, requires-, supports-, block-, disable-, cpu:. See #8830 for details.

buchgr

Overall LGTM. Thanks!

buchgr · 2019-08-29T09:26:56Z

src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java

+      return this.getAnalysisEnvironment().getSkylarkSemantics().experimentalAllowTagsPropagation();
+    } catch (InterruptedException e) {
+      // fallback to false if value of experimentalAllowTagsPropagation cannot be fetched
+      return false;


InterruptedException does not mean that the value can't be fetched but that something (i.e. ctrl+c) told it to stop fetching. I think the proper thing to do here is to add a throws InterruptedException.

makes sense. Fixed.

buchgr · 2019-08-29T09:27:43Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

 import javax.annotation.Nullable;
+import java.util.*;


for future: add the google-java-style in your IDE.

buchgr · 2019-08-29T09:29:26Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

+   *        actions' execution requirements, for more details {@see
+   *        SkylarkSematicOptions#experimentalAllowTagsPropagation}
+   */
+  public static Map<String, String> getExecutionInfo(Rule rule, boolean allowTagsPropagation) {


this method is only called in three different sides. Remove it and instead use ternary operator at the call side?

buchgr · 2019-08-29T09:31:03Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java

@@ -1548,6 +1554,9 @@ private CppCompileActionBuilder initializeCompileAction(Artifact sourceArtifact)
    builder.setCcCompilationContext(ccCompilationContext);
    builder.setCoptsFilter(coptsFilter);
    builder.setFeatureConfiguration(featureConfiguration);
+    if (ruleContext != null) {


if (ruleContext != null && ruleContext.isAllowTagsPropagation()) {
builder.addExecutionInfo(TargetUtils.getExecutionInfo(ruleContext.getRule()))
}

buchgr · 2019-08-29T09:31:41Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingHelper.java

@@ -141,19 +143,21 @@ public CcLinkingOutputs getCcLinkingOutputs() {
   * @param ccToolchain the C++ toolchain provider for the build
   * @param fdoContext the C++ FDO optimization support provider for the build
   * @param configuration the configuration that gives the directory of output artifacts
+   * @param ruleContext


add a comment here explaining ruleContext

buchgr · 2019-08-29T09:33:30Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java

@@ -148,6 +148,7 @@ public Artifact create(
  //  of them.
  private boolean isTestOrTestOnlyTarget;
  private boolean isStampingEnabled;
+  private Map<String, String> executionInfo = new LinkedHashMap<>();


can this be final?

Nit: Use ImmutableMap.builder()

buchgr · 2019-08-29T09:34:35Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java

@@ -1022,15 +1023,15 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
    // If the crosstool uses action_configs to configure cc compilation, collect execution info
    // from there, otherwise, use no execution info.
    // TODO(b/27903698): Assert that the crosstool has an action_config for this action.
-    Map<String, String> executionRequirements = new LinkedHashMap<>();
+//    Map<String, String> executionRequirements = new LinkedHashMap<>();


buchgr · 2019-08-29T09:37:48Z

src/test/shell/bazel/BUILD

@@ -767,6 +767,14 @@ sh_test(
    tags = ["no_windows"],
 )

+sh_test(
+    name = "tags_propagation_native_test",


@hlopko should this test be part of the bazel_rules_cc_test.sh?

I actually think we don't need an integration test for this, wdyt about adding an unit test to CcLibraryConfiguredTargetTest? We're testing analysis-only behavior anyway. You'll assert that the action in constructed as expected.

(and also a test to CcBinaryConfiguredTargetTest, GenruleConfiguredTargetTest...)

hm, we could do that. I just prefer integration tests in this case - to make sure that in 'real' env it actually works. I also find them more readable here :)

Well, you find them more readable because you didn't spend much time in the area :) We actually like our BuildViewTestCases more becasue they allow us to inspect the state of the world more easily. Instead of comparing handwritten line of text that aquery outputs you can have assertThat(executionInfo).containsExactly(...).

Plus the integration test will have exactly same coverage as the BuildViewTestCase. And you wouldn't have to deal with platform specific tags such as requires-darwin.

buchgr · 2019-08-29T09:38:52Z

src/test/shell/bazel/tags_propagation_native_test.sh

+      || fail "should have generated output successfully"
+
+  if [[  "${PLATFORM}" = "darwin"  ]]; then
+    assert_contains "ExecutionInfo: {local: '', no-cache: '', no-remote: '', requires-darwin: ''}" output1


Why is 'requires-darwin' relevant to this change?

because if you run a test on iOS platform, then testRunner (or smth like that) will add "requires-darwin" to the actions' execution requirements.

buchgr · 2019-08-29T09:40:56Z

src/test/shell/bazel/tags_propagation_native_test.sh

+  if [[  "${PLATFORM}" = "darwin"  ]]; then
+    assert_contains "ExecutionInfo: {local: '', no-cache: '', no-remote: '', requires-darwin: ''}" output1
+  else
+    assert_contains "ExecutionInfo: {local: '', no-cache: '', no-remote: ''}" output1


I think it would be nicer if the test didn't depend on the order. Consider

assert_contains "local: ''"
assert_contains "no-cache: ''"
assert_contains "no-remote: ''"

hlopko

Thanks for working on this! It really looks like this is the reasonable way forward!

hlopko · 2019-08-29T10:22:56Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

+    if (allowTagsPropagation){
+      return getExecutionInfo(rule);
+    } else {
+      return Collections.emptyMap();


suuuper nit: ImmutableMap.of() and a change in signature to return ImmutableMap. Or inline as Jakob suggested.

removed the method :)

hlopko · 2019-08-29T10:26:45Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java

@@ -352,7 +352,7 @@ public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleCont
                semantics,
                featureConfiguration,
                ccToolchain,
-                fdoContext)
+                fdoContext, ruleContext)


We try hard not to pass RuleContext into CcCompilationHelper and CcLinkingHelper because of its huge powers (e.g. it allows you to access rule attributes which is something we don't want to allow in our helpers to keep our code layering clean). Can we instead use ActionConstructionContext that is already passed to CcLinkingInfo? If not, let's pass executionInfo, not rule context, into the constructor of the Helper?

hm, it might work!
I've replaced RuleContext directly with a Rule and pushed isAllowTagsPropagation up to the ActionConstructionContext, @hlopko ptal?

hlopko · 2019-08-29T10:27:13Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java

+          CcToolchainProvider ccToolchain,
+          FdoContext fdoContext,
+          BuildConfiguration buildConfiguration,
+          @Nullable RuleContext ruleContext) {


Why nullable?

Because it can be null in case of sandwich, like we understood it with Jakob yesterday. See: https://github.com/bazelbuild/bazel/pull/9267/files/49ca1c4f3364a34a398a03b39b29a83552ce1e69#r318975035

I'll also have a look at ActionConstructionContext, maybe we could use that => this @nullable will not be needed at all, as well as RuleContext

hlopko · 2019-08-29T10:32:22Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java

@@ -1432,7 +1432,7 @@ public boolean isCcToolchainResolutionEnabled(SkylarkRuleContext skylarkRuleCont
                    .getActionConstructionContext()
                    .getConfiguration()
                    .getFragment(CppConfiguration.class),
-                ((BazelStarlarkContext) starlarkContext).getSymbolGenerator())
+                ((BazelStarlarkContext) starlarkContext).getSymbolGenerator(), null)


Why is null here ok? You can get to action construction context using actions.getActionConstructionContext() there.

hlopko · 2019-08-29T10:33:18Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java

@@ -148,6 +148,7 @@ public Artifact create(
  //  of them.
  private boolean isTestOrTestOnlyTarget;
  private boolean isStampingEnabled;
+  private Map<String, String> executionInfo = new LinkedHashMap<>();


Nit: Use ImmutableMap.builder()

hlopko · 2019-08-29T10:37:12Z

src/test/shell/bazel/BUILD

@@ -767,6 +767,14 @@ sh_test(
    tags = ["no_windows"],
 )

+sh_test(
+    name = "tags_propagation_native_test",


I actually think we don't need an integration test for this, wdyt about adding an unit test to CcLibraryConfiguredTargetTest? We're testing analysis-only behavior anyway. You'll assert that the action in constructed as expected.

hlopko · 2019-08-29T10:38:22Z

src/test/shell/bazel/BUILD

@@ -767,6 +767,14 @@ sh_test(
    tags = ["no_windows"],
 )

+sh_test(
+    name = "tags_propagation_native_test",


(and also a test to CcBinaryConfiguredTargetTest, GenruleConfiguredTargetTest...)

ishikhman · 2019-08-29T11:49:01Z

@hlopko ok, I'll have a look at the tests and BuildViewTestCases :)

What do you think about changes from RuleContext -> Rule and ActionCreationContext?

googlebot added the cla: yes label Aug 28, 2019

propagated for cc_library and cc_binary

eba7a48

ishikhman force-pushed the tags-cc branch from 1c52c10 to eba7a48 Compare August 28, 2019 11:43

ishikhman added 4 commits August 28, 2019 14:01

propagated for CcLinkAction

968880d

added flag

309e728

small fixes

222b4fd

small fixes

fde3196

ishikhman marked this pull request as ready for review August 28, 2019 16:08

ishikhman requested a review from hlopko as a code owner August 28, 2019 16:08

ishikhman requested a review from buchgr August 28, 2019 16:08

renamed flag and a few tests

49ca1c4

buchgr reviewed Aug 29, 2019

View reviewed changes

ishikhman mentioned this pull request Aug 29, 2019

Tags propagation for java rules #9274

Closed

hlopko reviewed Aug 29, 2019

View reviewed changes

ishikhman added 3 commits August 29, 2019 12:47

review fixes

48f6192

removed RuleContext dependency from Cc...Helper

08cbf06

nit

5cbfec0

ishikhman force-pushed the tags-cc branch from e6375dd to 5cbfec0 Compare August 29, 2019 11:18

ishikhman requested a review from hlopko August 29, 2019 11:36

iirina added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Aug 29, 2019

refactored to reduce dependencies on RuleContext

3a00a3e

bazel-io closed this in 94c24b3 Aug 30, 2019

ishikhman mentioned this pull request Sep 4, 2019

Tags native #8978

Closed

propagated for cc_library and cc_binary #9267

propagated for cc_library and cc_binary #9267

Conversation

ishikhman commented Aug 28, 2019 • edited Loading

buchgr 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlopko Aug 29, 2019 • edited Loading

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

hlopko 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

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

ishikhman commented Aug 29, 2019

ishikhman commented Aug 28, 2019 •

edited

Loading

hlopko Aug 29, 2019 •

edited

Loading