Pass --host_action_env to host options hostActionEnvironment attribute #12694

mai93 · 2020-12-14T09:55:37Z

This PR passes the value of host_action_env to hostActionEnvironment attribute of the host options in CoreOptions.getHost.

Fixes: #12403

This reverts commit aa809e5.

katre · 2020-12-14T15:15:42Z

src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java

@@ -109,8 +109,9 @@ public boolean isHostTransition() {

    @Override
    public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
-      if (executionPlatform == null) {
-        // No execution platform is known, so don't change anything.
+      if (executionPlatform == null || options.get(CoreOptions.class).isExec) {


This isn't safe: it's entirely possible to need to build tools in order to build other tools. How does this lead to the error with action_env?

I thought that the error of conflicting actions may be caused by having two copies of the same execution transition. Adding this check fixed the --host_action_env conflicting actions issue, so I created this PR to just to make sure it does not break other tests.

If the two configurations were exactly the same, they would have the same hash. Can you check what the difference between the two is? I do not think this change is safe to submit.

You can diff two configurations by using the "bazel config" command, with the same flags, after you've run the command to cause the error. The config subcommand checks for configurations present in skyframe's cache, so anything that invalidates the cache will make this fail.

The only difference between the 2 configurations is com.google.devtools.build.lib.analysis.config.CoreOptions.action_env where one config includes the env var passed with host_action_env and the other doesn't.

I see that the value of hostActionEnvironment isn't being kept in CoreOptions.getHost. If you add the line, does that deal with the issue?

// Pass host action environment variables host.actionEnvironment = hostActionEnvironment; host.hostActionEnvironment = hostActionEnvironment;

katre · 2020-12-15T14:30:50Z

Can you add a test for the host_action_env issue? It looks like there aren't any tests for the flag at all, putting them in https://cs.opensource.google/bazel/bazel/+/master:src/test/shell/integration/action_env_test.sh?ss=bazel&q=action_env would probably be appropriate.

katre · 2020-12-16T14:15:17Z

.bazelci/postsubmit.yml

-        -e 's/^# android_ndk_repository/android_ndk_repository/'
-        -e 's/^# rbe_autoconfig/rbe_autoconfig/'
-        -e 's/^# load("@bazel_toolchains/load("@bazel_toolchains/' WORKSPACE
+      - sed -i.bak


Why are there changes in the postsubmit and presubmit? We should get these reviewed by someone on who knows them better than I do.

I included these commits to fix the failing presubmit jdeps_test as mentioned in #12685

They should be submitted (and reviewed) separately, since it's not directly part of fixing this issue.

They are already submitted, rebasing the pull request should fix this, right?

Oh, I see. Yes, rebase onto master and that should be fine.

google-cla · 2020-12-16T16:20:28Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

This reverts commit aa809e5.

katre

One small nit, but otherwise this looks great. Thanks for all the work on this.

katre · 2020-12-16T17:28:33Z

src/test/shell/integration/action_env_test.sh

+
+  cat `bazel info ${PRODUCT_NAME}-genfiles`/pkg/successive_exec_tools_env.txt > $TEST_log
+  expect_log "FOO=bar"
+


Remove this extra blank line.

This PR passes the value of `host_action_env` to `hostActionEnvironment` attribute of the host options in `CoreOptions.getHost`. Fixes: #12403 Closes #12694. PiperOrigin-RevId: 348011332

This PR passes the value of `host_action_env` to `hostActionEnvironment` attribute of the host options in `CoreOptions.getHost`. Fixes: bazelbuild#12403 Closes bazelbuild#12694. PiperOrigin-RevId: 348011332

Return execution configuration without applying any transition

aa809e5

google-cla bot added the cla: yes label Dec 14, 2020

mai93 added 2 commits December 14, 2020 13:50

Revert "Return execution configuration without applying any transition"

480f5ba

This reverts commit aa809e5.

Retry initial commit

11e496b

mai93 force-pushed the conflicting_actions branch from 395a1c6 to 11e496b Compare December 14, 2020 14:26

Remove java.management from jdeps_modules.golden

59661a6

katre reviewed Dec 14, 2020

View reviewed changes

Pass host_action_env to hostActionEnvironment attribute of host options

6acbd9a

mai93 changed the title ~~Skip transition for input options coming from execution configuration~~ Pass --host_action_env to host options hostActionEnvironment attribute Dec 15, 2020

Adding unit test for host_action_env option

4fbc56d

katre reviewed Dec 16, 2020

View reviewed changes

mai93 requested review from aiuto, alandonovan, brandjon, floriographygoth, gregestren, jin, lberki and oquenchil as code owners December 16, 2020 16:20

google-cla bot added cla: no and removed cla: yes labels Dec 16, 2020

mai93 added 5 commits December 16, 2020 19:18

Return execution configuration without applying any transition

e36479c

Revert "Return execution configuration without applying any transition"

4f92404

This reverts commit aa809e5.

Retry initial commit

3c282ad

Remove java.management from jdeps_modules.golden

798890b

Pass host_action_env to hostActionEnvironment attribute of host options

40b804d

mai93 force-pushed the conflicting_actions branch from d3d1b00 to 40b804d Compare December 16, 2020 17:22

google-cla bot removed the cla: no label Dec 16, 2020

google-cla bot added the cla: yes label Dec 16, 2020

katre approved these changes Dec 16, 2020

View reviewed changes

Remove extra blank line

0a54610

mai93 removed request for jin, brandjon, aiuto, lberki, gregestren, alandonovan, oquenchil and floriographygoth December 16, 2020 17:39

bazel-io closed this in e667082 Dec 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass --host_action_env to host options hostActionEnvironment attribute #12694

Pass --host_action_env to host options hostActionEnvironment attribute #12694

mai93 commented Dec 14, 2020 •

edited

Loading

katre Dec 14, 2020

mai93 Dec 14, 2020

katre Dec 14, 2020

mai93 Dec 15, 2020

katre Dec 15, 2020

katre commented Dec 15, 2020

katre Dec 16, 2020

mai93 Dec 16, 2020

katre Dec 16, 2020

mai93 Dec 16, 2020

katre Dec 16, 2020

google-cla bot commented Dec 16, 2020

katre left a comment

katre Dec 16, 2020


		cat `bazel info ${PRODUCT_NAME}-genfiles`/pkg/successive_exec_tools_env.txt > $TEST_log
		expect_log "FOO=bar"

Pass --host_action_env to host options hostActionEnvironment attribute #12694

Pass --host_action_env to host options hostActionEnvironment attribute #12694

Conversation

mai93 commented Dec 14, 2020 • 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

Choose a reason for hiding this comment

katre commented Dec 15, 2020

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

google-cla bot commented Dec 16, 2020

katre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mai93 commented Dec 14, 2020 •

edited

Loading