bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
22.99k stars 4.03k forks source link

System-specific environment variables on Windows pollute remote cache #3500

Open sebjmxn opened 7 years ago

sebjmxn commented 7 years ago

Description of the problem / feature request / question:

When building C++ rules on Windows with MSVC, system-specific environment variables are part of the inputs, i.e.

SET TEMP=C:\Users\<my_user>\AppData\Local\Temp
SET TMP=C:\Users\<my_user>\AppData\Local\Temp
SET PATH="...;C:\Program Files (x86)\Skype\Phone\;C:\Program Files (x86)\Bitvise SSH Client;..."

For remote caching, this prevents sharing outputs between different systems.

Environment info

Have you found anything relevant by searching the web?

https://groups.google.com/forum/#!topic/bazel-discuss/4GV8lJzfeGQ

ulfjack commented 7 years ago

I consider this a dupe of #2574. Can you try setting --experimental_strict_action_env? It may not work yet if you have actions that try to write to TMP, which defaults to c:\windows, which is not user-writable. We'd like to hear back whether or not it works.

laszlocsomor commented 7 years ago

This affects other platforms too, not just Windows.

Bazel copies envvars from the client environment to the action's environment, and if those envvars are different for each user, there's no chance for cross-user caching.

The right solution is to set user-independent values for these somewhere in the execution machinery, as part of setting up the environment just before executing a spawn.

sebjmxn commented 7 years ago

Note that I tried bazel build //examples/cpp:hello-world --experimental_strict_action_env -s, but the console output produced by the -s flag did not change. It still contains the full PATH and TMP, TEMP. Is that to be expected?

I have yet to test the impact on the actual caching, I will post the results later.

(E: Still running release 0.5.4rc1)

ulfjack commented 7 years ago

Can you paste the console output you're seeing?

sebjmxn commented 7 years ago

Yes. See the attached file. bazel_output.txt

ulfjack commented 7 years ago

I don't see any of these env variables on MacOS. I wonder if the ./tools/cpp/windows_cc_configure.bzl is setting them?

ulfjack commented 7 years ago

Reopening - there might be an additional Windows-specific issue here.

ulfjack commented 7 years ago

Without --experimental_strict_action_env:

    APPLE_SDK_PLATFORM=MacOSX \
    APPLE_SDK_VERSION_OVERRIDE=10.12 \
    PATH=/Users/ulfjack/bin:/Users/ulfjack/homebrew/bin:/Users/ulfjack/bin:/usr/local/git/current/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin \
    TMPDIR=/var/folders/zl/pndhy4k906s372ln2spnnrnw002vrk/T/ \
    XCODE_VERSION_OVERRIDE=8.3.3 \

With --experimental_strict_action_env:

    APPLE_SDK_PLATFORM=MacOSX \
    APPLE_SDK_VERSION_OVERRIDE=10.12 \
    PATH=/bin:/usr/bin \
    XCODE_VERSION_OVERRIDE=8.3.3 \
ulfjack commented 7 years ago

@laszlocsomor , can you reproduce this on Windows?

ulfjack commented 7 years ago
$ bazel build //src/main/cpp/... --experimental_strict_action_env=1 -s
dslomov commented 7 years ago

we do set INCLUDE and LIB for C++ toolchain, to help linker locate system libraries, such as kernel32.lib and such. That is sure unhappy, we need a better solution for those /cc @meteorcloudy

meteorcloudy commented 7 years ago

@ulfjack You are right. It is indeed windows_cc_configure.bzl who set PATH, INCLUDE, LIB, TEMP and TMP environment variables. See https://github.com/bazelbuild/bazel/blob/master/tools/cpp/CROSSTOOL.tpl#L260 and https://github.com/bazelbuild/bazel/blob/master/tools/cpp/windows_cc_configure.bzl#L369

We calculate these env value by running a batch script.

I think the first step to solve this problem is by cleaning PATH, INCLUDE, LIB before running the script, so that irrelevant system paths like C:\Program Files (x86)\Skype\Phone\; don't get leaked.

But we still need to figure out what to do if MSVC is installed in different places.

laszlocsomor commented 5 years ago

I suppose this is still a problem with Bazel 0.23. @meteorcloudy , is that correct?

meteorcloudy commented 5 years ago

Yes, I think so

kkpattern commented 2 years ago

Looks lik bazel 4.2.2 still has this problem. The PATH is ok as long as all the team members install MSVC in the same path. But the TEMP and TMP are trickier. Force all the members have a same TEMP path doesn't seem a good idea. Any chance this can be fixed in the future?

kkpattern commented 2 years ago

Another problem we encounter is case-senstive. On Windows, path is not case-senstive. So C:\Windows\xxx is equal to C:\WINDOWS\xxx. However bazel will treat these two differently, cause remote cache miss. Maybe we can convert PATH to lower case? This sounds a different issue.