-
Notifications
You must be signed in to change notification settings - Fork 93
[#486] support bazel in macos #487
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
[#486] support bazel in macos #487
Conversation
Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 79.21%. Comparing base (
5f6a0dc) to head (2649f37).
Additional details and impacted files
@@ Coverage Diff @@ ## main #487 +/- ## ========================================== + Coverage 79.19% 79.21% +0.02% ========================================== Files 200 200 Lines 23716 23716 ========================================== + Hits 18781 18787 +6 + Misses 4935 4929 -6
@elfenpiff @elBoberido could you kindly help to review it?
Builds on my mac (examples work). Nice :)
I am no bazel wizard so I leave the details to @elBoberido
f9287a6 to
4b638a3
Compare
* select different releases for different os * build the cbindgen because its release doesn't support macos * setup cxxopts for testing and macos
4b638a3 to
1f746c3
Compare
@xieyuschen BTW feel free to add yourself to the contributors section of the README if you like. You've contributed a lot of great improvements.
@xieyuschen BTW feel free to add yourself to the contributors section of the README if you like. You've contributed a lot of great improvements.
thanks for your reminder, and i have added me inside the contributor section:)
hi @elBoberido , could you kindly review it when you have time? Thanks:)
@xieyuschen I'm a bit hesitant to add changes in the bazel setup. We have users with bazel 6.2 and it took us quite some time to get this working in their setup. Adding macOS into the bazel basket will add more load on the project when something in bazel breaks. That's also the reason we removed bazel for Windows. For now, we will only support bazel on Linux until we have a better understanding of the build system.
Sorry if you put effort into this and it's not going to be merged soon. You can keep this PR open and we will revisit it some time later.
hi @elBoberido , understood your concerns. I don't mind to put it here until we have a better understanding on bazel. I think the change here is straightforward as it allows the downloaded tools to be compatible with platform:)
Looks like I need to cherry-pick this commit when I need use bazel in my macos:(
Thanks for your review!
@xieyuschen are you actually using bazel on macOS? I had the impression you did it mainly to test the idea with the feature flag. If you intend to use iceoryx2 with bazel on macOS, this would of course change the situation.
Hi @elBoberido currently I use bazel for the sake of testing feature, and no real usage because I haven't saw the limitations of cargo.
I don't have a strong requirement now. Thanks
@xieyuschen okay, the we will review this for the v0.6 dev cycle. cargo will be our main build tool anyway.
similar work on bazelbuild/bazel-central-registry#3485 to use cxx easily via bzlmod
@wep21 as long as iceoryx2 can still be build with the legacy workspace and bazel 6.2 it should not be a problem to add bzlmod support. The problem is, that bazel 6.2 supports only a dated rules_rust version.
@xieyuschen please rebase to main to get the latest CI fix
Notes for Reviewer
Currently the bazel doesn't work in my desktop, so i want to support it in this PR/
Pre-Review Checklist for the PR Author
SPDX-License-Identifier: Apache-2.0 OR MITiox2-123-introduce-posix-ipc-example)[#123] Add posix ipc example)task-list-completed)Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References
Closes #486