From 32ae6fc5be84c113c7c3a3f6e9ab23f7df86774a Mon Sep 17 00:00:00 2001 From: Chris Novakovic Date: Fri, 28 Nov 2025 14:18:57 +0000 Subject: [PATCH] Don't collect headers and macro definitions transitively from dependencies Header files and macro definitions are currently collected from dependencies transitively, which is too aggressive. The only headers required to compile a C/C++ source file are the ones included by that source file, and the only macros that should be defined on the command line are the ones necessary to compile that source file. Collecting headers transitively pollutes the build environment (and increases the probability of unnecessary header name clashes); collecting macro definitions transitively risks causing action at a distance, especially for macros that change the meaning or behaviour of functions defined in the standard library (e.g. `_XOPEN_SOURCE`). Only collect headers exposed by direct dependencies, and don't collect macro definitions from dependencies at all. Fixes #95. --- build_defs/cc.build_defs | 59 ++++++++++---------- test/BUILD | 9 +++ test/labels/BUILD | 19 ------- test/labels/another_lib.cpp | 0 test/labels/lib1/include/lib.hpp | 3 - test/labels/lib_test.cpp | 9 --- test/transitive_deps_repo/.plzconfig | 2 + test/transitive_deps_repo/BUILD_FILE | 9 +++ test/transitive_deps_repo/binary.c | 16 ++++++ test/transitive_deps_repo/one/BUILD_FILE | 16 ++++++ test/transitive_deps_repo/one/one.c | 19 +++++++ test/transitive_deps_repo/one/one.h | 3 + test/transitive_deps_repo/one/one_private.h | 9 +++ test/transitive_deps_repo/plugins/BUILD_FILE | 4 ++ test/transitive_deps_repo/two/BUILD_FILE | 13 +++++ test/transitive_deps_repo/two/two.c | 10 ++++ test/transitive_deps_repo/two/two.h | 1 + test/transitive_deps_repo/two/two_private.h | 5 ++ 18 files changed, 147 insertions(+), 59 deletions(-) delete mode 100644 test/labels/BUILD delete mode 100644 test/labels/another_lib.cpp delete mode 100644 test/labels/lib1/include/lib.hpp delete mode 100644 test/labels/lib_test.cpp create mode 100644 test/transitive_deps_repo/.plzconfig create mode 100644 test/transitive_deps_repo/BUILD_FILE create mode 100644 test/transitive_deps_repo/binary.c create mode 100644 test/transitive_deps_repo/one/BUILD_FILE create mode 100644 test/transitive_deps_repo/one/one.c create mode 100644 test/transitive_deps_repo/one/one.h create mode 100644 test/transitive_deps_repo/one/one_private.h create mode 100644 test/transitive_deps_repo/plugins/BUILD_FILE create mode 100644 test/transitive_deps_repo/two/BUILD_FILE create mode 100644 test/transitive_deps_repo/two/two.c create mode 100644 test/transitive_deps_repo/two/two.h create mode 100644 test/transitive_deps_repo/two/two_private.h diff --git a/build_defs/cc.build_defs b/build_defs/cc.build_defs index ec41ea0..c7d64b8 100644 --- a/build_defs/cc.build_defs +++ b/build_defs/cc.build_defs @@ -91,7 +91,6 @@ def cc_library(name:str, srcs:list=[], hdrs:list=[], private_hdrs:list=[], deps: ['cc:pc:' + lib for lib in pkg_config_libs] + ['cc:pcc:' + cflag for cflag in pkg_config_cflags] + ['cc:inc:' + join_path(pkg_name, include) for include in includes] + - ['cc:def:' + define for define in defines] + labels) if not srcs and not _interfaces: @@ -120,14 +119,14 @@ def cc_library(name:str, srcs:list=[], hdrs:list=[], private_hdrs:list=[], deps: ) provides = {'cc_hdrs': hdrs_rule} - # TODO(pebers): handle includes and defines in _library_cmds as well. - pre_build = _library_transitive_labels(_c, compiler_flags, pkg_config_libs, pkg_config_cflags) if (deps or includes or defines or _interfaces) else None + # TODO(pebers): handle includes in _library_cmds as well. + pre_build = _library_apply_labels(_c, compiler_flags, pkg_config_libs, pkg_config_cflags, defines) if (deps or includes or _interfaces) else None pkg = package_name() if _interfaces: # Generate the module interface file xflags = _MODULE_FLAGS + ["--precompile", "-x", "c++-module", "-o", '"$OUT"'] - cmds, tools = _library_cmds(_c, compiler_flags + xflags, pkg_config_libs, pkg_config_cflags, archive=False) + cmds, tools = _library_cmds(_c, compiler_flags + xflags, pkg_config_libs, pkg_config_cflags, defines, archive=False) interface_rule = build_rule( name = name, tag = 'interface', @@ -147,7 +146,7 @@ def cc_library(name:str, srcs:list=[], hdrs:list=[], private_hdrs:list=[], deps: else: all_deps = deps - cmds, tools = _library_cmds(_c, compiler_flags, pkg_config_libs, pkg_config_cflags) + cmds, tools = _library_cmds(_c, compiler_flags, pkg_config_libs, pkg_config_cflags, defines) if not out: out = f'{name}.a' if name.startswith('lib') else f'lib{name}.a' if len(srcs) > 1: @@ -296,11 +295,10 @@ def cc_object(name:str, src:str, hdrs:list=[], private_hdrs:list=[], out:str=Non ['cc:pc:' + lib for lib in pkg_config_libs] + ['cc:pcc:' + cflag for cflag in pkg_config_cflags] + [f'cc:inc:{pkg}/{include}' for include in includes] + - ['cc:def:' + define for define in defines] + labels) if alwayslink: labels += ['cc:al:{pkg}/{name}.a'] - cmds, tools = _library_cmds(_c, compiler_flags, pkg_config_libs, pkg_config_cflags, archive=False) + cmds, tools = _library_cmds(_c, compiler_flags, pkg_config_libs, pkg_config_cflags, defines, archive=False) return build_rule( name=name, @@ -314,8 +312,8 @@ def cc_object(name:str, src:str, hdrs:list=[], private_hdrs:list=[], out:str=Non test_only=test_only, labels=labels, tools=tools, - pre_build=_library_transitive_labels(_c, compiler_flags, pkg_config_libs, pkg_config_cflags, archive=False) - if (deps or includes or defines) else None, + pre_build=_library_apply_labels(_c, compiler_flags, pkg_config_libs, pkg_config_cflags, defines, archive=False) + if (deps or includes) else None, needs_transitive_deps=True, ) @@ -821,8 +819,8 @@ def _escape_linker_flag(flag:str): return "-Wl," + flag.replace(" ", ",") -def _library_cmds(c:bool=False, compiler_flags:list=[], pkg_config_libs:list=[], pkg_config_cflags:list=[], extra_flags:list=[], - archive:bool=True): +def _library_cmds(c:bool=False, compiler_flags:list=[], pkg_config_libs:list=[], pkg_config_cflags:list=[], defines:list=[], + extra_flags:list=[], archive:bool=True): """Returns the commands needed for a cc_library rule.""" # Most compilers allow multiple action flags to be specified, with later flags overriding earlier ones. However, # specifying more than one action is an error (-Wunused-command-line-argument) as of Clang 20, so if the compiler @@ -831,8 +829,8 @@ def _library_cmds(c:bool=False, compiler_flags:list=[], pkg_config_libs:list=[], common_flags = [] if any([cf in _ACTION_FLAGS for cf in compiler_flags]) else ["-c"] common_flags += ["-I", "."] - dbg_flags = _build_flags(compiler_flags, pkg_config_libs, pkg_config_cflags, c=c, dbg=True) - opt_flags = _build_flags(compiler_flags, pkg_config_libs, pkg_config_cflags, c=c) + dbg_flags = _build_flags(compiler_flags, pkg_config_libs, pkg_config_cflags, defines, c=c, dbg=True) + opt_flags = _build_flags(compiler_flags, pkg_config_libs, pkg_config_cflags, defines, c=c) cmd_template = '"$TOOLS_PLEASE_CC" cc "$TOOLS_CC" %s ${SRCS_SRCS}' if archive: cmd_template += ' && "$TOOLS_ARCAT" ar -r && "$TOOLS_AR" s "$OUT"' @@ -891,28 +889,33 @@ def _module_file_flag(module_file:str): ) -def _library_transitive_labels(c:bool=False, compiler_flags:list=[], pkg_config_libs:list=[], pkg_config_cflags:list=[], archive:bool=True): - """Applies commands from transitive labels to a cc_library rule.""" - def apply_transitive_labels(name): - labels = get_labels(name, 'cc:') +def _library_apply_labels(c:bool=False, compiler_flags:list=[], pkg_config_libs:list=[], pkg_config_cflags:list=[], + defines:list=[], archive:bool=True): + """Extracts options from the labels of dependencies - transitively, in some cases - and adds them to the command of + a cc_library target. + """ + def apply_labels(name): flags = [] - for l in labels: - if l.startswith("inc:"): - flags += ["-isystem", l[4:]] - elif l.startswith("def:"): - flags += ["-D", l[4:]] - - pkg_config_libs += [l[3:] for l in labels if l.startswith('pc:') and l[3:] not in pkg_config_libs] - pkg_config_cflags += [l[4:] for l in labels if l.startswith('pcc:') and l[4:] not in pkg_config_cflags] - mods = [_module_file_flag(l[4:]) for l in labels if l.startswith('mod:')] + mods = [] + # Only the headers for this library's direct dependencies should be collected - transitive dependencies aren't + # needed at compile time, and just pollute the build environment. + for l in get_labels(name, "cc:inc:", maxdepth=1): + flags += ["-isystem", l] + for l in get_labels(name, "cc:"): + if l.startswith("pc:") and l[3:] not in pkg_config_libs: + pkg_config_libs += [l[3:]] + elif l.startswith("pcc:") and l[4:] not in pkg_config_cflags: + pkg_config_cflags += [l[4:]] + elif l.startswith("mod:"): + mods += [_module_file_flag(l[4:])] flags += mods if mods: flags += _MODULE_FLAGS if flags: # Don't update if there aren't any relevant labels - cmds, _ = _library_cmds(c, compiler_flags, pkg_config_libs, pkg_config_cflags, flags, archive=archive) + cmds, _ = _library_cmds(c, compiler_flags, pkg_config_libs, pkg_config_cflags, defines, flags, archive=archive) for k, v in cmds.items(): set_command(name, k, v) - return apply_transitive_labels + return apply_labels def _binary_transitive_labels(c:bool=False, linker_flags:list=[], pkg_config_libs:list=[], strip:bool=None, shared:bool=False, diff --git a/test/BUILD b/test/BUILD index 8282d4d..d1b76d1 100644 --- a/test/BUILD +++ b/test/BUILD @@ -109,3 +109,12 @@ plugin_e2e_test( "file": "executable" if CONFIG.TARGET_OS == "darwin" else "statically linked", }, ) + +plugin_e2e_test( + name = "transitive_deps_test", + repo = "transitive_deps_repo", + test_cmd = "plz run //:binary > out", + expected_output = { + "out": "one=1 one_plus_one=2", + }, +) diff --git a/test/labels/BUILD b/test/labels/BUILD deleted file mode 100644 index 65ab5ed..0000000 --- a/test/labels/BUILD +++ /dev/null @@ -1,19 +0,0 @@ -subinclude("//build_defs:cc") - -cc_library( - name = "lib1", - hdrs = ["lib1/include/lib.hpp"], - includes = ["lib1/include"], -) - -cc_library( - name = "another_lib", - srcs = ["another_lib.cpp"], - deps = [":lib1"], -) - -cc_test( - name = "lib_test", - srcs = ["lib_test.cpp"], - deps = [":another_lib"], -) diff --git a/test/labels/another_lib.cpp b/test/labels/another_lib.cpp deleted file mode 100644 index e69de29..0000000 diff --git a/test/labels/lib1/include/lib.hpp b/test/labels/lib1/include/lib.hpp deleted file mode 100644 index 932b324..0000000 --- a/test/labels/lib1/include/lib.hpp +++ /dev/null @@ -1,3 +0,0 @@ -inline int GetAnswer() { - return 42; -} diff --git a/test/labels/lib_test.cpp b/test/labels/lib_test.cpp deleted file mode 100644 index 1168b35..0000000 --- a/test/labels/lib_test.cpp +++ /dev/null @@ -1,9 +0,0 @@ -#include - -#include "lib.hpp" - -// The real test is at compilation time, this is here just to -// have a test that proves we really are doing something. -TEST(TheAnswer) { - CHECK_EQUAL(42, GetAnswer()); -} diff --git a/test/transitive_deps_repo/.plzconfig b/test/transitive_deps_repo/.plzconfig new file mode 100644 index 0000000..14ee086 --- /dev/null +++ b/test/transitive_deps_repo/.plzconfig @@ -0,0 +1,2 @@ +[Plugin "cc"] +Target = //plugins:cc diff --git a/test/transitive_deps_repo/BUILD_FILE b/test/transitive_deps_repo/BUILD_FILE new file mode 100644 index 0000000..168b8aa --- /dev/null +++ b/test/transitive_deps_repo/BUILD_FILE @@ -0,0 +1,9 @@ +subinclude("///cc//build_defs:c") + +c_binary( + name = "binary", + srcs = ["binary.c"], + deps = [ + "//one", + ], +) diff --git a/test/transitive_deps_repo/binary.c b/test/transitive_deps_repo/binary.c new file mode 100644 index 0000000..9f2115f --- /dev/null +++ b/test/transitive_deps_repo/binary.c @@ -0,0 +1,16 @@ +#include + +#include "one.h" + +#ifdef BUILDING_LIBONE +#error "BUILDING_LIBONE should not be defined" +#endif + +#ifdef BUILDING_LIBTWO +#error "BUILDING_LIBTWO should not be defined" +#endif + +int main() { + printf("one=%d one_plus_one=%d\n", one(), one_plus_one()); + return 0; +} diff --git a/test/transitive_deps_repo/one/BUILD_FILE b/test/transitive_deps_repo/one/BUILD_FILE new file mode 100644 index 0000000..ac96908 --- /dev/null +++ b/test/transitive_deps_repo/one/BUILD_FILE @@ -0,0 +1,16 @@ +subinclude("///cc//build_defs:c") + +c_library( + name = "one", + srcs = ["one.c"], + hdrs = ["one.h"], + private_hdrs = ["one_private.h"], + defines = ["BUILDING_LIBONE"], + includes = ["."], + visibility = [ + "//:binary", + ], + deps = [ + "//two", + ], +) diff --git a/test/transitive_deps_repo/one/one.c b/test/transitive_deps_repo/one/one.c new file mode 100644 index 0000000..f321993 --- /dev/null +++ b/test/transitive_deps_repo/one/one.c @@ -0,0 +1,19 @@ +#include "one.h" +#include "two.h" +#include "one_private.h" + +#ifndef BUILDING_LIBONE +#error "BUILDING_LIBONE should be defined" +#endif + +#ifdef BUILDING_LIBTWO +#error "BUILDING_LIBTWO should not be defined" +#endif + +int one() { + return ONE_INT; +} + +int one_plus_one() { + return two(); +} diff --git a/test/transitive_deps_repo/one/one.h b/test/transitive_deps_repo/one/one.h new file mode 100644 index 0000000..391b571 --- /dev/null +++ b/test/transitive_deps_repo/one/one.h @@ -0,0 +1,3 @@ +int one(); + +int one_plus_one(); diff --git a/test/transitive_deps_repo/one/one_private.h b/test/transitive_deps_repo/one/one_private.h new file mode 100644 index 0000000..46b3d36 --- /dev/null +++ b/test/transitive_deps_repo/one/one_private.h @@ -0,0 +1,9 @@ +#ifndef BUILDING_LIBONE +#error "BUILDING_LIBONE should be defined" +#endif + +#ifdef BUILDING_LIBTWO +#error "BUILDING_LIBTWO should not be defined" +#endif + +#define ONE_INT 1 diff --git a/test/transitive_deps_repo/plugins/BUILD_FILE b/test/transitive_deps_repo/plugins/BUILD_FILE new file mode 100644 index 0000000..c355374 --- /dev/null +++ b/test/transitive_deps_repo/plugins/BUILD_FILE @@ -0,0 +1,4 @@ +plugin_repo( + name = "cc", + revision = "e2e", +) diff --git a/test/transitive_deps_repo/two/BUILD_FILE b/test/transitive_deps_repo/two/BUILD_FILE new file mode 100644 index 0000000..273707d --- /dev/null +++ b/test/transitive_deps_repo/two/BUILD_FILE @@ -0,0 +1,13 @@ +subinclude("///cc//build_defs:c") + +c_library( + name = "two", + srcs = ["two.c"], + hdrs = ["two.h"], + private_hdrs = ["two_private.h"], + defines = ["BUILDING_LIBTWO"], + includes = ["."], + visibility = [ + "//one", + ], +) diff --git a/test/transitive_deps_repo/two/two.c b/test/transitive_deps_repo/two/two.c new file mode 100644 index 0000000..f841e17 --- /dev/null +++ b/test/transitive_deps_repo/two/two.c @@ -0,0 +1,10 @@ +#include "two.h" +#include "two_private.h" + +#ifndef BUILDING_LIBTWO +#error "BUILDING_LIBTWO should be defined" +#endif + +int two() { + return TWO_INT; +} diff --git a/test/transitive_deps_repo/two/two.h b/test/transitive_deps_repo/two/two.h new file mode 100644 index 0000000..6108197 --- /dev/null +++ b/test/transitive_deps_repo/two/two.h @@ -0,0 +1 @@ +int two(); diff --git a/test/transitive_deps_repo/two/two_private.h b/test/transitive_deps_repo/two/two_private.h new file mode 100644 index 0000000..6b0bff1 --- /dev/null +++ b/test/transitive_deps_repo/two/two_private.h @@ -0,0 +1,5 @@ +#ifndef BUILDING_LIBTWO +#error "BUILDING_LIBTWO should be defined" +#endif + +#define TWO_INT 2