bazelbuild / bazelbuild/rules_cc

Misleading toolchain API documentation for requires_true, requires_false

Open
#617 0 comments 0 reactions 0 assignees View on GitHub
category: toolchains P3 type: bug
Dominant language
Starlark
Stars
247
Forks
196
PR merge metrics
No merged PRs in 30d

Description

While configuring a toolchain, we were trying to override the legacy `fully_static_link` feature to always be enabled, except when the variable `is_cc_test` is true or if the target needs runtime libraries.

The [documentation](https://github.com/bazelbuild/rules_cc/blob/01a1eced1d61670e3fe1310ccaaa45635aaf90b1/docs/toolchain_api.md#cc_args-requires_true) for `requires_true` (`requires_false`) says that the given variable will be checked for truthiness (falsiness). so we were trying to use them in the following setup:

```
cc_nested_args(
name = "if_not_cc_test",
args = ["-static"],
requires_false = "@rules_cc//cc/toolchains/variables:is_cc_test",
)

cc_args(
name = "fully_static_args",
actions = [
"@rules_cc//cc/toolchains/actions:link_actions",
],
nested = [":if_not_cc_test"],
requires_false = "@rules_cc//cc/toolchains/variables:runtime_library_search_directories",
)

cc_feature(
name = "fully_static_link",
args = [":fully_static_args"],
overrides = "@rules_cc//cc/toolchains/features/legacy:fully_static_link",
visibility = ["//visibility:public"],
)
```

We were expecting `requires_false = "@rules_cc//cc/toolchains/variables:runtime_library_search_directories"` to work, as Bazel's `expand_if_true`/`expand_if_false` [evaluate an empty sequence](https://github.com/bazelbuild/bazel/blob/3c0915e46491e7490154dd07cb18ff03d02c05bc/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java#L619) to `false`, however we get an error as the current implementation [restricts the allowed types to `bool`](https://github.com/bazelbuild/rules_cc/blob/01a1eced1d61670e3fe1310ccaaa45635aaf90b1/cc/toolchains/impl/nested_args.bzl#L247).

Patching `nested_args.bzl` to accept `list` as well makes our setup work.

We think either the documentation should be updated to reflect that only variables of type `bool` are accepted, or the implementation should match what `expand_if_true`/`expand_if_false` do.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.