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

Pass C++ runtime lib when C++ toolchain declares it #562

Merged
merged 2 commits into from
Feb 9, 2021
Merged

Pass C++ runtime lib when C++ toolchain declares it #562

merged 2 commits into from
Feb 9, 2021

Conversation

hlopko
Copy link
Member

@hlopko hlopko commented Jan 28, 2021 •
edited
Loading

Currently rules_rust won't link against the C++ standard library in case
C++ toolchain is the static_link_cpp_runtimes feature (therefore when
it uses cc_toolchain.static_runtime_lib and cc_toolchain.dynamic_runtime_lib
attributes to declare which version of C++ static library should be
linked into the final binary).

This PR modifies add_native_link_flags to also look into the
C++ toolchain and depend on C++ standard library from there.

While there, I moved the get_cc_toolchain function to the utils.rs,
to be consistent with the handling of the Rust toolchain.

@google-cla google-cla bot added the cla: yes label Jan 28, 2021
@hlopko hlopko force-pushed the pass_runtime_libs branch 2 times, most recently from f1a6c6d to 6408822 Compare January 28, 2021 13:22
@hlopko
Copy link
Member Author

hlopko commented Jan 28, 2021

cc @MForster

dfreese
dfreese reviewed Jan 28, 2021
View reviewed changes
rust/private/utils.bzl Outdated Show resolved Hide resolved
Base automatically changed from master to main January 28, 2021 20:47
dfreese
dfreese reviewed Jan 28, 2021
View reviewed changes
Copy link
Collaborator

@dfreese dfreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last question, and then generally good. Would appreciate a second set of eyes from someone with more of a bazel background.

rust/private/utils.bzl Outdated Show resolved Hide resolved
rust/private/utils.bzl Outdated Show resolved Hide resolved
@hlopko
Copy link
Member Author

hlopko commented Feb 1, 2021

Update:

I discovered that when creating an rlib artifact, rustc will open every static library and "copy" its contents into the rlib. This results in a quadratic disk usage growth and that's not acceptable in our workloads. I'll create a separate PR to stop passing native libraries when producing rlibs and wait with this one.

hlopko added a commit to hlopko/rules_rust that referenced this pull request Feb 3, 2021
@hlopko
Do not pass native deps to rlib compilations
4584d8d 
When creating an rlib artifact, rustc will open every
native static library and "copy" its contents into the rlib. This
results in a quadratic disk usage growth and that's not necessary for
rules_rust because we track native dependencies are we can safely pass
them to the final linking actions.

This fixes the build failure when a native dep is actually a linker
script. Rustc will try to open the linker script as if it was a static
archive, and then fail the build.

Creating shared library artifacts (dylib, cdylib) and binaries is not
affected, for those rustc uses the linker, and linker will consume the
linker script just fine. Rustc won't try to open/interpret the linker
script itself.

This also unblocks bazelbuild#562.
@hlopko hlopko mentioned this pull request Feb 3, 2021
Merged
hlopko added a commit to hlopko/rules_rust that referenced this pull request Feb 3, 2021
@hlopko
Do not pass native deps to rlib compilations
483943f 
When creating an rlib artifact, rustc will open every
native static library and "copy" its contents into the rlib. This
results in a quadratic disk usage growth and that's not necessary for
rules_rust because we track native dependencies are we can safely pass
them to the final linking actions.

This fixes the build failure when a native dep is actually a linker
script. Rustc will try to open the linker script as if it was a static
archive, and then fail the build.

Creating shared library artifacts (dylib, cdylib) and binaries is not
affected, for those rustc uses the linker, and linker will consume the
linker script just fine. Rustc won't try to open/interpret the linker
script itself.

This also unblocks bazelbuild#562.
hlopko added a commit to hlopko/rules_rust that referenced this pull request Feb 4, 2021
@hlopko
Do not pass native deps to rlib compilations
5671cd4 
When creating an rlib artifact, rustc will open every
native static library and "copy" its contents into the rlib. This
results in a quadratic disk usage growth and that's not necessary for
rules_rust because we track native dependencies are we can safely pass
them to the final linking actions.

This fixes the build failure when a native dep is actually a linker
script. Rustc will try to open the linker script as if it was a static
archive, and then fail the build.

Creating shared library artifacts (dylib, cdylib) and binaries is not
affected, for those rustc uses the linker, and linker will consume the
linker script just fine. Rustc won't try to open/interpret the linker
script itself.

This also unblocks bazelbuild#562.
hlopko added a commit to hlopko/rules_rust that referenced this pull request Feb 4, 2021
@hlopko
Do not pass native deps to rlib compilations
757510f 
When creating an rlib artifact, rustc will open every
native static library and "copy" its contents into the rlib. This
results in a quadratic disk usage growth and that's not necessary for
rules_rust because we track native dependencies are we can safely pass
them to the final linking actions.

This fixes the build failure when a native dep is actually a linker
script. Rustc will try to open the linker script as if it was a static
archive, and then fail the build.

Creating shared library artifacts (dylib, cdylib) and binaries is not
affected, for those rustc uses the linker, and linker will consume the
linker script just fine. Rustc won't try to open/interpret the linker
script itself.

This also unblocks bazelbuild#562.
@hlopko hlopko force-pushed the pass_runtime_libs branch 3 times, most recently from f88c3ce to b0d91e2 Compare February 4, 2021 21:35
@hlopko
Copy link
Member Author

hlopko commented Feb 4, 2021

#576 has to be submitted first.

hlopko added a commit that referenced this pull request Feb 5, 2021
@hlopko
Do not pass native deps to rlib compilations (#576)
3b02397 
When creating an rlib artifact, rustc will open every
native static library and "copy" its contents into the rlib. This
results in a quadratic disk usage growth and that's not necessary for
rules_rust because we track native dependencies are we can safely pass
them to the final linking actions.

This fixes the build failure when a native dep is actually a linker
script. Rustc will try to open the linker script as if it was a static
archive, and then fail the build.

Creating shared library artifacts (dylib, cdylib) and binaries is not
affected, for those rustc uses the linker, and linker will consume the
linker script just fine. Rustc won't try to open/interpret the linker
script itself.

This also unblocks #562.
@hlopko hlopko force-pushed the pass_runtime_libs branch 2 times, most recently from 37d83d0 to ff2d7f2 Compare February 5, 2021 07:16
@hlopko
Pass C++ runtime lib when C++ toolchain declares it
8ac6e3a 
Currently `rules_rust` won't link against the C++ standard library in case
C++ toolchain is the `static_link_cpp_runtimes` feature (therefore when
it uses `cc_toolchain.static_runtime_lib` and `cc_toolchain.dynamic_runtime_lib`
attributes to declare which version of C++ static library should be
linked into the final binary).

This PR modifies `get_libs_for_static_executable` to also look into the
C++ toolchain and depend on C++ standard library from there.

Once we implement the support for dynamically linked rust binaries we
will likely have an equivalend `get_libs_for_dynamic_executable`, which
would then look into `cc_toolchain.dynamic_runtime_lib`.

While there, I moved the `get_cc_toolchain` function to the `utils.rs`,
to be consistent with the handling of the Rust toolchain.
@hlopko
Copy link
Member Author

hlopko commented Feb 5, 2021

This PR is now ready for review. Blocking PRs have been merged, and I've tested it internally with a C++ toolchain that uses static/dynamic_runtime_lib. Please take another look :)

@hlopko hlopko merged commit 820a149 into bazelbuild:main Feb 9, 2021
@hlopko hlopko deleted the pass_runtime_libs branch April 1, 2021 07:48
@illicitonion illicitonion mentioned this pull request May 27, 2021
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfreese dfreese dfreese left review comments

@damienmg damienmg damienmg approved these changes

@mfarrugi mfarrugi Awaiting requested review from mfarrugi

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hlopko @dfreese @damienmg