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

Stop unconditionally adding -B/usr/bin flag to C/C++ toolchains #5634

Closed
georgi-d opened this issue Jul 19, 2018 · 2 comments
Closed

Stop unconditionally adding -B/usr/bin flag to C/C++ toolchains #5634

georgi-d opened this issue Jul 19, 2018 · 2 comments
Assignees
@hlopko
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules type: feature request

Comments

@georgi-d
Copy link

The addition of this flag was done as a fix to Issue #760

The problem with this is that it completely breaks any custom toolchain setup because it forces use of tools such as ld, as, ar and others from /usr/bin. And since this is added so early in the process it is not possible override it in the build files.

I see that the original problem in #760 was that process-wrapper and build-runfiles could not run because they failed to find libstdc++ which has a better solution to just link them statically as described in #4137

At the end of #760 there are other comments mentioning that hard coding -B/usr/bin is wrong.
@hlopko hlopko added type: feature request P2 We'll consider working on this in future. (Assignee optional) labels Jul 31, 2018
@hlopko
Copy link
Member

hlopko commented Jul 31, 2018

Thanks for the report. I don't plan to work on this until #5187 is done. And I can see an universe in which we won't need so many env vars to tweak C++ toolchain. But nothing concrete yet. If this is urgent, let me know, and I will help the PR.

@ilya-biryukov
Copy link
Contributor

+1 to fixing this. This blocks using non-standard toolchains with bazel.
E.g. in our experiments with downloading a toolchain on-the-fly in Tensorflow, we can't currently use the LLD linker from the downloaded package, because -B flag is added by bazel and can't be overriden (so we end up picking LLD in /usr/bin/ if it's installed).

My intuition is that there should be a fix for #760 that does not involve setting -B.

@hlopko hlopko added team-Rules-CPP Issues for C++ rules and removed team-Rules-CPP Issues for C++ rules category: rules > C++ labels Oct 11, 2018
mzeren-vmw pushed a commit to mzeren-vmw/bazel that referenced this issue Oct 11, 2018
@gdimitrov-vmware @mzeren-vmw
[bazel] Remove hardcoded link option "-B/usr/bin"
0fd2a12 
- having this option breaks use of custom C/C++ toolchain since support binaries
  such as ld, as, etc., will incorrectly "leak" to the build machine.
- fixes issue bazelbuild#5634

Testing Done: local build of envoy. Checked that the build fails without the fix
and succeeds with the fix.
Review URL: https://reviewboard.eng.vmware.com/r/1390677/
mzeren-vmw added a commit to mzeren-vmw/bazel that referenced this issue Oct 11, 2018
@mzeren-vmw
cc_configure: Remove hardcoded link option "-B/usr/bin"
4da584d 
- Putting -B/usr/bin on the link line breaks use of custom C/C++
  toolchain since support binaries such as ld, as, etc., will
  incorrectly "leak" to the build machine.

- Fixes issue bazelbuild#5634

It looks like this logic was originally added here:

  810d60a Apr 22 2016 dmarting@google.com
  "cc_configure: Add -B to compiler flag too"

And that that was done to get Tensor Flow building on some (but failing
on other?) versions of RedHat. bazelbuild#1152.

Also this may have "fixed" the hombrew builds for
bazelbuild#1177.

Testing Done: local build of envoy on older host using non-host
crosscompiler. Build fails without the fix and succeeds with the fix.
mzeren-vmw added a commit to mzeren-vmw/bazel that referenced this issue Oct 11, 2018
@mzeren-vmw
cc_configure: Remove hardcoded link option "-B/usr/bin"
f97954a 
Putting -B/usr/bin on the link line breaks use of custom C/C++ toolchain
since support binaries such as ld, as, etc., will incorrectly "leak" to
the build machine.

Fixes issue bazelbuild#5634

Archeaology: It looks like this logic was originally added here:

  810d60a Apr 22 2016 dmarting@google.com
  "cc_configure: Add -B to compiler flag too"

.. and that that was done to get Tensor Flow building on some (but failing
on other?) versions of RedHat. bazelbuild#1152.

This may have also "fixed" the hombrew builds for
bazelbuild#1177.

Testing Done:

* Building envoy on older Centos host using non-host
  crosscompiler. Build fails without the fix and succeeds with the fix.

* bazel test //src/test/java/com/google/devtools/build/lib:all
  1 flaky test failed before and after:
  bazel test //src/test/java/com/google/devtools/build/lib:vfs_test
mzeren-vmw added a commit to mzeren-vmw/bazel that referenced this issue Oct 11, 2018
@mzeren-vmw
cc_configure: Remove hardcoded link option "-B/usr/bin"
89bc933 
Putting -B/usr/bin on the link line breaks use of custom C/C++ toolchain
since support binaries such as ld, as, etc., will incorrectly "leak" to
the build machine.

Fixes bazelbuild#5634

Archeaology: It looks like this logic was originally added here:

  810d60a Apr 22 2016 dmarting@google.com
  "cc_configure: Add -B to compiler flag too"

.. and that that was done to get Tensor Flow building on some (but failing
on other?) versions of RedHat. bazelbuild#1152.

This may have also "fixed" the hombrew builds for
bazelbuild#1177.

Testing Done:

* Building envoy on older Centos host using non-host
  crosscompiler. Build fails without the fix and succeeds with the fix.

* bazel test //src/test/java/com/google/devtools/build/lib:all
  1 flaky test failed before and after:
  bazel test //src/test/java/com/google/devtools/build/lib:vfs_test
@mzeren-vmw mzeren-vmw mentioned this issue Oct 11, 2018
Closed
mzeren-vmw added a commit to mzeren-vmw/bazel that referenced this issue Oct 11, 2018
@mzeren-vmw
cc_configure: Remove hardcoded link option "-B/usr/bin"
ca25e12 
Putting -B/usr/bin on the link line breaks use of custom C/C++ toolchain
since support binaries such as ld, as, etc., will incorrectly "leak" to
the build machine.

Fixes bazelbuild#5634

Archeaology: It looks like this logic was originally added here:

  810d60a Apr 22 2016 dmarting@google.com
  "cc_configure: Add -B to compiler flag too"

.. and that that was done to get Tensor Flow building on some (but failing
on other?) versions of RedHat. bazelbuild#1152.

This may have also "fixed" the hombrew builds for
bazelbuild#1177.

Testing Done:

* Building envoy on older Centos host using non-host
  crosscompiler. Build fails without the fix and succeeds with the fix.

* bazel test //src/test/java/com/google/devtools/build/lib:all
  1 flaky test failed before and after:
  bazel test //src/test/java/com/google/devtools/build/lib:vfs_test
mzeren-vmw added a commit to mzeren-vmw/bazel that referenced this issue Oct 11, 2018
@mzeren-vmw
cc_configure: Remove hardcoded link option "-B/usr/bin"
778b21d 
Putting -B/usr/bin on the link line breaks use of custom C/C++ toolchain
since support binaries such as ld, as, etc., will incorrectly "leak" to
the build machine.

Fixes bazelbuild#5634

Archeaology: It looks like this logic was originally added here:

  810d60a Apr 22 2016 dmarting@google.com
  "cc_configure: Add -B to compiler flag too"

.. and that that was done to get Tensor Flow building on some (but failing
on other?) versions of RedHat. bazelbuild#1152.

This may have also "fixed" the hombrew builds for
bazelbuild#1177.

Testing Done:

* Building envoy on older Centos host using non-host
  crosscompiler. Build fails without the fix and succeeds with the fix.

* bazel test //src/test/java/com/google/devtools/build/lib:all
  1 flaky test failed before and after:
  bazel test //src/test/java/com/google/devtools/build/lib:vfs_test
Flamefire added a commit to Flamefire/tensorflow that referenced this issue Nov 12, 2019
@Flamefire
Remove adding of /usr/bin to compiler paths
04ef5dd 
As the underlying Bazel issue bazelbuild/bazel#5634
is resolved, this coda can (and should) go now
Flamefire added a commit to Flamefire/tensorflow that referenced this issue Nov 12, 2019
@Flamefire
Remove adding of /usr/bin to compiler paths
ba40b1e 
As the underlying Bazel issue bazelbuild/bazel#5634
is resolved, this coda can (and should) go now
Flamefire added a commit to Flamefire/tensorflow that referenced this issue Nov 12, 2019
@Flamefire
Remove adding of /usr/bin to compiler paths
36d31a2 
As the underlying Bazel issue bazelbuild/bazel#5634
is resolved, this code can (and should) go now
@Flamefire Flamefire mentioned this issue Nov 12, 2019
Merged
Flamefire added a commit to Flamefire/tensorflow that referenced this issue Nov 14, 2019
@Flamefire
Remove adding of /usr/bin to compiler paths
2082d70 
As the underlying Bazel issue bazelbuild/bazel#5634
is resolved, this code can (and should) go now

Adding /usr/bin unconditionally does break builds with custom binutils
installed elsewhere
@Flamefire Flamefire mentioned this issue May 7, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hlopko hlopko

Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules type: feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cc_configure: Remove hardcoded link option "-B/usr/bin" mzeren-vmw/bazel
4 participants
@jin @ilya-biryukov @hlopko @georgi-d