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

Jump to bottom

Pass --host_action_env to host options hostActionEnvironment attribute #12694

Closed
wants to merge 12 commits into from
Closed

Pass --host_action_env to host options hostActionEnvironment attribute #12694

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
aa809e5
Return execution configuration without applying any transition
mai93 Dec 14, 2020
480f5ba
Revert "Return execution configuration without applying any transition"
mai93 Dec 14, 2020
11e496b
Retry initial commit
mai93 Dec 14, 2020
59661a6
Remove java.management from jdeps_modules.golden
mai93 Dec 14, 2020
6acbd9a
Pass host_action_env to hostActionEnvironment attribute of host options
mai93 Dec 15, 2020
4fbc56d
Adding unit test for host_action_env option
mai93 Dec 16, 2020
e36479c
Return execution configuration without applying any transition
mai93 Dec 14, 2020
4f92404
Revert "Return execution configuration without applying any transition"
mai93 Dec 14, 2020
3c282ad
Retry initial commit
mai93 Dec 14, 2020
798890b
Remove java.management from jdeps_modules.golden
mai93 Dec 14, 2020
40b804d
Pass host_action_env to hostActionEnvironment attribute of host options
mai93 Dec 15, 2020
0a54610
Remove extra blank line
mai93 Dec 16, 2020
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
1 change: 0 additions & 1 deletion src/jdeps_modules.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ java.base
java.compiler
java.instrument
java.logging
java.management
java.naming
java.sql
java.xml
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionTransitionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,9 @@ public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments()

@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) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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;

// If no execution platform is known or the input already comes from execution configuration,
// don't change anything and return the existing value.
return options.underlying();
}
return cache.applyTransition(
Expand Down