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

Fix for bindgen use in rust_shared_library #2517

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix for bindgen use in rust_shared_library #2517

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bindgen/private/bindgen.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def rust_bindgen_library(
rust_library(
name = name,
srcs = [name + "__bindgen.rs"],
deps = deps + [name + "__bindgen"],
Copy link
Contributor

@daivinhtran daivinhtran Feb 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

From rust_bindgen implementation, name + "__bindgen" seems to already re-export cc_lib through the CcInfo provider.

I'm afraid this isn't the right fix because name + "__bindgen" target now becomes completely unused. I think CI not failing yields that we don't have enough testing coverage in-place.

Could you look into why cc_lib is not propagated to downstream correctly?

#2422 might give some context.

Copy link
Contributor

Choose a reason for hiding this comment

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

rules_rust/test/bindgen/bindgen_test.bzl

Lines 15 to 35 in 777f3e5

cc_library(
name = name + "_cc",
srcs = ["simple.cc"],
hdrs = ["simple.h"],
linkopts = ["-shared"],
tags = ["manual"],
)
rust_bindgen_library(
name = name + "_rust_bindgen",
cc_lib = name + "_cc",
header = "simple.h",
tags = ["manual"],
edition = "2021",
)
rust_binary(
name = name + "_rust_binary",
srcs = ["main.rs"],
deps = [name + "_rust_bindgen"],
tags = ["manual"],
edition = "2021",
)

is a test where rust_bindgen_library is a dep of rust_binary. Since it works for for rust_binary, it's probably a bug if it doesn't work for rusts_shared_library.

Copy link
Contributor Author

@neilisaac neilisaac Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@daivinhtran if I'm reading the error message correctly, it seems like it's linking the cc_lib's code from the .rlib file rather than a .a file from the original cc_library target. That would work fine if you're linking statically.

There's a comment in _rust_bindgen_impl:

            # As in https://github.com/bazelbuild/rules_rust/pull/2361, we want
            # to link cc_lib to the direct parent (rlib) using `-lstatic=<cc_lib>` rustc flag
            # Hence, we do not need to provide the whole CcInfo of cc_lib because
            # it will cause the downstream binary to link the cc_lib again
            # (same effect as setting `leak_symbols` attribute above)
            # The CcInfo here only contains the custom link flags (i.e. linkopts attribute)
            # specified by users in cc_lib

I'm not sure if that's a good idea. Why not let rules_cc handle the corner cases itself? I don't have all the context here, but why can't rust_bindgen just be a code generator? That's what I assumed it was from the documentation

Generates a rust source file from a cc_library and a header.

hence I expected rust_bindgen_library to wrap it in a rust_library with dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the long-awaited reply. I was on vacation for past few weeks.

@UebelAndre

It looks like the decision of linking cc statically was originated in https://github.com/bazelbuild/rules_rust/pull/2361/files#diff-4bccb0ba6f0f44e4670d5726a5fcc04375b792fdaf7c0ad68b8891d1557c26d2R116. @neilisaac brought up a good point that this approach doesn't seem correct when in rust_shared_library. @UebelAndre Have you thought about this scenario before? Or was it an oversight?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did not think about the scenario with rust_shared_library. It would be very helpful if this PR could add an integration test that builds and consumes a rust_shared_library which uses rust_bindgen so we can come to an implementation that works for all use cases.

deps = deps + [cc_lib],
tags = tags,
**kwargs
)
Expand Down
Loading