-
Notifications
You must be signed in to change notification settings - Fork 398
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-#2372: Depend on @ncurses when building with Bazel #2373
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2373 +/- ##
==========================================
- Coverage 78.70% 78.69% -0.01%
==========================================
Files 440 440
Lines 16981 16981
Branches 2361 2361
==========================================
- Hits 13365 13364 -1
Misses 2736 2736
- Partials 880 881 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@lalten from my side it looks fine, except that the ubuntu bazel build fails. I assigned @elBoberido to this PR since he is the one with the most bazel experience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for fixing this. I think you need to add the license header to the new files. You can use bazel run //:buildifier_lint_check
to locally check whether the CI will accept the bazel files and bazel run //:buildifier
to fix the formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. You need to rebase in order to make the CI run. If it's green, this can be merged.
Notes for Reviewer
Depend on third-party
@ncurses
when building with Bazel. This is easy with Bzlmod, but I went the hard path and made it work for legacy Workspace as well.Testing:
Note also the change from
ncurses.h
vscurses.h
. There is some background info in https://stackoverflow.com/questions/59555474/difference-between-the-headers-ncurses-h-and-curses-hThe issue is mostly that
ncurses.h
isn't available in the current BCR ncurses.There are no implementation differences iiuc.
Pre-Review Checklist for the PR Author
iox-123-this-is-a-branch
)iox-#123 commit text
)task-list-completed
)Checklist for the PR Reviewer
iceoryx_hoofs
have been added to./clang-tidy-diff-scans.txt
Post-review Checklist for the PR Author
References