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

iox-#1542 Support Bazel build system #1543

Merged
merged 14 commits into from
Jul 29, 2022

Conversation

kwallner
Copy link
Contributor

@kwallner kwallner commented Jul 25, 2022

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-123-this-is-a-branch)
  5. Commits messages are according to this guideline
    • Commit messages have the issue ID (iox-#123 commit text)
    • Commit messages are signed (git commit -s)
    • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. All touched (C/C++) source code files are added to ./clang-tidy-diff-scans.txt
  11. Assign PR to reviewer

Notes for Reviewer

Building

Building as described in doc/website/getting-started/installation.md with:

bazel build //...

Build will use gcc. Other compilers can be selected using environment variables:

CC=clang bazel build //...

Testing

Running all tests with:

bazel test //...

Issues / Open Points / Discuss

1. Bazel version is not defined

Would be an option to set a specific bazel version in .bazelversion file.

2. C++-Standard is set to C++14

Used .bazelrc to set compiler flags for c++14.
Changing will require to edit .bazelrc.

3. Only linux (ubuntu 20.04) tested

Extending to different operating systems would be an follow up issue.
Maybe should be created right away.

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
    • Each unit test case has a unique UUID
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • All touched (C/C++) source code files have been added to ./clang-tidy-diff-scans.txt
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

Copy link
Contributor

@elfenpiff elfenpiff left a comment

Choose a reason for hiding this comment

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

I support this new feature completely but three questions/things are still open for me:

  1. Who is responsible for the bazel setup and takes care of additional bazel issues? @kwallner would you be willing to respond quickly to upcoming bazel issues?

  2. We have at the moment no verification by the CI that this bazel setup is actually working. Could you add a bazel CI target for ubuntu-latest where everything is build.

  3. For which platforms do we want to provide bazel support? Only linux or the full platform list, namely: FreeBSD, MacOS, Windows, QNX and Linux? This is nowhere mentioned in the markdown files and needs to be added if bazel support is restricted to certain platforms.

@dkroenke
Copy link
Member

@elfenpiff

  1. Who is responsible for the bazel setup and takes care of additional bazel issues? @kwallner would you be willing to respond quickly to upcoming bazel issues?

That will be me.

  1. We have at the moment no verification by the CI that this bazel setup is actually working. Could you add a bazel CI target for ubuntu-latest where everything is build.

The CI target will be added with this MR, Karl just made the intial commits ready.
We will add a sanity check job on Ubuntu that builds and test with bazel.

  1. For which platforms do we want to provide bazel support?

Ubuntu only (20.04).

@kwallner kwallner requested a review from dkroenke July 25, 2022 14:22
@dkroenke
Copy link
Member

dkroenke commented Jul 25, 2022

@kwallner
Regarding the open points to discuss:

  1. Bazel version is not defined
    Would be an option to set a specific bazel version in .bazelversion file.

Wouldn't this not make our project harder to integrate into other bazel workspaces?
For me it make sense to support the latest (stable) version of bazel that we should test in the CI.
According to the bazel Bazel versioning this would be currently 5.2 to use in the CI.
Support for older versions is not planned.
For now we can try to live without a bazel version file for more flexibility, if we face increased effort by maintaining multiple versions we can still fix the version.

  1. C++-Standard is set to C++14
    Used .bazelrc to set compiler flags for c++14.
    Changing will require to edit .bazelrc.

For now it is ok to enforce here C++14 as we do the same in CMake. We do not change the C++ standard that often so that maintaining this file should not be a problem.

  1. Only linux (ubuntu 20.04) tested
    Extending to different operating systems would be an follow up issue.
    Maybe should be created right away.

We have to consider that we already face considerable build times on the GitHub Actions with the existing jobs due to increased usage of Runners for etc.
MacOS and Windows support is mainly driven by the ROS 2 integration where Bazel is not used as build system.
The MacOS runners are already in excessive use so that i would here more rely on the community if the build on MacOS faces a problem.

Possible follow-ups on the following questions:

  • Code-coverage
  • Adress-sanitizer and friends in bazel

@dkroenke
Copy link
Member

@kwallner You need to add the bazel-* directories to .gitignore file.

@mossmaurice mossmaurice added the tooling All iceoryx related tooling (scripts etc.) label Jul 25, 2022
@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

Merging #1543 (0e1dff1) into master (c30d930) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1543      +/-   ##
==========================================
+ Coverage   78.88%   78.94%   +0.05%     
==========================================
  Files         378      379       +1     
  Lines       14483    14489       +6     
  Branches     2028     2028              
==========================================
+ Hits        11425    11438      +13     
+ Misses       2431     2424       -7     
  Partials      627      627              
Flag Coverage Δ
unittests 78.60% <ø> (+0.05%) ⬆️
unittests_timing 14.89% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
iceoryx_hoofs/include/iceoryx_hoofs/cxx/string.hpp 100.00% <ø> (ø)
.../internal/relocatable_pointer/relative_pointer.hpp 100.00% <ø> (ø)
...oofs/source/posix_wrapper/shared_memory_object.cpp 77.63% <0.00%> (-0.58%) ⬇️
...eoryx_hoofs/source/posix_wrapper/message_queue.cpp 0.00% <0.00%> (ø)
...nternal/relocatable_pointer/pointer_repository.inl 88.63% <0.00%> (ø)
...include/iceoryx_hoofs/internal/concurrent/sofi.hpp 100.00% <0.00%> (ø)
...include/iceoryx_hoofs/internal/concurrent/sofi.inl 85.00% <0.00%> (+0.78%) ⬆️
iceoryx_hoofs/source/posix_wrapper/named_pipe.cpp 59.37% <0.00%> (+0.86%) ⬆️
iceoryx_posh/source/roudi/port_manager.cpp 85.36% <0.00%> (+1.12%) ⬆️
iceoryx_hoofs/source/concurrent/loffli.cpp 91.42% <0.00%> (+2.85%) ⬆️

@kwallner
Copy link
Contributor Author

kwallner commented Jul 25, 2022

@elfenpiff

"Code Linting" is caused by failed request to https://github.com/eclipse-iceoryx/iceoryx/blob/master/BUILD .
This file will exist after the merge.

How to handle this?

Fixed already.

@kwallner
Copy link
Contributor Author

@dkroenke

Don't understand checklist task "All touched (C/C++) source code files are added to ./clang-tidy-diff-scans.txt". What is the actual TODO here?

@dkroenke
Copy link
Member

@dkroenke

Don't understand checklist task "All touched (C/C++) source code files are added to ./clang-tidy-diff-scans.txt". What is the actual TODO here?

@kwallner this check affects only the C++ code in iceoryx, since we are only touching the include paths for the internal API (and correct it) this is out of scope here.

@dkroenke dkroenke self-requested a review July 28, 2022 12:07
Add linter check for bazel in CI

Signed-off-by: Dietrich Krönke <[email protected]>
dkroenke added a commit to kwallner/iceoryx that referenced this pull request Jul 28, 2022
@dkroenke
Copy link
Member

LGTM from my side! Many thanks @kwallner for this great contribution. 👏

@dkroenke
Copy link
Member

dkroenke commented Jul 28, 2022

Follow-up topics for Bazel are tracked here: #1547

@dkroenke dkroenke requested a review from elfenpiff July 28, 2022 15:38
@dkroenke dkroenke merged commit 2c6827c into eclipse-iceoryx:master Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling All iceoryx related tooling (scripts etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Bazel build system (as alternative to CMake)
4 participants