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.

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

Advertise CcInfo provider on rules #2126

Merged
merged 6 commits into from
Jan 5, 2024

Conversation

cameron-martin
Copy link
Contributor

The aspect used in the cc_shared_library implementation in bazel 6.3 and 7 only traverses attributes that advertise the CcInfo provider via the provides attribute. This means in these versions of bazel, rust libraries are currently omitted from libraries built with cc_shared_library.

Fixes #2101.

The aspect used in the `cc_shared_library` implementation in bazel 6.3 and 7 only traverses attributes that advertise the `CcInfo` provider via the `provides` attribute. This means in these versions of bazel, rust libraries are currently omitted from libraries built with `cc_shared_library`.
@cameron-martin
Copy link
Contributor Author

cameron-martin commented Sep 6, 2023

It seems that the CcInfo provider isn't returned when the target arch is wasm, which causes an error in this case. However, you don't have this information when creating the rule. What do you think the best way around this is? Is there a reason why it can't return a CcInfo provider in the wasm case? cc @UebelAndre.

@cameron-martin
Copy link
Contributor Author

Looks like the issue with wasm not emitting CcInfo is fixed by #1979

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Thanks! I think this looks fine but I had one question.


# Tests that cc_shared_library correctly traverses into
# `rust_static_library` when linking.
cc_shared_library(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this lead to the same issue as #2359 if experimental_cc_shared_library is not set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean if people are querying it as an external repository, where the root workspace does not have this set?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose it will, although in 7.0.0 this is enabled by default so it shouldn't be an issue with the latest version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@illicitonion given that the issue was referring to a cquery within the rules_rust repo, do you think this is an acceptable change? I do like the procides changes and love the test. I just don't wanna immediately regress something here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I sent out #2398 which add tests for what I think we should be aiming to support (i.e. bazel query ... and bazel cquery ... from inside the rules_rust repo) - if we have more things we want to support (e.g. bazel query @rules_rust//... from a repo that depends on rules_rust) , we should add tests to CI for them.

This PR passes the tests I sent out, so I'm happy :)

@UebelAndre
Copy link
Collaborator

Sounds good. Just needs a rebase!

@UebelAndre UebelAndre merged commit 95e5d44 into bazelbuild:main Jan 5, 2024
3 checks passed
@cameron-martin cameron-martin deleted the advertise-cc-info branch January 10, 2024 11:23
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.

Libraries not linked by cc_shared_library
3 participants