feat: Add default_preprocessopts and preprocessopts for C preprocessor flags - #110
Conversation
jacky8hyf
left a comment
jacky8hyf
left a comment
There was a problem hiding this comment.
re default_copts:
This is the simple path to resolve this (custom flags to the preprocessor), but IMHO, a more decoupled path should be:
(1) In the registered toolchain, add flags for the action "preprocess_assemble"
(2) Here, we retrieve these flags from cc_common, e.g. via get_memory_inefficient_command_line(), and feed them in.
Reason: the preprocessor binary comes from the C toolchain, so any flags to invoke it should also come from the C toolchain, not in the DT toolchain.
If my comment is not clear, feel free to open an issue in https://github.com/bazel-contrib/rules_devicetree/issues and we can discuss there.
OTOH, the mechanism for copts looks okay. Though I am not sure about the naming. This is not the C language. We are just using the C preprocessor to preprocess, as an implementation detail. But OTOH, preprocessopts or preprocess_opts looks unreadable/long. @Gansito144 WDYT?
|
On a second thought, @UebelAndre please remove the |
It's actually |
…essor flags `devicetree_toolchain()` already had `default_dtcopts`, but the C preprocessor invocation in `dtb()` and `dtbo()` had no equivalent at either level. Its command line was hardcoded, and it resolves only the tool for `ACTION_NAMES.preprocess_assemble` without building a command line from that action's flag sets, so there was no way to pass a `-D` to DTS preprocessing at all. `devicetree_toolchain(default_preprocessopts=)` now sets project-wide flags and `dtb(preprocessopts=)`/`dtbo(preprocessopts=)` append to them, mirroring the existing `default_dtcopts`/`dtcopts` pair. Both are placed after `-undef -D__DTS__`, so `-undef` does not discard them, and before `-x assembler-with-cpp`, so the flags cannot change the preprocessing mode. Setting either one when the source is not preprocessed is an error rather than a no-op. The two cases are a toolchain with `preprocess = False`, and a toolchain that leaves preprocessing optional with no CC toolchain registered. Dropping the flags silently would build a devicetree that does not match what they ask for. The preprocess action now carries a `DtPreprocess` mnemonic so it can be identified without matching on output filenames. `preprocessopts_test` asserts the resulting flag order by inspecting the preprocess action's `argv`, and `preprocessopts_error_test` asserts the failure above. Both reach a purpose-built toolchain through the `--extra_toolchains` config setting, which avoids registering a test toolchain module-wide.
Do you have a custom CC toolchain, or an automatically configured one? If you have the former I would suggest doing it there, but if you have the latter it makse sense to add this |
It's custom (the former). But I don't want to alter my toolchain just for flags added to |
UebelAndre
changed the title
default_copts and copts for C preprocessor flagsdefault_preprocessopts and preprocessopts for C preprocessor flags
jacky8hyf
left a comment
jacky8hyf
left a comment
There was a problem hiding this comment.
You convinced me. However, I still think DtPreprocess should both take the flags from the CC toolchain, and be configurable with default_copts. But your change alone is good enough for the latter -- We can do the "take flags from the CC toolchain" later.
|
Waiting for @Gansito144 for a second review (in a different time zone) @Gansito144 -- if this looks good to you, please click the "Merge when ready" green button. Thanks! |
|
@jacky8hyf @Gansito144 would it be possible to get a release for this change? |
It's automatic (once a day) and it is kicked off just now: https://github.com/bazel-contrib/rules_devicetree/actions/runs/35135260809 |
|
Actually -- it is once per two-weeks, so next time would be at the end of September I think. https://github.com/bazel-contrib/rules_devicetree/blob/main/.github/workflows/tag.yaml#L33 Do you urgently need it? If so I can trigger the workflow manually now |
3 participants
devicetree_toolchain()already haddefault_dtcopts, but the C preprocessor invocation indtb()anddtbo()had no equivalent at either level. Its command line was hardcoded, and it resolves only the tool forACTION_NAMES.preprocess_assemblewithout building a command line from that action's flag sets, so there was no way to pass a-Dto DTS preprocessing at all.devicetree_toolchain(default_preprocessopts=)now sets project-wide flags anddtb(preprocessopts=)/dtbo(preprocessopts=)append to them, mirroring the existingdefault_dtcopts/dtcoptspair.Both are placed after
-undef -D__DTS__, so-undefdoes not discard them, and before-x assembler-with-cpp, so the flags cannot change the preprocessing mode.Setting either one when the source is not preprocessed is an error rather than a no-op. The two cases are a toolchain with
preprocess = False, and a toolchain that leaves preprocessing optional with no CC toolchain registered. Dropping the flags silently would build a devicetree that does not match what they ask for.The preprocess action now carries a
DtPreprocessmnemonic so it can be identified without matching on output filenames.preprocessopts_testasserts the resulting flag order by inspecting the preprocess action'sargv, andpreprocessopts_error_testasserts the failure above. Both reach a purpose-built toolchain through the--extra_toolchainsconfig setting, which avoids registering a test toolchain module-wide.