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

Revert renaming of tools to exec_tools #43156

Merged
merged 1 commit into from Nov 5, 2020

Conversation

Flamefire
Copy link
Contributor

Reverts part of f827c02 as that causes trouble due to action_env variables not passed through to any dependent build which breaks builds using TF_SYSTEM_LIBS

Fixes #43019

Reverts part of f827c02 as that causes
trouble due to action_env variables not passed through to any dependent
build which breaks builds using TF_SYSTEM_LIBS
@google-ml-butler google-ml-butler bot added the size:XS CL Change Size: Extra Small label Sep 11, 2020
@gbaned gbaned self-assigned this Sep 11, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Sep 11, 2020
@mihaimaruseac
Copy link
Collaborator

Let's hold on this one, Bazel folks are investigating at the moment whether we need to use tools or exec_tools.

@Flamefire
Copy link
Contributor Author

Sure. Just to highlight what I found:

  • Using tools instead makes action_env be passed to builds triggered further down the dependency chain, with exec_tools they are not
  • Some code which was affected by the tools->exec_tools renaming in 2.3 has since been changed to (IIRC) use _local_genrule. I'm unsure how to verify that action_envs are passed through that.
  • There was some discussion in Bazel about introducing host_env variables similar to action_env. I'm not sure I could follow that distinction as e.g. for us the whole build is executed on the local machine, so in that case those 2 (if there were 2) should be equal. At best by default
  • It is absolutely essential to have a way to pass variables like CPATH, LIBRARY_PATH and the like (not speaking of PATH and LD_LIBRARY_PATH which seem be to passed through already) to all invocations without exception or stuff breaks.

Hope that helps in resolving this. In the meantime this patch is used by us to build TF on our HPC systems

@gbaned gbaned added the awaiting review Pull request awaiting review label Sep 23, 2020
@gbaned
Copy link
Contributor

gbaned commented Oct 6, 2020

@mihaimaruseac Any update on this PR? Please. Thanks!

@mihaimaruseac
Copy link
Collaborator

No update yet from the Bazel team

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Oct 8, 2020
@gbaned gbaned added the stat:awaiting tensorflower Status - Awaiting response from tensorflower label Oct 14, 2020
@Flamefire
Copy link
Contributor Author

Any update yet? As for

whether we need to use tools or exec_tools

For the build using system protobuf using exec_tools does not work but tools does. For the non-system protobuf either work. So IMO this is a step forward and discussion which should be used could be done in a follow-up issue and likely depends on bugfix or feature addition on the Bazel side because at the current state exec_tools cannot be the correct choice if it breaks builds.

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Nov 3, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Nov 3, 2020
@mihaimaruseac
Copy link
Collaborator

Approving though it might be reverted if things fail

@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 3, 2020
@gbaned gbaned added ready to pull PR ready for merge process and removed stat:awaiting tensorflower Status - Awaiting response from tensorflower ready to pull PR ready for merge process labels Nov 4, 2020
@copybara-service copybara-service bot merged commit 6d9e088 into tensorflow:master Nov 5, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Nov 5, 2020
@Flamefire Flamefire deleted the action_env_tools_arg branch November 5, 2020 13:26
Flamefire added a commit to Flamefire/tensorflow that referenced this pull request Nov 16, 2020
Follow up to tensorflow#43156
Based on
bazelbuild/bazel#12059 (comment)
exec_tools might no longer be needed and hence can be replaced by tools.
This fixes various build failures caused by missing environment
variables in environments where they are required, e.g. using custom
compilers.
geetachavan1 pushed a commit to geetachavan1/tensorflow that referenced this pull request Nov 23, 2020
Follow up to tensorflow#43156
Based on
bazelbuild/bazel#12059 (comment)
exec_tools might no longer be needed and hence can be replaced by tools.
This fixes various build failures caused by missing environment
variables in environments where they are required, e.g. using custom
compilers.
geetachavan1 pushed a commit to geetachavan1/tensorflow that referenced this pull request Nov 23, 2020
Follow up to tensorflow#43156
Based on
bazelbuild/bazel#12059 (comment)
exec_tools might no longer be needed and hence can be replaced by tools.
This fixes various build failures caused by missing environment
variables in environments where they are required, e.g. using custom
compilers.
serach24 pushed a commit to serach24/Arbitor that referenced this pull request Jun 5, 2021
Follow up to tensorflow#43156
Based on
bazelbuild/bazel#12059 (comment)
exec_tools might no longer be needed and hence can be replaced by tools.
This fixes various build failures caused by missing environment
variables in environments where they are required, e.g. using custom
compilers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

action_env ignored in some cc_library calls
6 participants