Skip to content

Let aquery print effective environment for all CommandActions #17108

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
Closed
fmeum wants to merge 1 commit into from
Closed

Let aquery print effective environment for all CommandActions #17108

fmeum wants to merge 1 commit into from

Conversation

@fmeum
Copy link
Collaborator

@fmeum fmeum commented Jan 2, 2023 •
edited
Loading

Instead of printing the fixed part of the ActionEnvironment of all SpawnActions, aquery now prints the effective environment resolved against an empty client environment of all AbstractActions that implement CommandAction.

For SpawnActions, this should not result in any observable change, but C++ actions, which only override AbstractAction#getEffectiveEnvironment, now have their fixed environments (e.g. toolchain env vars such as INCLUDE with MSVC) emitted.

Work towards #12852

@fmeum fmeum force-pushed the 12852-toolchain-env-variables branch 3 times, most recently from bf14b1a to bacf1b6 Compare January 2, 2023 11:20
@fmeum
Let aquery print effective environment for all CommandActions
04719c3 
Instead of printing the fixed part of the `ActionEnvironment` of all
`SpawnActions`, `aquery` now prints the effective environment resolved
against an empty client environment of all `AbstractAction`s that
implement `CommandAction`.

For `SpawnAction`s, this should not result in any observable change, but
C++ actions, which only override
`AbstractAction#getEffectiveEnvironment`, now have their fixed
environments (e.g. toolchain env vars such as `INCLUDE` with MSVC)
emitted.
@fmeum fmeum force-pushed the 12852-toolchain-env-variables branch from bacf1b6 to 04719c3 Compare January 2, 2023 12:01
@fmeum fmeum changed the title WIP: Let aquery print environment for all AbstractActions Let aquery print effective environment for all CommandActions Jan 2, 2023
@fmeum fmeum marked this pull request as ready for review January 2, 2023 12:24
@fmeum
Copy link
Collaborator Author

fmeum commented Jan 2, 2023

@meisterT Could you review this PR?

@cpsauer Could you check that this fixes the aquery problem? I don't think it would affect extra actions, but then I also know almost nothing about them. Also don't expect DEVELOPER_DIR to show up as that is not available on the action but only on the spawn level.

@sgowroji sgowroji added team-Performance Issues for Performance teams awaiting-review PR is awaiting review from an assigned reviewer labels Jan 3, 2023
@cpsauer
Copy link
Contributor

cpsauer commented Jan 3, 2023

Thanks for diving in @fmeum! Looks to be a good improvement to me.

[I don't have much experience in this part of the codebase, but radiating ideas: Might it make sense to pass through the actual client environment if it might influence the execution environment? Certainly that shouldn't block merging or anything, but it strikes me that people probably run this trying to understand how the build maps to commands on their specific machine/environment. That's true of the tool I maintain that uses this at least.]

@fmeum
Copy link
Collaborator Author

fmeum commented Jan 3, 2023 •
edited
Loading

@cpsauer Making the output of aquery dependent on the host environment is a kind of change that definitely needs more thought. It's possible that adding new fields for inherited variables is better - any kind of external tooling consuming aquery output could just add the values of the environment itself, but the output would remain host-independent.

The Xcode variables receive special handling by Bazel that is more or less equivalent to (but more efficient than) running xcode-locator as part of the action. Since with this change all environment variables that go into xcode-locator are available from aquery output, external tooling could run xcode-locator itself to determine the missing values.

@cpsauer
Copy link
Contributor

cpsauer commented Jan 4, 2023

Seems like it already is--inferred platform, environment specifying compiler, etc., right?--but you guys are certainly the experts.

Definitely don't want this side discussion to block this improvement, though.

@meisterT meisterT added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 10, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Jan 10, 2023

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jan 10, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Jan 10, 2023

Seems like it already is--inferred platform, environment specifying compiler, etc., right?--but you guys are certainly the experts.

Definitely don't want this side discussion to block this improvement, though.

All these things are deterministic functions of the outputs of repository rules, which you can either circumvent by specifying values for command-line flags such --platforms or at least enumerate and thus get into a hermetic state with just a bit of work. Depending on the entire client environment would make that next to impossible.

@cpsauer
Copy link
Contributor

cpsauer commented Jan 10, 2023

Sweet. Thanks again, guys.

cpsauer added a commit to hedronvision/bazel-compile-commands-extractor that referenced this pull request Jan 10, 2023
@cpsauer
Add notes about bazelbuild/bazel#17108
1266d6a
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 11, 2023
@fmeum fmeum deleted the 12852-toolchain-env-variables branch January 11, 2023 10:27
@ShreeM01
Copy link
Contributor

ShreeM01 commented Jan 19, 2023

@bazel-io fork 6.1.0

@bazel-io bazel-io mentioned this pull request Jan 19, 2023
Closed
@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jan 19, 2023
@ShreeM01 ShreeM01 mentioned this pull request Jan 19, 2023
Merged
@ShreeM01
Copy link
Contributor

ShreeM01 commented Jan 26, 2023

Hi @fmeum! We are trying to cherry-pick the changes to release 6.1.0. But pre-submits checks are failing ([https://github.com//pull/17274]) . So could you please help us in cherry-picking this changes to release 6.1.0?

hvadehra pushed a commit that referenced this pull request Feb 14, 2023
@fmeum @hvadehra
Let aquery print effective environment for all CommandActions
7b11d6e 
Instead of printing the fixed part of the `ActionEnvironment` of all `SpawnActions`, `aquery` now prints the effective environment resolved against an empty client environment of all `AbstractAction`s that implement `CommandAction`.

For `SpawnAction`s, this should not result in any observable change, but C++ actions, which only override `AbstractAction#getEffectiveEnvironment`, now have their fixed environments (e.g. toolchain env vars such as `INCLUDE` with MSVC) emitted.

Work towards #12852

Closes #17108.

PiperOrigin-RevId: 501198850
Change-Id: I035a8cfde32193ca7f1f71030bd183c20edfdc0d
ShreeM01 added a commit that referenced this pull request Feb 21, 2023
@ShreeM01 @fmeum
[6.1.0]Let aquery print effective environment for all `CommandActio…
3ab8a0a 
…n`s (#17274)

* Let `aquery` print effective environment for all `CommandAction`s

Instead of printing the fixed part of the `ActionEnvironment` of all `SpawnActions`, `aquery` now prints the effective environment resolved against an empty client environment of all `AbstractAction`s that implement `CommandAction`.

For `SpawnAction`s, this should not result in any observable change, but C++ actions, which only override `AbstractAction#getEffectiveEnvironment`, now have their fixed environments (e.g. toolchain env vars such as `INCLUDE` with MSVC) emitted.

Work towards #12852

Closes #17108.

PiperOrigin-RevId: 501198850
Change-Id: I035a8cfde32193ca7f1f71030bd183c20edfdc0d

* Removed the function test_does_not_fail_horribly_with_file()

---------

Co-authored-by: Fabian Meumertzheim <[email protected]>
@cpsauer cpsauer mentioned this pull request Jan 2, 2024
Closed
gitrusmahone added a commit to gitrusmahone/bazel-compile-commands-extractor that referenced this pull request Aug 17, 2024
@gitrusmahone @cpsauer
Add notes about bazelbuild/bazel#17108
60c123c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@meisterT meisterT meisterT approved these changes

Assignees

No one assigned

Labels

team-Performance Issues for Performance teams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fmeum @cpsauer @ShreeM01 @meisterT @bazel-io @sgowroji