iox-#2370 Fix Bzlmod dev_dependency setup #2371

lalten · 2024-11-15T14:27:18Z

Notes for Reviewer

Fix Bzlmod dev_dependency setup

Pre-Review Checklist for the PR Author

Checklist for the PR Reviewer

Consider a second reviewer for complex new features or larger refactorings
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
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Closes incorrect Bazel dev_dependency setup #2370

codecov · 2024-11-15T14:57:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.25%. Comparing base (7592b2a) to head (5a207da).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2371   +/-   ##
=======================================
  Coverage   78.25%   78.25%           
=======================================
  Files         445      445           
  Lines       17091    17091           
  Branches     2373     2373           
=======================================
+ Hits        13374    13375    +1     
  Misses       2835     2835           
+ Partials      882      881    -1

Flag	Coverage Δ
unittests	`78.05% <ø> (+<0.01%)`	⬆️
unittests_timing	`15.31% <ø> (ø)`

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

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

elfenpiff · 2024-11-18T10:57:53Z

@lalten looks fine. The MinGW CI failure is a flaky test, which we can ignore, but the Ubuntu bazel target fails.

I assigned @elBoberido since he may has some more insights here.

elBoberido · 2024-11-18T14:38:13Z

@lalten the bzlmod code came from a community member and I'm not that familiar with the code. Can you specify which public targets are affected by the dev dependencies?

lalten · 2024-11-18T15:38:34Z

@lalten the bzlmod code came from a community member and I'm not that familiar with the code. Can you specify which public targets are affected by the dev dependencies?

Buildifier is unconditionally depended on in the root BUILD

iceoryx/BUILD.bazel

Line 18 in d628cb7

load("@buildifier_prebuilt//:rules.bzl", "buildifier")

iceoryx/BUILD.bazel

Lines 61 to 65 in d628cb7

    
           # Execute `bazel run //:buildifier` to fix formating of all starlark files in the workspace 
        
           buildifier( 
        
               name = "buildifier", 
        
               verbose = True, 
        
           )

Googletest is depended on in a regular cc_library with public visibility:

iceoryx/iceoryx_hoofs/BUILD.bazel

Line 107 in d628cb7

"@googletest//:gtest",

elBoberido · 2024-11-18T16:06:31Z

Okay, the buildifier is definitely a dev dependency. The iceoryx_hoofs_testing is actually also a dev dependency but I'm not sure how this could be modeled with bazel. It is basically meant to be used for tests which use iceoryx. For example it contains a logger which suppresses all the output but if a test fails, it prints all log messages to help debugging.

Is there another way to mark this library as a dev dependency but still make it available to users?

In the end, I guess it should not matter and as long as one does depend only on e.g. iceoryx_hoofs bazel will not require gTest to be available. Please correct me if I'm wrong.

lalten · 2024-11-18T16:19:11Z

dev dependencies can not be depended on outside the root module, i.e. as soon as iceoryx is used as Bzlmod dependency of another project (which of course is the intended use case), dev dependencies are not there anymore.

See https://bazel.build/rules/lib/globals/module#parameters_1

dev_dependency: If true, this dependency will be ignored if the current module is not the root module or --ignore_dev_dependency is enabled.

lalten · 2024-11-18T16:21:08Z

~~Note that Bazel will want to analyze the entire graph, so even if a user doesn't want to depend on iceoryx_hoofs_testing (or buildifier), these Bazel repos must be visible to the root module.~~

Looks like the buildifier usage is actually ok, here is another repo with the same setup:
https://github.com/aspect-build/rules_ts/blob/v3.3.1/MODULE.bazel#L26
https://github.com/aspect-build/rules_ts/blob/v3.3.1/BUILD.bazel#L3

lalten · 2024-11-18T16:42:22Z

🤷

❯ cat MODULE.bazel
module(name = "test")

bazel_dep(name = "org_eclipse_iceoryx")
git_override(
    module_name = "org_eclipse_iceoryx",
    commit = "d628cb7111051ae1cd27620d9a621a366d5cac82",
    remote = "https://github.com/eclipse-iceoryx/iceoryx",
)
❯ bazel build @org_eclipse_iceoryx//:iceoryx_binding_c
WARNING: Target pattern parsing failed.
ERROR: Skipping '@org_eclipse_iceoryx//:iceoryx_binding_c': error loading package '@@org_eclipse_iceoryx~//': Unable to find package for @@[unknown repo 'buildifier_prebuilt' requested from @@org_eclipse_iceoryx~]//:rules.bzl: The repository '@@[unknown repo 'buildifier_prebuilt' requested from @@org_eclipse_iceoryx~]' could not be resolved: No repository visible as '@buildifier_prebuilt' from repository '@@org_eclipse_iceoryx~'.
ERROR: error loading package '@@org_eclipse_iceoryx~//': Unable to find package for @@[unknown repo 'buildifier_prebuilt' requested from @@org_eclipse_iceoryx~]//:rules.bzl: The repository '@@[unknown repo 'buildifier_prebuilt' requested from @@org_eclipse_iceoryx~]' could not be resolved: No repository visible as '@buildifier_prebuilt' from repository '@@org_eclipse_iceoryx~'.
INFO: Elapsed time: 1.897s
INFO: 0 processes.
ERROR: Build did NOT complete successfully

elBoberido · 2024-11-18T18:19:40Z

Okay, this is weird. Why should the buildifier be needed in order to build iceoryx_hoofs? Bazel has some weird dependency handling.

Do you have time to check it there is an obvious flaw in the bzlmod setup?

lalten · 2025-03-14T09:19:18Z

This is still required, without this patch consumers can not depend on the repo as a library.

lalten · 2025-03-14T09:22:08Z

Okay, this is weird. Why should the buildifier be needed in order to build iceoryx_hoofs? Bazel has some weird dependency handling.

Do you have time to check it there is an obvious flaw in the bzlmod setup?

The reason that buildifier_prebuilt must be available is that it is load()ed in

iceoryx/BUILD.bazel

Line 18 in 7592b2a

load("@buildifier_prebuilt//:rules.bzl", "buildifier")

this BUILD must go through build graph analysis by all consumers of iceoryx as a library. Since iceoryx does not state a dependency on buildifier_prebuilt (dev_dependencies are invisible when not the root repo) this load fails and breaks the consumer's build graph.

lalten · 2025-03-14T10:40:48Z

I rebased the commit. There isn't really any drawback to just depending on buildifier and googletest. Users don't need to install anything or do any other action, Bazel will handle everything for them.

The change is completely transparent for developers of the repo who use it as root module, and it enables use of the repo as a library for downstream consumers.

Add https://github.com/eclipse-iceoryx/iceoryx/releases/tag/v2.95.4 Includes patches for these PRs: * eclipse-iceoryx/iceoryx#2371 * eclipse-iceoryx/iceoryx#2433 * eclipse-iceoryx/iceoryx#2436 * eclipse-iceoryx/iceoryx#2437 * eclipse-iceoryx/iceoryx#2439 Added `test_targets` in presubmit as well, except for those that are failing due to lack of resources in ci

elBoberido · 2025-03-14T14:11:14Z

Okay, this is weird. Why should the buildifier be needed in order to build iceoryx_hoofs? Bazel has some weird dependency handling.
Do you have time to check it there is an obvious flaw in the bzlmod setup?

The reason that buildifier_prebuilt must be available is that it is load()ed in

iceoryx/BUILD.bazel

Line 18 in 7592b2a

load("@buildifier_prebuilt//:rules.bzl", "buildifier")

this BUILD must go through build graph analysis by all consumers of iceoryx as a library. Since iceoryx does not state a dependency on buildifier_prebuilt (dev_dependencies are invisible when not the root repo) this load fails and breaks the consumer's build graph.

Okay, just out of interest. Assuming I would not want to declare this as public dependency but keep it as dev dependency. Would it be possible to adjust the root BUILD.bazel file in a way to achieve this goal?

lalten · 2025-03-14T14:39:14Z

Would it be possible to adjust the root BUILD.bazel file in a way to achieve this goal?

I think it may work if you only load() the buildifier repo in BUILD files that don't contain any targets that are dependencies of publicly visible targets. Would need to try that out.

Keep in mind consumers may actually want to run the tests (like in the presubmit CI of the BCR), so the BUILDs that contain the tests should work.
One approach is to have a separate MODULE.bazel for that that loads the main module from .. and adds googletest and friends as bazel_dep (example: https://github.com/bazelbuild/bazel-central-registry/blob/019b40da8e5beb3071058bc57d76d0547c2258d9/modules/pcl/1.14.1/overlay/test/MODULE.bazel) but that is not great for developers wanting to work in the repo itself

elBoberido · 2025-03-14T15:03:31Z

Having the tests for the users is perfectly fine. It's the buildifier that is totally irrelevant to the consumers to the library.

Currently, the bazel/BUILD.bazel file is empty. Would it make sense to move the buildifier stuff to that file and keep it as dev_dependency?

lalten · 2025-03-14T17:07:33Z

tbh I don't think you need the buildifier to be part of the Bazel build graph at all. It's much more common to put it in pre-commit instead: https://github.com/warchant/pre-commit-buildifier

elBoberido · 2025-03-14T18:45:52Z

Agreed with the buildifier not being part of the bazel grahp. But since not all of our contributors are using bazel, we cannot rely on the commit hooks. A CI check would be sufficient, though. Could you add a buildifier action to the bazel job and adjust the doc/website/advanced/installation-guide-for-contributors.md file accordingly. Maybe add that one can get a prebuild version from https://github.com/keith/buildifier-prebuilt/. Then the buildifier could be removed from the bazel graph and tools/ci/build-test-ubuntu-bazel.sh.

If it is easier, it would also be fine to add the prebuild buildifier to tools/ci/build-test-ubuntu-bazel.sh instead of the github action.

lalten · 2025-03-14T21:42:33Z

Agreed with the buildifier not being part of the bazel grahp. But since not all of our contributors are using bazel, we cannot rely on the commit hooks. A CI check would be sufficient, though. Could you add a buildifier action to the bazel job and adjust the doc/website/advanced/installation-guide-for-contributors.md file accordingly. Maybe add that one can get a prebuild version from https://github.com/keith/buildifier-prebuilt/. Then the buildifier could be removed from the bazel graph and tools/ci/build-test-ubuntu-bazel.sh.

If it is easier, it would also be fine to add the prebuild buildifier to tools/ci/build-test-ubuntu-bazel.sh instead of the github action.

I sent #2441

Add https://github.com/eclipse-iceoryx/iceoryx/releases/tag/v2.95.4 Includes patches for these PRs: * eclipse-iceoryx/iceoryx#2371 * eclipse-iceoryx/iceoryx#2433 * eclipse-iceoryx/iceoryx#2436 * eclipse-iceoryx/iceoryx#2437 * eclipse-iceoryx/iceoryx#2439 Added `test_targets` in presubmit as well, except for those that are failing due to lack of resources in ci

elfenpiff requested a review from elBoberido November 18, 2024 10:55

lalten mentioned this pull request Dec 23, 2024

feat: add iceoryx bazelbuild/bazel-central-registry#1274

Merged

lalten mentioned this pull request Mar 13, 2025

Add [email protected] bazelbuild/bazel-central-registry#4035

Merged

lalten force-pushed the iox-#2370 branch from 41faebb to 5f86388 Compare March 14, 2025 10:38

iox-eclipse-iceoryx#2370: Fix Bzlmod dev_dependency setup

5a207da

lalten force-pushed the iox-#2370 branch from 5f86388 to 5a207da Compare March 14, 2025 10:41

iox-#2370 Fix Bzlmod dev_dependency setup #2371

Are you sure you want to change the base?

iox-#2370 Fix Bzlmod dev_dependency setup #2371

Uh oh!

Conversation

lalten commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes for Reviewer

Pre-Review Checklist for the PR Author

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

Uh oh!

codecov bot commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

elfenpiff commented Nov 18, 2024

Uh oh!

elBoberido commented Nov 18, 2024

Uh oh!

lalten commented Nov 18, 2024

Uh oh!

elBoberido commented Nov 18, 2024

Uh oh!

lalten commented Nov 18, 2024

Uh oh!

lalten commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lalten commented Nov 18, 2024

Uh oh!

elBoberido commented Nov 18, 2024

Uh oh!

lalten commented Mar 14, 2025

Uh oh!

lalten commented Mar 14, 2025

Uh oh!

lalten commented Mar 14, 2025

Uh oh!

elBoberido commented Mar 14, 2025

Uh oh!

lalten commented Mar 14, 2025

Uh oh!

elBoberido commented Mar 14, 2025

Uh oh!

lalten commented Mar 14, 2025

Uh oh!

elBoberido commented Mar 14, 2025

Uh oh!

lalten commented Mar 14, 2025

Uh oh!

Uh oh!

lalten commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading

lalten commented Nov 18, 2024 •

edited

Loading