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

fix(cmake): make sure link flags are propagated to module library builds #1308

Merged
merged 8 commits into from
Nov 27, 2024

Conversation

jungleraptor
Copy link
Contributor

@jungleraptor jungleraptor commented Oct 24, 2024

CMAKE_SHARED_LINKER_FLAGS_INIT alone does not populate the link flags for module library targets, i.e.:

add_library(foo MODULE)

This sets the variable in the crosstool script.

`CMAKE_SHARED_LINKER_FLAGS_INIT` alone does not populate the link flags
for module library targets, i.e.:

add_library(foo MODULE)

This sets the variable in the crosstool script.
@jungleraptor jungleraptor marked this pull request as ready for review October 24, 2024 23:29
@jungleraptor
Copy link
Contributor Author

All that's failing is the buildifier step but no logs indicating an issue with the code.

jungleraptor added a commit to enfabrica/enkit that referenced this pull request Oct 28, 2024
…#1129)

Fixes bazel's linker flags not being passed to CMake module library
targets - e.g.:

```cmake
add_library(foo MODULE)
```

This can lead to surprising behavior - see:
https://github.com/enfabrica/internal/blob/5ac4068d3afad4a662962740981dde0764972c98/bazel/dependencies/rdma-core.BUILD.bazel#L47

Further we can remove the hardcoded linker flags in the above target.

We should merge this if upstream takes the fix:
bazel-contrib/rules_foreign_cc#1308.

Once we upgrade it can be removed.
@jungleraptor jungleraptor reopened this Oct 31, 2024
Copy link
Member

@jsharpe jsharpe left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me - the buildifier errors are lexocographical orderings - We just need to reorder some of the dictionary key entries.

foreign_cc/private/cmake_script.bzl Outdated Show resolved Hide resolved
test/cmake_text_tests.bzl Outdated Show resolved Hide resolved
test/cmake_text_tests.bzl Outdated Show resolved Hide resolved
@jsharpe jsharpe enabled auto-merge (squash) November 27, 2024 00:14
@jsharpe jsharpe merged commit 760789a into bazel-contrib:main Nov 27, 2024
1 check passed
@jungleraptor
Copy link
Contributor Author

Thanks for your help getting this merged @jsharpe !

jsun-splunk added a commit to jsun-splunk/rules_foreign_cc that referenced this pull request Dec 17, 2024
The PR bazel-contrib#1308, added
`CMAKE_MODULE_LINKER_FLAGS_INIT` with the value of cxx_linker_shared to the
generated toolchain file in CMAKE.

This works for most use cases, however, it broke some macos builds that build
bundles. This is because the toolchain usually sets "-shared" or "-dynamiclib"
while CMAKE sets "-bundle" for Apple platforms. "-bundle" cannot co-exist with
the previous two flags.
jsun-splunk added a commit to jsun-splunk/rules_foreign_cc that referenced this pull request Dec 17, 2024
The PR bazel-contrib#1308, added
`CMAKE_MODULE_LINKER_FLAGS_INIT` with the value of cxx_linker_shared to the
generated toolchain file in CMAKE.

This works for most use cases, however, it broke some macos builds that build
bundles. This is because the toolchain usually sets "-shared" or "-dynamiclib"
while CMAKE sets "-bundle" for Apple platforms. "-bundle" cannot co-exist with
the previous two flags.
jsun-splunk added a commit to jsun-splunk/rules_foreign_cc that referenced this pull request Dec 17, 2024
The PR bazel-contrib#1308, added
`CMAKE_MODULE_LINKER_FLAGS_INIT` with the value of cxx_linker_shared to the
generated toolchain file in CMAKE.

This works for most use cases, however, it broke some macos builds that build
bundles. This is because the toolchain usually sets "-shared" or "-dynamiclib"
while CMAKE sets "-bundle" for Apple platforms. "-bundle" cannot co-exist with
the previous two flags.
jsun-splunk added a commit to jsun-splunk/rules_foreign_cc that referenced this pull request Dec 17, 2024
The PR bazel-contrib#1308, added
`CMAKE_MODULE_LINKER_FLAGS_INIT` with the value of cxx_linker_shared to the
generated toolchain file in CMAKE.

This works for most use cases, however, it broke some macos builds that build
bundles. This is because the toolchain usually sets "-shared" or "-dynamiclib"
while CMAKE sets "-bundle" for Apple platforms. "-bundle" cannot co-exist with
the previous two flags.
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