[#476] Introduce test feature in iceoryx-bb for bump_allocator #478

xieyuschen · 2024-10-16T14:52:31Z

Notes for Reviewer

See proposal in the issue. By the way, could I know the rules/requirements to be met for a collaborator? Currently, I cannot assign you guys as reviewers:(

Pre-Review Checklist for the PR Author

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

All open points are addressed and tracked via issues

References

Closes #476

codecov · 2024-10-16T15:06:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.22%. Comparing base (5f6a0dc) to head (8ffa846).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #478      +/-   ##
==========================================
+ Coverage   79.19%   79.22%   +0.02%     
==========================================
  Files         200      200              
  Lines       23716    23716              
==========================================
+ Hits        18781    18788       +7     
+ Misses       4935     4928       -7

Files with missing lines	Coverage Δ
iceoryx2-bb/elementary/src/bump_allocator.rs	`86.36% <ø> (ø)`

... and 3 files with indirect coverage changes

elfenpiff · 2024-10-16T15:33:10Z

@xieyuschen I really like the approach - great idea! I am pinging @elBoberido since he is a bit more familiar with the idioms of Rust.

iceoryx2-bb/elementary/Cargo.toml

elBoberido · 2024-10-18T01:04:31Z

iceoryx2-bb/container/Cargo.toml

@@ -14,7 +14,7 @@ version = { workspace = true }

 [dependencies]
 iceoryx2-bb-derive-macros = { workspace = true }
-iceoryx2-bb-elementary = { workspace = true }
+iceoryx2-bb-elementary = { workspace = true , features = ["iox2-test"]}


This will make the features available to all user of iceoryx2-bb-elementary, at least in this workspace. I'm not sure what happens with external users. See also https://doc.rust-lang.org/cargo/reference/features.html#feature-unification

Could you check if an external crate that depends on container and elementary has access to the BumpAllocator without specifying the feature flag itself?

A workaround could be to add iceoryx2-bb-elementary = { workspace = true , features = ["iox2-test"]} as dev dependency and leave the regular dependency without the flag. One could even go one step further and just add iceoryx2-bb-elementary with the flag as regular dependency to iox2-bb-testing and re-export it in that module. Since there would be no transitive dependency on a iceoryx-bb-elementary with the flag set, except the dev targets, regular users of the crate should not have access to the BumpAllocator by accident.

What do you think?

I still need to test this with bazel tomorrow.

My bad, the workspace usage actually enables this feature to all users. But we can enable the feature inside the dev-dependency so it could solve this issue here perfectly.

If an external user runs cargo build, rustc fails and reports the errors. This satisfy our requirement:

internal testing cases doesn't be affected

outside users cannot use it except explicit enable the testing feature

error[E0433]: failed to resolve: could not find `bump_allocator` in `iceoryx2_bb_elementary` --> src/main.rs:6:36 | 6 | let _a=iceoryx2_bb_elementary::bump_allocator::BumpAllocator::new(8); | ^^^^^^^^^^^^^^ could not find `bump_allocator` in `iceoryx2_bb_elementary` | note: found an item that was configured out --> /Users/yuchen.xie/.cargo/git/checkouts/iceoryx2-d2f97997d049ef12/e3225b5/iceoryx2-bb/elementary/src/lib.rs:23:9 | 23 | pub mod bump_allocator; | ^^^^^^^^^^^^^^ note: the item is gated behind the `testing` feature --> /Users/yuchen.xie/.cargo/git/checkouts/iceoryx2-d2f97997d049ef12/e3225b5/iceoryx2-bb/elementary/src/lib.rs:22:7 | 22 | #[cfg(feature = "testing")]

external user cargo toml

[package] name = "iceoryx2-playground" version = "0.1.0" edition = "2021" [dependencies] iceoryx2 = { git = "https://github.com/xieyuschen/iceoryx2", rev = "e3225b55" } iceoryx2-bb-elementary = { git = "https://github.com/xieyuschen/iceoryx2", rev = "e3225b55" }

main.rs

use core::time::Duration; use iceoryx2::prelude::*; const CYCLE_TIME: Duration = Duration::from_secs(1); fn main() -> Result<(), Box<dyn std::error::Error>> { let _a=iceoryx2_bb_elementary::bump_allocator::BumpAllocator::new(8); let node = NodeBuilder::new().create::<ipc::Service>()?; let service = node.service_builder(&"My/Funk/ServiceName".try_into()?) .publish_subscribe::<usize>() .open_or_create()?; let publisher = service.publisher_builder().create()?; while node.wait(CYCLE_TIME).is_ok() { let sample = publisher.loan_uninit()?; let sample = sample.write_payload(1234); sample.send()?; } Ok(()) }

elBoberido · 2024-10-21T07:29:45Z

@xieyuschen it's caused by an update of the nightly toolchain. A few months ago we alreasy added a workaround to those two tests. Let's wait for a few days to see if another nightly toolchain update fixes the problem. If it persists, we will have a closer look again.

Btw, I still did not manage to check whether this approach works with bazel. I'm also on the ROSCon the next days, so it might take till Friday until I have time

elBoberido · 2024-10-21T11:23:36Z

You basically need to run bazel test //... on your branch after rebasing to main, once #482 is merged.
With the feature, it will fail and you need to add crate_features = [ "testing" ], to the rust_test_suite of the failing tests, right below srcs to kerp it consistent with other cases.

elBoberido · 2024-10-21T20:01:11Z

@xieyuschen it's now merged and you can try your luck. After writing the last comment, I'm convinced it should work without issues.

…llocator

xieyuschen · 2024-10-22T15:24:45Z

Hi @elBoberido , i have change the bazel code and verify it could pass the test in my ubuntu so i think it's ready for you to review.
The bazel doesn't work in my MacOS, but we can discuss it more in the issue so we can focus on the topic of this PR. thanks!

elBoberido · 2024-10-23T12:15:27Z

@xieyuschen it works great on bazel, even in external builds. On main there are some more cases that use the

#[doc(hidden)]
pub mod testing;

pattern. Would be great if you could also adjust those cases and also add an entry to the FAQ_ICEORYX_DEVS.md

xieyuschen · 2024-10-23T13:40:42Z

@xieyuschen it works great on bazel, even in external builds. On main there are some more cases that use the
#[doc(hidden)]
pub mod testing;
pattern. Would be great if you could also adjust those cases and also add an entry to the FAQ_ICEORYX_DEVS.md

Probably we can merge this first so you needn't take efforts to see this part of change again when i do the code change for the other cases. HDYT @elBoberido

Anyway, I will do the left things tomorrow.

elBoberido · 2024-10-23T18:34:58Z

iceoryx2-bb/elementary/BUILD.bazel

@@ -22,6 +22,7 @@ filegroup(
 rust_library(
    name = "iceoryx2-bb-elementary",
    srcs = glob(["src/**/*.rs"]),
+    crate_features = [ "testing" ],


Okay, I think here we have a problem. I totally overlooked that this one needs to be part of rust_test_suite. But in that won't work and if it is part of rust_library all consumers of iceoryx2-bb-elementary will have access to the BumpAllocator.

It seems what we want to achieve is not compatible with bazel :/

probably we can define 2 rust_library for different purposes, one for users and one for testing. I will try it today to see whether it works or not.

@elBoberido never mind, I have defined a new rust_library inside elementary with testing feature and define an alias inside container for the test cases. So it doesn't affect the common rust_library definitions.

…azel

xieyuschen · 2024-10-24T10:38:58Z

hi @elBoberido I have enabled the testing feature for test_suite only, so I believe there is no concern now:)

Please let me know whether you like this current approach, or we have a better way to do so. Then, after merging this PR and #487, I can support the others in this approach as well.

xieyuschen · 2024-10-28T05:03:08Z

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

elBoberido · 2024-10-30T00:12:00Z

@xieyuschen for now, I would just keep everything as is instead of adding a second rust_library. It's not only this one case but we use the doc hidden flag also in other places and this would result in quite a few lib duplications.

xieyuschen · 2024-10-30T03:26:36Z

hi @elBoberido , I think this is limited by bazel itself, we cannot enable a feature at the consumer side, ref.

In Bazel you can not select features of library in the consumer of the library, features should be defined in the library itself.

I don't like defining one more rust_library for testing as well, but that's the best way in bazel now. But given the case that the original goal is to hide test structures by adding new feature, looks like we don't have a consensus that whether this change makes things better.

Thanks for your review.

elBoberido · 2024-10-30T16:00:15Z

@xieyuschen I would suggest to keep this PR open and review the decision for the v0.6 development cycle.

elfenpiff self-requested a review October 16, 2024 15:30

elfenpiff assigned xieyuschen Oct 16, 2024

elfenpiff requested review from elBoberido and removed request for elfenpiff October 16, 2024 15:33

elBoberido reviewed Oct 18, 2024

View reviewed changes

xieyuschen force-pushed the iox2-476-put-allocator-under-test-feature branch from 61d544f to e3225b5 Compare October 18, 2024 10:03

xieyuschen requested a review from elBoberido October 18, 2024 10:12

xieyuschen force-pushed the iox2-476-put-allocator-under-test-feature branch from cd7e13a to e3225b5 Compare October 19, 2024 17:16

This comment was marked as resolved.

Sign in to view

[eclipse-iceoryx#476] Introduce test feature in iceoryx-bb for bump_a…

ee880ea

…llocator

xieyuschen mentioned this pull request Oct 22, 2024

Bazel doesn't work in MacOS #486

Open

xieyuschen force-pushed the iox2-476-put-allocator-under-test-feature branch from e3225b5 to bed0f27 Compare October 22, 2024 15:17

elBoberido reviewed Oct 23, 2024

View reviewed changes

[eclipse-iceoryx#476] Enable testing feature for test_suite only in b…

8ffa846

…azel

xieyuschen force-pushed the iox2-476-put-allocator-under-test-feature branch from bed0f27 to 8ffa846 Compare October 24, 2024 10:37

xieyuschen requested a review from elBoberido October 26, 2024 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#476] Introduce test feature in iceoryx-bb for bump_allocator #478

[#476] Introduce test feature in iceoryx-bb for bump_allocator #478

xieyuschen commented Oct 16, 2024 •

edited by elBoberido

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

elfenpiff commented Oct 16, 2024

elBoberido Oct 18, 2024

xieyuschen Oct 18, 2024 •

edited

Loading

This comment was marked as resolved.

elBoberido commented Oct 21, 2024

This comment was marked as resolved.

elBoberido commented Oct 21, 2024 •

edited

Loading

elBoberido commented Oct 21, 2024

This comment was marked as resolved.

xieyuschen commented Oct 22, 2024

elBoberido commented Oct 23, 2024

xieyuschen commented Oct 23, 2024 •

edited

Loading

elBoberido Oct 23, 2024

xieyuschen Oct 24, 2024

xieyuschen Oct 24, 2024

xieyuschen commented Oct 24, 2024 •

edited

Loading

xieyuschen commented Oct 28, 2024

elBoberido commented Oct 30, 2024

xieyuschen commented Oct 30, 2024

elBoberido commented Oct 30, 2024

[#476] Introduce test feature in iceoryx-bb for bump_allocator #478

Are you sure you want to change the base?

[#476] Introduce test feature in iceoryx-bb for bump_allocator #478

Conversation

xieyuschen commented Oct 16, 2024 • edited by elBoberido Loading

Notes for Reviewer

Pre-Review Checklist for the PR Author

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

elfenpiff commented Oct 16, 2024

elBoberido Oct 18, 2024

Choose a reason for hiding this comment

xieyuschen Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

external user cargo toml

This comment was marked as resolved.

elBoberido commented Oct 21, 2024

This comment was marked as resolved.

elBoberido commented Oct 21, 2024 • edited Loading

elBoberido commented Oct 21, 2024

This comment was marked as resolved.

xieyuschen commented Oct 22, 2024

elBoberido commented Oct 23, 2024

xieyuschen commented Oct 23, 2024 • edited Loading

elBoberido Oct 23, 2024

Choose a reason for hiding this comment

xieyuschen Oct 24, 2024

Choose a reason for hiding this comment

xieyuschen Oct 24, 2024

Choose a reason for hiding this comment

xieyuschen commented Oct 24, 2024 • edited Loading

xieyuschen commented Oct 28, 2024

elBoberido commented Oct 30, 2024

xieyuschen commented Oct 30, 2024

elBoberido commented Oct 30, 2024

xieyuschen commented Oct 16, 2024 •

edited by elBoberido

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

xieyuschen Oct 18, 2024 •

edited

Loading

elBoberido commented Oct 21, 2024 •

edited

Loading

xieyuschen commented Oct 23, 2024 •

edited

Loading

xieyuschen commented Oct 24, 2024 •

edited

Loading