google / emboss

Emboss is a tool for generating code that reads and writes binary data structures.
Apache License 2.0
71 stars 21 forks source link

Bazel: fatal error: 'runtime/cpp/emboss_cpp_util.h' file not found #108

Closed BenjaminLawson closed 7 months ago

BenjaminLawson commented 7 months ago

Clang seems unable to find the cpp_utils headers when the emboss_cc_library rule is used.

bazel-out/k8-fastbuild/bin/external/pigweed/pw_bluetooth/public/pw_bluetooth/hci_common.emb.h:12:10: fatal error: 'runtime/cpp/emboss_cpp_util.h' file not found
   12 | #include "runtime/cpp/emboss_cpp_util.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
BenjaminLawson commented 7 months ago

It seems like private_hdrs passed to cc_common.compile() in _cc_emboss_aspect_impl are not working for a repo outside of emboss. I wonder why this is working in other projects. Maybe a different version of Bazel?

BenjaminLawson commented 7 months ago

cc_common.compile() appears to no longer even be supported in Bazel 7: https://bazel.build/rules/lib/toplevel/cc_common

I am using 6.4.0 though.

reventlov commented 7 months ago

Your team may be the only active Emboss + Bazel users.

I am tempted to just revert 378cd5dcdb0e8baa74c3e8ac5815727d53de4328 and handle the import-of-imports-across-projects problem a different way, because it really feels like the aspect stuff isn't quite stable enough, and it doesn't seem to be working quite the way I thought it did.

There are some non-public issues, as well.

BenjaminLawson commented 7 months ago

This is the depset that is passed as private_hdrs to cc_common.compile:

depset([<source file runtime/cpp/emboss_arithmetic.h>, <source file runtime/cpp/emboss_arithmetic_all_known_generated.h>, <source file runtime/cpp/emboss_arithmetic_maximum_operation_generated.h>, <source file runtime/cpp/emboss_array_view.h>, <source file runtime/cpp/emboss_bit_util.h>, <source file runtime/cpp/emboss_constant_view.h>, <source file runtime/cpp/emboss_cpp_types.h>, <source file runtime/cpp/emboss_cpp_util.h>, <source file runtime/cpp/emboss_defines.h>, <source file runtime/cpp/emboss_enum_view.h>, <source file runtime/cpp/emboss_maybe.h>, <source file runtime/cpp/emboss_memory_util.h>, <source file runtime/cpp/emboss_prelude.h>, <source file runtime/cpp/emboss_text_util.h>, <source file runtime/cpp/emboss_view_parameters.h>, <generated file external/pigweed/pw_bluetooth/public/pw_bluetooth/hci_common.emb.h>, <generated file external/pigweed/pw_bluetooth/public/pw_bluetooth/hci_vendor.emb.h>])
BenjaminLawson commented 7 months ago

I found a terrible workaround. If I prefix all internal emboss C++ header includes with "external/com_googletest_emboss", then clang can find the headers:

#include "external/com_google_emboss/runtime/cpp/emboss_cpp_util.h"
tpudlik commented 7 months ago

Another workaround might be to add the includes = ["."] attribute to the cpp_utils cc_library.

@comius I suspect this should not be required, but there's a bug in how the Emboss build rules use cc_common. Could you take a look? Maybe the error will be obvious to you!

BenjaminLawson commented 7 months ago

I found what seems to be a fix:

    runtime_cc_info = ctx.attr._emboss_cc_runtime[CcInfo]
    transitive_headers = depset(
        direct = headers,
        transitive = [
                         dep[EmbossCcHeaderInfo].transitive_headers
                         for dep in ctx.rule.attr.deps
                     ],
    )
    (cc_compilation_context, cc_compilation_outputs) = cc_common.compile(
        name = ctx.label.name,
        actions = ctx.actions,
        feature_configuration = feature_configuration,
        cc_toolchain = cc_toolchain,
        public_hdrs = headers,
        private_hdrs = transitive_headers.to_list(),
        compilation_contexts = [runtime_cc_info.compilation_context]
    )

Instead of adding the cpp_utils headers to private_hdrs, add the cpp_utils compilation context to cc_common.compile.

reventlov commented 7 months ago

I'm happy to go with the compilation context fix, if that actually works. Hopefully that is the end of the problems, at least for now.