Repository navigation
bazel: adopt module_template layout and policy-owned features - #625
Conversation
|
Documentation preview for this pull request is available at: |
2367648 to
6bb1468
Compare
1d65bd4 to
9af3f19
Compare
58e43cf to
de66e43
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Central configuration adoption and warning-feature injection remain incomplete, and touched tests lack required traceability metadata.
Review effort: Balanced
Findings: 1
Open (20)
Use try-import for the optional central toolchain configuration · New Adopt the central S-CORE toolchain section unchanged · New Inject default warning features into GCC, QCC, and LLVM toolchains · New Inject warning policy features into the 1.0.3 integration toolchain · New Preserve the upstream score toolchain rc file unchanged · New Add required verification metadata to the touched gtest · New Add required verification metadata to the touched gtest · New Add required verification metadata to the touched gtest · New Add required verification metadata to the touched gtest · New Add runtime verification metadata inside the parameterized gtest · New Replace legacy test metadata with compliant verification metadata · New Replace legacy test metadata with compliant verification metadata · New Avoid implementation-defined conversion of 0xFFFFFFFF to int32_t · New Replace legacy test metadata with compliant verification metadata · New Replace legacy test metadata with compliant verification metadata · New Add required verification metadata to the touched gtest · New Replace legacy test metadata with compliant verification metadata · New Add required verification metadata to the touched gtest · New Rename mock_mut to mock_mutex · New Rename the lambda variable to follow snake_case · New
What changed in this PR
Migrates Baselibs toward the module_template Bazel layout and policy-owned compiler features, with related warning fixes.
Changes:
- Splits Bazel platform, sanitizer, and lint configuration into dedicated files.
- Renames platform configs while preserving deprecated aliases.
- Injects warning/sanitizer features and resolves newly exposed C++ warnings.
| File | Description |
|---|---|
tools/coverage/coverage.bazelrc |
Updates config name and warning exclusions. |
tools/BUILD |
Adds REPO.bazel to copyright checks. |
score/utils/BUILD |
Removes obsolete warning features. |
score/string_manipulation/BUILD |
Removes obsolete warning features. |
score/string_manipulation/arguments/BUILD |
Removes obsolete warning features. |
score/static_reflection_with_serialization/visitor/BUILD |
Updates warning feature set. |
score/static_reflection_with_serialization/serialization/BUILD |
Updates warning feature set. |
score/os/utils/qnx/resource_manager/tests/stubs/simple_read_function_stub.cpp |
Makes offset conversion explicit. |
score/os/utils/qnx/resource_manager/tests/simple_read_function_test.cpp |
Makes test offsets constant. |
score/os/utils/qnx/resource_manager/src/simple_read_function.cpp |
Adds explicit QNX type conversions. |
score/os/toolchain_features.bzl |
Retains only the strict warning feature. |
score/os/test/unistdTest.cpp |
Updates invalid UID construction. |
score/os/test/socket_test.cpp |
Casts received message length. |
score/os/test/qnx/neutrino_impl_test.cpp |
Adjusts signed QNX API values. |
score/os/test/qnx/mman_test.cpp |
Adds explicit QNX handle/address casts. |
score/os/test/qnx/fs_crypto_impl_test.cpp |
Removes an unused variable. |
score/os/qnx/procmgr_impl.cpp |
Reports the current errno. |
score/os/mocklib/ifaddrs_mock.cpp |
Casts address-family type explicitly. |
score/network/test/BUILD |
Removes obsolete warning features. |
score/network/sock_async/BUILD |
Removes obsolete warning features. |
score/network/sock_async_udp/BUILD |
Removes obsolete warning features. |
score/network/sock_async_tcp/BUILD |
Removes obsolete warning features. |
score/network/sock_async_raw/BUILD |
Removes obsolete warning features. |
score/network/mocklib/BUILD |
Removes obsolete warning features. |
score/network/BUILD |
Removes obsolete warning features. |
score/memory/BUILD |
Removes obsolete warning features. |
score/language/safecpp/toolchain_features.bzl |
Documents policy-owned warning features. |
score/language/safecpp/aborts_upon_exception/BUILD |
Removes obsolete warning features. |
score/json/internal/parser/vajson/vajson_impl/util/number.h |
Makes numeric comparison promotions explicit. |
score/json/internal/parser/vajson/unit_test/ct_vajson_v2_parsers.cpp |
Clarifies a parameter name. |
score/hash/code/sha256digest/BUILD |
Removes obsolete warning features. |
score/hash/code/openssl/openssl_wrapper/BUILD |
Updates warning feature set. |
score/hash/code/openssl/BUILD |
Updates warning feature set. |
score/hash/code/crc/BUILD |
Removes obsolete warning features. |
score/hash/code/core/factory/impl/BUILD |
Updates warning feature set. |
score/hash/code/core/factory/BUILD |
Updates warning feature set. |
score/hash/code/core/BUILD |
Updates warning feature set. |
score/hash/code/common/BUILD |
Updates warning feature set. |
score/hash/BUILD |
Updates warning feature set. |
score/flatbuffers/test/generate_json_schema/BUILD |
Updates documented build command. |
score/filesystem/filestream/file_factory_test.cpp |
Adds explicit UID/GID conversions. |
score/filesystem/filestream/file_factory_test_extra.cpp |
Adds explicit UID/GID conversions. |
score/concurrency/locked_ptr_test.cpp |
Renames warning-triggering test variables. |
score/bitmanipulation/BUILD |
Removes obsolete warning features. |
score.bazelrc/score_toolchain.bazelrc |
Adds platform configs and aliases. |
score.bazelrc/score_sanitizers.bazelrc |
Extracts sanitizer configuration. |
score.bazelrc/score_lint.bazelrc |
Extracts lint configuration. |
REPO.bazel |
Enables repository warning policy. |
README.md |
Documents renamed platform configs. |
MODULE.bazel.lock |
Records updated dependencies and features. |
MODULE.bazel |
Upgrades and configures C++ toolchains. |
examples/integration/MODULE.bazel.lock |
Updates integration dependency resolution. |
examples/integration/MODULE.bazel |
Upgrades integration toolchains. |
AGENTS.md |
Updates Bazel configuration guidance. |
.github/workflows/sanitizers_linux.yml |
Uses the renamed Linux config. |
.github/workflows/lint.yml |
Uses the renamed Linux config. |
.github/workflows/build_qnx.yml |
Renames QNX matrix configs. |
.github/workflows/build_linux.yml |
Renames Linux matrix configs. |
.bazelrc |
Imports the extracted Bazel configurations. |
.agents/skills/requirements-management-skill/test-to-requirement-linking.md |
Updates the documented test command. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| WARNING_FEATURES = [ | ||
| "@score_cpp_policies//warnings/gcc/features:minimal_warnings", | ||
| "@score_cpp_policies//warnings/gcc/features:strict_warnings", | ||
| "@score_cpp_policies//warnings/gcc/features:all_wall_warnings", | ||
| "@score_cpp_policies//warnings/gcc/features:warnings_as_errors", |
There was a problem hiding this comment.
Actually, the extra_enabled_features is maybe better than the REPO.bazel and .bazelrc approach.
There was a problem hiding this comment.
The current solution allows -Werror plus all the features in baselibs without breaking third party code compilation (because that code may emit warnings that we have zero control about).
| bazel_dep(name = "score_bazel_platforms", version = "1.1.0") | ||
|
|
||
| bazel_dep(name = "score_bazel_cpp_toolchains", version = "0.5.4", dev_dependency = True) | ||
| bazel_dep(name = "score_bazel_cpp_toolchains", version = "1.0.3", dev_dependency = True) |
There was a problem hiding this comment.
@copilot Create a github issue to address this separately.
There was a problem hiding this comment.
I can’t create GitHub issues with the available tools in this session. Please open a separate issue for the integration-workspace warning-policy change.
| score::Result<size_t> SimpleReadFunctionStub::read(const off_t offset, const size_t nbytes, uint64_t& result) | ||
| { | ||
| if ((offset * nbytes) >= data_.size()) | ||
| if ((static_cast<size_t>(offset) * nbytes) >= data_.size()) |
There was a problem hiding this comment.
off_t is a signed integer. It looks wrong to cast it to unsigned size_t
There was a problem hiding this comment.
this caused compilation failure due to a warning but it's an actual problem that requires a fix instead of silencing
| # flush coverage counters (only active under --config=llvm_cov). | ||
| "@score_coverage//:enable_llvm_coverage_for_death_tests", | ||
| ], | ||
| ] + WARNING_FEATURES + LLVM_SANITIZER_FEATURES, |
There was a problem hiding this comment.
According to @nradakovic it's a bad idea to inject "@score_cpp_policies//warnings/gcc/features:* to LLVM toolchain as these features are created specifically for GCC. But in this PR the LLVM-cov still works fine, so maybe it's ok 🤷♂️
There was a problem hiding this comment.
that's true, but clang ignores the unknown feature flags, so it doesn't matter much.
| # The injected flags come after a target's own copts, so enabling the warning levels globally | ||
| # would defeat the `-w` and `-Wno-error` opt-outs of third-party code. Only levels belong here; | ||
| # a REPO.bazel travels with the sources, so `warnings_as_errors` stays in .bazelrc. | ||
| repo(features = [ |
There was a problem hiding this comment.
I'm not very familiar with the REPO.bazel.
- How does it work when the module is sourced from a bazel registry? Does the
REPO.bazelhave any effect? - How is it better than enabling features in
.bazelrc?
There was a problem hiding this comment.
Check also the comment about the extra_enabled_features.
There was a problem hiding this comment.
This is basically fixing the problem that we had (e.g. with googlebench) where the warning flags were also applied for the third party code (which happens when enabling features in .bazelrc), effectively this allowed removal of this workaround
I don't see how there would be a difference when consuming baselibs from bazel registry.
Split the monolithic .bazelrc into score.bazelrc/ files and rename the platform configs to the score-<os>-<cpu> scheme. The old bl-* names are kept as aliases. Fixes #455
The next commit activates strict_warnings, which adds -Wdouble-promotion on Linux and -Wshadow, -Wconversion and -Wsign-conversion on QNX. The vajson number cast promotes a float to double in the comparison and three tests shadow an outer local or a fixture member. The rest is signedness: uid_t and gid_t are signed on QNX, procmgr_value_current reports failure as UINT64_MAX, and the QNX-only resource manager mixes off_t with the unsigned offset of _xtype_offset. Cast explicitly, or take the error from errno. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
score_bazel_cpp_toolchains 1.0.0 removed its built-in features, so bump to 1.0.3 and inject the policy-owned ones via extra_known_features. Without that the feature names silently do nothing. The injected flags come after a target's own copts, so the warning levels are enabled in REPO.bazel, for this repository only. warnings_as_errors stays in .bazelrc, which is read for the root module only and keeps -Werror out of consumers' builds. The policy covers GCC only, so clang-tidy and llvm_cov drop strict_warnings and warnings_as_errors. Dropping the global -strict_warnings negation from .bazelrc activates strict warnings for the first time; it used to override the per-target features attribute. Closes #519 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
treat_warnings_as_errors, additional_warnings and strict_warnings_no_error were never defined by any toolchain and were silently ignored.
de66e43 to
f32ed4f
Compare



Split .bazelrc into score.bazelrc/ and rename the platform configs to
score--; the bl-* names stay as aliases.
score_bazel_cpp_toolchains 1.0.0 dropped its built-in features, so bump to
1.0.3 and inject the score_cpp_policies ones. Same reason the dead names go
from COMPILER_WARNING_FEATURES. Strict warnings are now actually active,
hence the vajson number cast fix.
Closes #455
Closes #519