Skip to content

[#460] Add dev_permission flag to bazel #467

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

Conversation

elBoberido
Copy link
Member

@elBoberido elBoberido commented Oct 14, 2024

Notes for Reviewer

  1. Permissions are used to manage access to shared memory objects on the file system used by iceoryx2
  2. In a docker environment, the user permissions on the host are not aligned with that within the container environment
  3. A dev_permissions feature flag has been added, allowing all processes access to all shared memory objects to allow usage in the docker environment
  4. This PR adds the dev_permissions feature flag to the bazel build

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

Relates to #460

@elBoberido elBoberido self-assigned this Oct 14, 2024
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.12%. Comparing base (a48cd97) to head (2d27b26).
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #467      +/-   ##
==========================================
- Coverage   79.21%   79.12%   -0.09%     
==========================================
  Files         196      196              
  Lines       23538    23538              
==========================================
- Hits        18646    18625      -21     
- Misses       4892     4913      +21     

see 8 files with indirect coverage changes

orecham
orecham previously approved these changes Oct 14, 2024
@elBoberido elBoberido force-pushed the iox2-460-add-dev-permission-flag-to-bazel branch from a12936a to fb759bb Compare October 14, 2024 21:38
### Feature Flags

iceoryx2 provides several feature flags that can be configured using bazel
build options.These flags allow you to enable or disable specific features
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
build options.These flags allow you to enable or disable specific features
build options. These flags allow you to enable or disable specific features

Copy link
Contributor

Choose a reason for hiding this comment

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

Also some other issues the linter would catch. Did you install an auto-formatter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it part of a git hook?

Copy link
Contributor

@orecham orecham Oct 14, 2024

Choose a reason for hiding this comment

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

@elBoberido No but am not so sure if it is a good idea for that. Ideally the auto-corrections should be verified quickly (either with a git diff or in-editor with an auto formatter). There is this script the fixing from the terminal: https://github.com/eclipse-iceoryx/iceoryx2/blob/main/internal/scripts/check_markdown_linting.sh

Copy link
Member Author

Choose a reason for hiding this comment

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

@orecham I know that script but forgot to run it. I'll create a git hook to run it on all markdown files in the commit if the markdown linter is available. That should be fairly fast and catches the issues immediately for regular contributors.

Copy link
Contributor

Choose a reason for hiding this comment

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

@elBoberido If you want to do that, the script should already take care of finding the files on the current branch (including those not committed), so the hook would only need to run the script.

The hesitation I have is the time it takes (could be too long for a hook) and the line length correction which can mess up some intentional formatting.

Feel free to change the script however you want though.

Copy link
Member Author

Choose a reason for hiding this comment

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

The hook would only do the check, not perform any changes. That would be up to the developer to trigger.

@elBoberido elBoberido force-pushed the iox2-460-add-dev-permission-flag-to-bazel branch from fb759bb to 8cc757b Compare October 14, 2024 21:45
@elBoberido elBoberido force-pushed the iox2-460-add-dev-permission-flag-to-bazel branch from 8cc757b to 2d27b26 Compare October 14, 2024 21:45
@elBoberido elBoberido requested a review from orecham October 14, 2024 21:48
@elBoberido elBoberido merged commit e5b44a9 into eclipse-iceoryx:main Oct 15, 2024
54 checks passed
@elBoberido elBoberido deleted the iox2-460-add-dev-permission-flag-to-bazel branch October 15, 2024 00:53
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