Prototypical Bazel Integration

[christophruethingbmw]

No description provided.

[the-nick-fischer]

Here's some first level feedback. Looks good in principle, but there's some general concerns/issues I would like to raise. I will add more feedback over the next days, just wanted to get a disussion started.

[the-nick-fischer]

Is there a specific reason to pick this version? If there's no specific requirements, I would advise picking a LTS release from here: https://bazel.build/release:

• The latest LTS version 9.0.2 was released very recently and is not stable yet (rules_cc support).
• I would suggest picking the latest maintenance LTS version instead: 8.6.0 (End of support Dec 2027)
• I would avoid using version 7.X.X. since there have been some big, breaking changes 7.X.X -> 8.X.X (BAZELMOD, cc_toolchains,...). If we really need to stick with it, at least pick the matching LTS version: 7.7.1 (End of support Dec 2026)

[christophruethingbmw]

@the-nick-fischer , so there was no major reason, it's just that in internal projects we used version 7.6 and thus started from there. I personally would also like to go with the latest version 8 (since as you say 9 is still very new), but we have to see about compatability in case a project who wants to integrate might only support bazel 7.

[the-nick-fischer]

These flags globally apply to any kind of build command you issue to bazel.
Instead, they should be applied on a per toolchain basis using features / flag_sets

[christophruethingbmw]

Yes, for sure, I think that was only done as a quick fix since toolchains are not yet properly integrated in this prototypical PR. We already have examples of proper toolchain setups which we can share.

[the-nick-fischer]

These architecture flags need to be tied to the specific S32K148 toolchain not set globaly here. They can be part of a default_compile_flags feature / flag_set and will be set when the toolchain is selected via toolchain resolution.

[christophruethingbmw]

Exactly, bascially the same point as above I think, needs to be properly integrated into the toolchain. As said, it's not included in this PR but we can share a properly working toolchain setup during the process of our bazel integration :)

[the-nick-fischer]

• --define is a legacy Bazel mechanism for conditional configuration. select() should be used for conditional configuration instead.
• select() is an appropriate tool to handle variation that is determined by: platform, OS, CPU, or compiler specifics or similar. In those cases, the build graph remains static and fully queryable.
• They SHOULD NOT be used for application-level configuration like middleware. Instead explicit target definitions should be used to keep the build graph simple, static and fully queryable.

[the-nick-fischer]

These preprocessor defines / flags should also be tied to the toolchains directly. E.g. default_compile_flags feature

[the-nick-fischer]

This should not be defined globally but instead using dedicated test targets.
If mock dependencies need to be injected, there's various ways of doing that too. select() is also not such a great idea for that in my opinion

[the-nick-fischer]

In general I think using select() here for variation handling works functionally, but it may not be the best pattern. Disclaimer: I'm biased to avoid select() whenever possible due to past experience.

Problems with current implementation:

• select() logic unnecessarily duplicated across multiple fields (srcs, hdrs, include, deps).
• The current approach does not scale well if other variant dimensions are added later.
• The variant selection here seems more like a global concern to me but is currently encoded locally in each target, which adds a lot of bloat.

Solutions:

1. If we don't have a lot of variants and dimensions, just explicitly define all target variants. Although this adds a bit of overhead when creating the BUILD files it's actually the most readable / easy to understand. Also the build tree stays unconditional.

cc_library(
    name = "bspInterruptsImpl_freertos",
    [...]
    deps = [
        "//libs/bsw/platform",
        "//libs/3rdparty/freeRtos",
    ],
)

cc_library(
    name = "bspInterruptsImpl_threadx",
    [...]
    deps = [
        "//libs/bsw/platform",
        "//libs/3rdparty/threadx:threadX",
    ],
)

2. If you want to keep select(): Define a middleware string_flag

string_flag(
    name = "middleware",
    build_setting_default = "freertos",
    values = ["freertos", "threadx"],
)

config_setting(
    name = "freertos",
    flag_values = {":middleware": "freertos"},
)

config_setting(
    name = "threadx",
    flag_values = {":middleware": "threadx"},
)

load("@bazel_skylib//rules:common_settings.bzl", "string_flag")
cc_library(
    name = "bspInterruptsImpl_freertos",
    [...]
)

cc_library(
    name = "bspInterruptsImpl_threadx",
    [...]
)

alias(
    name = "bspInterruptsImpl",
    # Select only once at the top level
    actual = select({
        "//.../middleware_config:freertos": ":bspInterruptsImpl_freertos",
        "//.../middleware_config:threadx":  ":bspInterruptsImpl_threadx",
        # A custom error should be implemented to avoid cryptic
        # "Would a default condition help?" error.
        # "//.../middleware_config:no_middleware": "no_middleware_error",
        # "//conditions:default": "no_middleware_error",

    }),
)

[christophruethingbmw]

I fully agree, the variant handling is something we have to think of properly, we already use select in other projects and we are also not happy with it always. I think we should define a proper approach for this.

[the-nick-fischer]

Platform dependent compile/link flags should move to the respective toolchain (feature / flag_sets)

[the-nick-fischer]

I'm no compiler expert, but I believe instead strip_include_prefix should be used to make headers accessible without the include prefix here. includes exports headers with -isystem and may have far-reaching effects as per the Bazel documentation:

List of include dirs to be added to the compile line. Subject to ["Make variable"](https://bazel.build/reference/be/make-variables) substitution.
Each string is prepended with the package path and passed to the C++ toolchain for expansion via the "include_paths" CROSSTOOL feature.
A toolchain running on a POSIX system with typical feature definitions will produce -isystem path_to_package/include_entry.
This should only be used for third-party libraries that do not conform to the Google style of writing #include statements.
Unlike [COPTS](https://bazel.build/reference/be/c-cpp#cc_binary.copts), these flags are added for this rule and every rule that depends on it. (Note: not the rules it depends upon!)
Be very careful, since this may have far-reaching effects.
When in doubt, add "-I" flags to [COPTS](https://bazel.build/reference/be/c-cpp#cc_binary.copts) instead.
The added include paths will include generated files as well as files in the source tree.

[christophruethingbmw]

Fully agree, I always use strip_include_prefix, I think it had some issues in the past with being not properly propagated through dependencies but I think that's resolved already.