erenon / bazel_clang_tidy

Run clang-tidy on Bazel C++ targets directly, efficiently, with caching enabled
MIT License
109 stars 58 forks source link

Filter Rust targets from aspect #45

Closed kyle-figure closed 1 year ago

kyle-figure commented 1 year ago

rust_library rules contain the CcInfo provider, so the bazel aspect does not filter rust_library targets and subsequently fails the build. This PR adds a simple check for the CrateInfo provider, which seems to be the right way to filter out Rust rules.

erenon commented 1 year ago

Thanks, this looks interesting. Why does it fail to build if a rust crate is encountered? Shouldn't it just skip the rs files?

I have two concerns: what if a rust crate is built from cpp sources? Second: this adds a rust dependency to a cpp focused library, which is unfortunate.

kyle-figure commented 1 year ago

clang-tidy binary does not seem check the file extension before executing on a file. You could pass it any arbitrary file and it will fail unless it's a c++ file.

  1. You cannot build a rust_library from cpp sources. I believe the proper bazel way of linking cpp object files to a rust binary would be to build a separate cc_library target and add that as a dependency to a rust_binary target. Here is the error message when passing a non-rust file in the srcs attribute of a rust_library target: source file '//hello_lib_rs:lib.cpp' is misplaced here (expected .rs)
  2. We can remove the dependency on rules_rust and filter .rs files by changing _rules_sources to only add a src if it doesn't end with .rs:
    def _rule_sources(ctx):
    srcs = []
    if hasattr(ctx.rule.attr, "srcs"):
        for src in ctx.rule.attr.srcs:
            srcs += [src for src in src.files.to_list() if src.is_source and not src.basename.endswith(".rs")]
    return srcs

    Let me know if this is preferable and I will make the change.

erenon commented 1 year ago

I think filtering based on the source extension would be preferable. Also, I'd prefer to opt-in the sources that bazel allows (https://bazel.build/reference/be/c-cpp#cc_binary.srcs) instead of removing .rs files. This would work for other languages. Thanks!

erenon commented 1 year ago

Thanks!