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

[#486] support bazel in macos #487

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

xieyuschen
Copy link
Contributor

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

  1. Add sensible notes for the reviewer
  2. PR title is short, expressive and meaningful
  3. Relevant issues are linked in the References section
  4. Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  5. Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  6. Commits messages are according to this guideline
  7. Tests follow the best practice for testing
  8. Changelog updated in the unreleased section including API breaking changes
  9. Assign PR to reviewer
  10. All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

Closes #486

Copy link

codecov bot commented Oct 23, 2024

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

Impacted file tree graph

@@            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     

see 9 files with indirect coverage changes

@xieyuschen
Copy link
Contributor Author

@elfenpiff @elBoberido could you kindly help to review it?

@orecham
Copy link
Contributor

orecham commented Oct 23, 2024

Builds on my mac (examples work). Nice :)

I am no bazel wizard so I leave the details to @elBoberido

.bazelrc Outdated Show resolved Hide resolved
    * select different releases for different os

    * build the cbindgen because its release doesn't support macos

    * setup cxxopts for testing and macos
@xieyuschen xieyuschen force-pushed the iox2-486-support-bazel-in-macos branch from 4b638a3 to 1f746c3 Compare October 24, 2024 10:50
@orecham
Copy link
Contributor

orecham commented Oct 24, 2024

@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
Copy link
Contributor Author

@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:)

@xieyuschen
Copy link
Contributor Author

hi @elBoberido , could you kindly review it when you have time? Thanks:)

@elBoberido
Copy link
Member

@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.

@xieyuschen
Copy link
Contributor Author

xieyuschen commented Oct 30, 2024

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!

@elBoberido
Copy link
Member

@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.

@xieyuschen
Copy link
Contributor Author

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

@elBoberido
Copy link
Member

@xieyuschen okay, the we will review this for the v0.6 dev cycle. cargo will be our main build tool anyway.

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.

Bazel doesn't work in MacOS
3 participants