Skip to content

Apply get_lib_name correctly to the C++ runtime libraries #1508

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

Merged
merged 2 commits into from
Aug 9, 2022

Conversation

scentini
Copy link
Collaborator

@scentini scentini commented Aug 9, 2022

#1500 added an additional for_windows parameter to get_lib_name. I missed the fact that we also pass that function to map_each here: https://github.com/bazelbuild/rules_rust/blob/main/rust/private/rustc.bzl#L1671
and as such, this code does not always work correctly (we don't get to pass the for_windows parameter, and internally at Google it ended up evaluating to True on Linux builds).

I tried to avoid flattening the cc_toolchain.dynamic_runtime_lib and cc_toolchain.static_runtime_lib depsets by using a lambda:

args.add_all(
    cc_toolchain.dynamic_runtime_lib(feature_configuration = feature_configuration),
    map_each = lambda x: get_lib_name(x, for_windows = toolchain.os.startswith("windows)),
    format_each = "-ldylib=%s",
)

However it looks like such usage of lambdas is not allowed:

Error in add_all: to avoid unintended retention of analysis data structures,
the map_each function (declared at ...) must be declared by a top-level def statement

So instead of get_lib_name we now have get_lib_name_default and get_lib_name_for_windows.

@scentini scentini changed the title Also apply get_lib_name correctly to the C++ runtime libraries Apply get_lib_name correctly to the C++ runtime libraries Aug 9, 2022
@scentini scentini requested a review from krasimirgg August 9, 2022 13:51
@scentini scentini marked this pull request as ready for review August 9, 2022 13:51
Copy link
Collaborator

@krasimirgg krasimirgg left a comment

Choose a reason for hiding this comment

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

LGTM

@scentini
Copy link
Collaborator Author

scentini commented Aug 9, 2022

hlopko@ pointed to me why this didn't work internally, when it does work externally:
Internally we populate the cc_toolchain.{static|dynamic}_runtime_lib fields, while externally they are not populated in the default C++ toolchain configuration for Bazel.

As for why this turned out to be an issue on Linux when get_lib_name had for_windows = False by default:
From args.add_all documentation:

map_each: callable; or None; default = None
A function that converts each item to zero or more strings, which may be further processed before appending. If this param is not provided, the standard conversion is used.
The function is passed either one or two positional arguments: the item to convert, followed by an optional DirectoryExpander. The second argument will be passed only if the supplied function is user-defined (not built-in) and declares more than one parameter.

So we end up passing a DirectoryExpander as the second argument, and as it is not None, for_windows will evaluate to True. -.-

@scentini scentini merged commit 1cd0788 into bazelbuild:main Aug 9, 2022
@scentini scentini deleted the libcpp branch August 9, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants