-
Notifications
You must be signed in to change notification settings - Fork 522
Add runtime_libs as rust compile action inputs #3741
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
Conversation
| args.add_all(make_link_flags_args, map_each = make_link_flags) | ||
|
|
||
| args.add_all(linkstamp_outs, before_each = "-C", format_each = "link-args=%s") | ||
| args.add_all(linkstamp_outs, format_each = "-Clink-args=%s") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive-by cleanup
| if crate_type in ["dylib", "cdylib"]: | ||
| # For shared libraries we want to link C++ runtime library dynamically | ||
| # (for example libstdc++.so or libc++.so). | ||
| args.add_all( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of this is drive-by cleanup; I initially thought they needed to be passed by full path and then discovered they weren't in the sandbox at all :)
UebelAndre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
8cb02aa to
deb6664
Compare
The current handling of
static_runtime_lib/dynamic_runtime_libis broken because it never adds these libs to the action. I think it assumes they are coming fromcc_toolchain.all_filesbut that's not necessarily a safe assumption; when these libs are built from source there's no reason to force all actions to depend on both of them via the toolchain files if we may not end up using them. (We hit this scenario in https://github.com/cerisier/toolchains_llvm_bootstrapped)