bazelbuild / bazel-skylib

Common useful functions and rules for Bazel
https://bazel.build/
Apache License 2.0
394 stars 180 forks source link

Example/docs for string_list_setting #296

Open Michaelhobo opened 3 years ago

Michaelhobo commented 3 years ago

Is there more documentation (preferably with examples) for using string_list_setting? I'm trying to pass defines to a cc_library rule, like this:

cc_library(
  name = "...",
  defines = ":nrf_defines",
)

string_list_setting(
  name = "nrf_defines",
  build_setting_default = [],
)

When I build this, cc_library rules spit out the error:

expected value of type 'list(string)' for attribute 'defines' in 'cc_library' rule, but got ":nrf_defines" (string)

This also makes sense, since the types are different, but I don't know how to proceed. How do I convert the string_list_setting into a list of strings when filling in the defines field of a cc_library?

tetromino commented 3 years ago

Fair point - we don't have much documentation for string_list_setting, or really for any part of common_settings.

Pinging @gregestren from Bazel Configurability for advice.

gregestren commented 3 years ago

https://github.com/bazelbuild/examples/tree/master/rules/starlark_configurations is probably the best spot for examples, linked from the docs at https://docs.bazel.build/versions/master/skylark/config.html#predefined-settings.

In your case, the biggest challenge is that Starlark settings (which string_list_setting is) work most thoroughly with Starlark rules. cc_library is a builtin rule, so it doesn't know how to unpack them.

There are some workarounds, but it depends on exactly what you want to do. how many values do you imagine setting in the defines? Anything arbitrary? Just a pre-known set of possibilities?

Michaelhobo commented 3 years ago

There are some workarounds, but it depends on exactly what you want to do. how many values do you imagine setting in the defines? Anything arbitrary? Just a pre-known set of possibilities?

There are a number of different options (10+ different defines) that are set using -D in the nrf52 SDK, so there are technically a lot of possibilities. These include enabling timers and other features, stack/heap size, board model, as well as user-defined settings. Once set, I don't think they change very often though, so if we have to boil the possibilities down into a set of common ones, that's a possibility, as long as the user can change them.

What workarounds are you thinking of?

Michaelhobo commented 3 years ago

I see the selectable copts in the starlark_configurations examples: https://github.com/bazelbuild/examples/tree/master/rules/starlark_configurations/cc_binary_selectable_copts

Since the cc_binary will be in different places than the cc_library rules, I'd prefer the list of defines be next to the cc_binary, so it's clear what is getting set.

I already have a custom nrf_cc_binary rule that sets a number of transitions, which looks something like this, except the nrf_defines field doesn't work. I'd like the nrf_defines to pass the string list to its dependencies inside the nrf52 SDK.

nrf_cc_binary(
    name = "ble_app_blinky_c",
    srcs = ["main.c"],
    copts = [
        "-mcpu=cortex-m4",
        "-mthumb",
        "-mabi=aapcs",
        "-Wall",
        "-Werror",
        "-mfloat-abi=hard",
        "-mfpu=fpv4-sp-d16",
        "-ffunction-sections",
        "-fdata-sections",
        "-fno-strict-aliasing",
        "-fno-builtin",
        "-fshort-enums",
    ],
    nrf_defines = [
        "APP_TIMER_V2",
        "APP_TIMER_V2_RTC1_ENABLED",
        "BOARD_PCA10040",
        "CONFIG_GPIO_AS_PINRESET",
        "FLOAT_ABI_HARD",
        "NRF52",
        "NRF52832_XXAA",
        "NRF52_PAN_74",
        "NRF_SD_BLE_API_VERSION=7",
        "S132",
        "SOFTDEVICE_PRESENT",
        "__HEAP_SIZE=0",
        "__STACK_SIZE=8192",
    ],
    remap = {
        "sdk_config.h": "//nRF5_SDK_17.0.0_9d13099/examples/ble_central/ble_app_blinky_c/pca10040/s132/config:sdk_config",
    },
    deps = [
        "//nRF5_SDK_17.0.0_9d13099/components/ble/ble_advertising",
        "//nRF5_SDK_17.0.0_9d13099/components/ble/ble_db_discovery",
        "//nRF5_SDK_17.0.0_9d13099/components/ble/ble_services/ble_lbs_c",
        "//nRF5_SDK_17.0.0_9d13099/components/ble/common:ble_conn_params",
        "//nRF5_SDK_17.0.0_9d13099/components/ble/nrf_ble_gatt",
        "//nRF5_SDK_17.0.0_9d13099/components/ble/nrf_ble_scan",
        "//nRF5_SDK_17.0.0_9d13099/components/boards",
        "//nRF5_SDK_17.0.0_9d13099/components/libraries/bsp",
        "//nRF5_SDK_17.0.0_9d13099/components/libraries/bsp:bsp_btn_ble",
        "//nRF5_SDK_17.0.0_9d13099/components/libraries/log:nrf_log",
        "//nRF5_SDK_17.0.0_9d13099/components/libraries/log:nrf_log_ctrl",
        "//nRF5_SDK_17.0.0_9d13099/components/libraries/log:nrf_log_default_backends",
        "//nRF5_SDK_17.0.0_9d13099/components/libraries/pwr_mgmt:nrf_pwr_mgmt",
        "//nRF5_SDK_17.0.0_9d13099/components/libraries/timer:app_timer",
        "//nRF5_SDK_17.0.0_9d13099/components/softdevice/common:nrf_sdh",
        "//nRF5_SDK_17.0.0_9d13099/components/softdevice/common:nrf_sdh_ble",
        "//nRF5_SDK_17.0.0_9d13099/components/softdevice/common:nrf_sdh_soc",
        "//nRF5_SDK_17.0.0_9d13099/components/softdevice/s132/headers:ble",
        "//nRF5_SDK_17.0.0_9d13099/components/softdevice/s132/headers:ble_hci",
    ],
)

Based on your cc_binary_selectable_copts example, it seems like I could have some file binary_defines.bzl which contains a dictionary BINARY_DEFINES of config_setting to lists of defines. The nrf_cc_binary can set that setting based on a custom ID in a setting, and the cc_library rules can:

load("binary_defines.bzl", "BINARY_DEFINES")

cc_library(
  ...
  defines = select(BINARY_DEFINES),
)

This is not preferable, due to the central binary_defines.bzl file. I'd much prefer the defines sit directly next to the cc_binary for readability.

gregestren commented 3 years ago

The main workaround I was thinking of is simply:

cc_binary / cc_library
    defines = select({...})

which works when the total space of possible values is known and small.

But I don't think that works for your case, nor with wanting to define the values directly next to the cc_binary.

What if you have the transition at your cc_binary set --copt? You should be able to add -Dkey=val values for --copt, and those should be consumed by cc_librarys below just as if they had directly set defines.

You could even trigger the transition on nrf_defines if you still wanted to control the whole thing from nrf_defines specifically.

Michaelhobo commented 3 years ago

What if you have the transition at your cc_binary set --copt? You should be able to add -Dkey=val values for --copt, and those should be consumed by cc_librarys below just as if they had directly set defines.

Ah, that should solve my problem perfectly! Thank you, I guess I should've read the documentation for copts a bit better. I'll try it soon, but I'm pretty sure that'll solve my problem.

Even if my immediate problem is solved, it seems like we still want better documentation - should we leave this issue open for that?

tetromino commented 3 years ago

Yes - a PR improving the docs is still very much welcome

gregestren commented 3 years ago

Agreed.

Michaelhobo commented 3 years ago

Actually, according to this doc, copts doesn't propagate to its dependencies: https://docs.bazel.build/versions/master/be/c-cpp.html#cc_binary

Each string in this attribute is added in the given order to COPTS before compiling the binary target. The flags take effect only for compiling this target, not its dependencies, so be careful about header files included elsewhere. All paths should be relative to the workspace, not to the current package.

I believe that means, if I have this:

nrf_cc_binary(
  copts = [
    "-DA=B",
  ],
)

This will only work for compiling that target, but all the cc_library deps won't have this when it's defined.

Michaelhobo commented 3 years ago

Ok, after reading bazel docs some more, I see there's a --copt tool flag, and I should be able to set it with //command_line_option:copt.