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

add support for cc_shared_library deps #1243

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

matt-sm
Copy link
Contributor

@matt-sm matt-sm commented Aug 1, 2024

This MR adds a new dynamic_deps attr that allows the libs created by cc_shared_library targets to be added as deps.

Closes #1242

@jsharpe
Copy link
Member

jsharpe commented Aug 1, 2024

This looks great!
Would you be able to also add an example, say using libz.so to the examples folder so that this is tested in CI?

@xiedeacc
Copy link

xiedeacc commented Aug 4, 2024

this commit is helpful, can guys fix check failed, then merge it into master?

@matt-sm
Copy link
Contributor Author

matt-sm commented Aug 4, 2024

There is still an issue I am trying to fix with the output CcInfo.

This can't be merged yet.

@matt-sm matt-sm force-pushed the cc-shared-library branch 5 times, most recently from 45b0783 to 3f163d0 Compare August 8, 2024 11:48
@matt-sm
Copy link
Contributor Author

matt-sm commented Aug 8, 2024

I fixed the output issue and added an example.

I did however notice 'CcSharedLibraryInfo' is not defined in the minimum version required test?

@matt-sm matt-sm force-pushed the cc-shared-library branch from 4471462 to 812c98c Compare August 8, 2024 21:39
@matt-sm
Copy link
Contributor Author

matt-sm commented Aug 9, 2024

I assume this is a blocker for adding cc_shared_library support

@jsharpe
Copy link
Member

jsharpe commented Aug 9, 2024

I assume this is a blocker for adding cc_shared_library support

Sigh, yes; as this even requires enforcing an experimental flag in bazel 6 as even if we went via rules_cc I think the act of referencing a cc_shared_library Provider will trigger the flag needing to be set in bazel 6?

@matt-sm matt-sm force-pushed the cc-shared-library branch 2 times, most recently from 4ae7f25 to fb72e78 Compare August 11, 2024 22:54
@matt-sm
Copy link
Contributor Author

matt-sm commented Aug 12, 2024

I have managed to work around this via bazel_features.

I added CcSharedLibraryInfo to globals: bazel-contrib/bazel_features@4f01bd6

Which we can now reference and prevent earlier bazel versions from failing when compiling .bzl.

The part I don't get is how to add the bazel_features dep to stardoc. See https://buildkite.com/bazel/rules-foreign-cc/builds/5889#019143a7-0041-4a33-b20a-d01ec4779041

@jsharpe
Copy link
Member

jsharpe commented Aug 12, 2024

I have managed to work around this via bazel_features.

I added CcSharedLibraryInfo to globals: bazel-contrib/bazel_features@4f01bd6

Which we can now reference and prevent earlier bazel versions from failing when compiling .bzl.

The part I don't get is how to add the bazel_features dep to stardoc. See buildkite.com/bazel/rules-foreign-cc/builds/5889#019143a7-0041-4a33-b20a-d01ec4779041

I think you need to add bazel_features to the docs WORKSPACE file?

@matt-sm matt-sm force-pushed the cc-shared-library branch 2 times, most recently from cdc809e to f883976 Compare August 12, 2024 10:24
@matt-sm
Copy link
Contributor Author

matt-sm commented Aug 12, 2024

add bazel_features to the docs WORKSPACE file

Yes I tried this but it's still failing...

@jsharpe
Copy link
Member

jsharpe commented Aug 12, 2024

add bazel_features to the docs WORKSPACE file

Yes I tried this but it's still failing...

Hmm, I'm not sure then - understanding the docs generation is on my TODO list as its failing to run on main.. @UebelAndre can you remember how the docs gen works as you put it together originally IIRC?

@matt-sm matt-sm force-pushed the cc-shared-library branch from f883976 to 7227f43 Compare August 13, 2024 03:49
@matt-sm matt-sm force-pushed the cc-shared-library branch from 7227f43 to 6dc2427 Compare August 13, 2024 03:54
@matt-sm
Copy link
Contributor Author

matt-sm commented Aug 13, 2024

Fixed - it was an error in the bzl_library deps

I wasn't using the bzl_library target @bazel_features//:features

@jsharpe jsharpe merged commit 979172f into bazel-contrib:main Aug 13, 2024
3 of 16 checks passed
jsharpe pushed a commit to jsharpe/rules_foreign_cc that referenced this pull request Aug 23, 2024
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.

cc_shared_library targets cannot be used as deps
3 participants