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.

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

SDKROOT, DEVELOPER_DIR, INCLUDE, LIB missing from Environment Variables in Aquery and Extra Actions #12852

Open
cpsauer opened this issue Jan 19, 2021 · 14 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) platform: apple team-Rules-CPP Issues for C++ rules team-Rules-ObjC Issues for Objective-C maintainers type: bug

Comments

@cpsauer
Copy link
Contributor

cpsauer commented Jan 19, 2021

Description of the problem:

When listening to CppCompile and ObjcCompile (mnemonic) actions on macOS and Windows, the key environment variables DEVELOPER_DIR and SDKROOT (Apple Platforms) and INCLUDE and LIB and other essentials (Windows) are missing from the environment variables in aquery and extra actions output, despite being needed to run the action successfully.

For Apple platforms, the bazel-supplied, wrapped Xcode compiler is dependent on these environment variables, and crashes if they aren't provided. For Windows, MSVC is dependent on these variables for system headers and libraries. Those environment variables provide key information about the compilation command.

On Apple platforms, some other, less-standard variables are supplied: XCODE_VERSION_OVERRIDE, APPLE_SDK_VERSION_OVERRIDE, and APPLE_SDK_PLATFORM. On Windows...not so much.

What underlying problem are you trying to solve?

The lack of these variables significantly complicates building C++ developer tooling around Bazel.

In my case, I'm integrating Bazel with clangd autocomplete in a wide variety of editors, to make development using Bazel more fun. We're linked to from the bazel docs :)

But I'd imagine this would also be useful for Kythe--the complication databases it generates would be incomplete without this, and they use action_listeners/extra_action, while we use aquery. As above, the problem exists in both.

The workaround I'm currently using on macOS using is to go direct to the system, re-inferring SDKROOT and DEVELOPER_DIR from the system Xcode commands. On Windows, I inspect the top level cc_toolchain. While these generally work, they'll miss any configuration the user does in the build graph.

More generally, it seems odd that those key variables are missing for the core cc rules. It feels like they'd be the primary purpose of being able to query the environment of C++ actions. It feels like there might be something even more general broken here.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Just run bazel aquery "mnemonic('(Objc|Cpp)Compile',//...)" on a cc_library on macOS or Windows and look at the (lack of) those environment variables.

Reproing the extra action issue is more involved, and probably is just another manifestation of the same underlying problem. But I think the fastest way to do that would be to grab Kythe's implementation of an extractor (https://github.com/kythe/kythe/blob/master/kythe/cxx/extractor/README.md) and examine the extra_actions_base_proto.

@cpsauer
Copy link
Contributor Author

cpsauer commented Feb 11, 2021

As an add-on: Seems like all these variables are missing from aquery output, too.

@oquenchil oquenchil added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed untriaged labels Feb 11, 2021
@cpsauer cpsauer changed the title SDKROOT and DEVELOPER_DIR missing from Environment Variables in Extra Actions Proto SDKROOT, DEVELOPER_DIR, INCLUDE, LIB missing from Environment Variables in Aquery and Extra Actions Apr 6, 2022
@cpsauer
Copy link
Contributor Author

cpsauer commented Apr 6, 2022

It seems like maybe there's something much more general broken with environment variable reporting for actions.

I ran back into an equivalent problem on Windows and with aquery, and it really does complicate and embrittle tooling.

I've updated the original report to reflect that.

@gregestren @oquenchil, might this merit different labels/priorities if its a more general, non-apple-specific, non-extra-action-specific bug?

@gregestren
Copy link
Contributor

I don't have expertise on this part of the code base (aquery, extra actions, environment variable consumption). I'm going to defer to @oquenchil .

@sualehasif
Copy link

This is blocking us too. Is there any meaningful progress here?

@cpsauer
Copy link
Contributor Author

cpsauer commented Jul 1, 2022

(Sadly not that I know of.) But I'm curious what you're building on top of aquery/extra_actions, @sualehasif!

@cpsauer
Copy link
Contributor Author

cpsauer commented Nov 12, 2022

I think this may be an even more general bug where toolchain environment variables are missing from aquery output.

(I've been trying to help a user over at hedronvision/bazel-compile-commands-extractor#83 (comment) and noticing that EM_BIN_PATH and the other emscripten environment variables are missing from aquery output, despite being set in the toolchain)

@oquenchil, any chance of a higher prioritization, given it's now clear it's a more general issue (not platform-specific), present across the action querying mechanisms (not just extra action), and causing problems for others, too?

@oquenchil
Copy link
Contributor

oquenchil commented Nov 15, 2022

Is this just for C++ rules and it works for others like Java?

@cpsauer
Copy link
Contributor Author

cpsauer commented Nov 16, 2022

I'm pretty sure it also affects Objective-C, at least, but I'm not sure whether it affects other language toolchains/Java.
[It's been a problem everywhere I've looked, but I haven't needed to dive into the Bazel toolchains for other languages just yet.]

@oquenchil oquenchil added team-Rules-ObjC Issues for Objective-C maintainers team-Rules-CPP Issues for C++ rules and removed team-Rules-CPP Issues for C++ rules labels Nov 22, 2022
@0xf005ba11
Copy link

This is complicating tooling for me as well. I've implemented a toolchain to use the EWDK to build windows drivers, and it is a real bummer to not be able to query the INCLUDE environment variable for this.

@oquenchil
Copy link
Contributor

@meisterT is this something that aquery could get to?

@fmeum
Copy link
Collaborator

fmeum commented Jan 2, 2023

Regarding DEVELOPER_DIR and SDKROOT: I suspect that these are missing from aquery output as the real values for these variable are only determined at execution time by running xcode-locator and are then injected into spawns that need them (see https://cs.opensource.google/bazel/bazel/+/abe26b84df03eac149f731f1e71fd9a5f0138f0a:src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java;l=68). Ultimately that seems to be necessary as the values have to be host-dependent absolute paths and thus shouldn't be part of the action key.

@fmeum
Copy link
Collaborator

fmeum commented Jan 2, 2023

Regarding the missing toolchain variables on all OSes: Based on a cursory glance, it looks to me as if aquery only emits environment variables for SpawnActions: https://cs.opensource.google/bazel/bazel/+/abe26b84df03eac149f731f1e71fd9a5f0138f0a:src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java;l=177. But the C++ compilation and linking actions aren't SpawnActions. Maybe that if could just check for AbstractAction instead?

@cpsauer
Copy link
Contributor Author

cpsauer commented Jan 3, 2023

Thanks so much for diving in, guys.

copybara-service bot pushed a commit that referenced this issue Jan 11, 2023
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
hvadehra pushed a commit that referenced this issue Feb 14, 2023
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 issue Feb 21, 2023
…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
Copy link
Contributor Author

cpsauer commented Jan 2, 2024

Can confirm that @fmeum's good work in #17108 surfaced the emscripten environment variables--but the Apple (and I presume the Windows) variables from the original issue are still missing.

Still think this might be worth fixing (at least by starlarkificiation time).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) platform: apple team-Rules-CPP Issues for C++ rules team-Rules-ObjC Issues for Objective-C maintainers type: bug
Projects
None yet
Development

No branches or pull requests

7 participants