From 22d96cf4372f679530b67e703de077f2e3a69e3a Mon Sep 17 00:00:00 2001 From: Dan Hawson <18214721+GertyP@users.noreply.github.com> Date: Sun, 2 Jul 2023 20:19:55 +0100 Subject: [PATCH] Adds new 'build' and 'source' bool args for include_directories() func. Motivation: Frequent, unnecessary (and maybe even problematic) doubling up of both build and src-relative include search paths as discussed here: https://github.com/mesonbuild/meson/discussions/11919 'build' specifies whether the include dirs should be treated as build- relative and 'src', whether source-relative. They both default to true to keep previous behaviour when unspecified. Added new 'test cases/common/267 include_directories use types' test (should these live under 'common'?). Added documentation and new feature snippet. Respect the new bools in vs2010backend (and adding more type annotations to help readability). --- docs/markdown/snippets/inc_dir_use_type.md | 18 ++++++ docs/yaml/functions/include_directories.yaml | 41 +++++++++++--- mesonbuild/backend/ninjabackend.py | 56 ++++++++++++++----- mesonbuild/backend/vs2010backend.py | 38 +++++++++---- mesonbuild/build.py | 29 +++++++++- mesonbuild/interpreter/interpreter.py | 27 +++++++-- mesonbuild/interpreter/kwargs.py | 3 + .../meson.build | 49 ++++++++++++++++ .../moreheaders/build_only._h | 1 + .../moreheaders/meson.build | 7 +++ .../moreheaders/src_only.h | 1 + .../src/main.cpp | 15 +++++ .../src/main2.cpp | 19 +++++++ .../whereareyoufindingme.h | 1 + unittests/allplatformstests.py | 6 ++ 15 files changed, 272 insertions(+), 39 deletions(-) create mode 100644 docs/markdown/snippets/inc_dir_use_type.md create mode 100644 test cases/common/267 include_directories relative/meson.build create mode 100644 test cases/common/267 include_directories relative/moreheaders/build_only._h create mode 100644 test cases/common/267 include_directories relative/moreheaders/meson.build create mode 100644 test cases/common/267 include_directories relative/moreheaders/src_only.h create mode 100644 test cases/common/267 include_directories relative/src/main.cpp create mode 100644 test cases/common/267 include_directories relative/src/main2.cpp create mode 100644 test cases/common/267 include_directories relative/whereareyoufindingme.h diff --git a/docs/markdown/snippets/inc_dir_use_type.md b/docs/markdown/snippets/inc_dir_use_type.md new file mode 100644 index 000000000000..742e4ebf1496 --- /dev/null +++ b/docs/markdown/snippets/inc_dir_use_type.md @@ -0,0 +1,18 @@ +## Added a new `build` and 'source' args to `include_directories(...)` + +The previous behaviour of `include_directories(...)` is that it ends up adding +both source-relative and build-relative include search paths to the compile +options or, if using absolute paths, then simply duplicating the same absolute +include search path. Even if the user wants _either_ a build-relative _or_ +src-relative path only, they're forced to use both, which could cause problems +as well as just adding unnecessary compile options. + +New `build` and `source` bool arguments are added to `include_directories(...)`. +If unspecified, both `build` and `source` default to True. +It is invalid for both to be False. +It is invalid to use absolute paths with _only_ 'build' set to True, since a +user asking for build-relative absolute include directories is meaningless or +at least suggests a misunderstanding. + +Absolute include search paths are allowed if `source` is `true`. If `build` +is also `true`, any absolute paths will only be added once. diff --git a/docs/yaml/functions/include_directories.yaml b/docs/yaml/functions/include_directories.yaml index 77faeb4f3caf..0b3ed35da4c0 100644 --- a/docs/yaml/functions/include_directories.yaml +++ b/docs/yaml/functions/include_directories.yaml @@ -2,11 +2,11 @@ name: include_directories returns: inc description: | Returns an opaque object which contains the directories (relative to - the current directory) given in the positional arguments. The result - can then be passed to the `include_directories:` keyword argument when - building executables or libraries. You can use the returned object in - any subdirectory you want, Meson will make the paths work - automatically. + the current build and/or source directory) given in the positional + arguments. The result can then be passed to the `include_directories:` + keyword argument when building executables or libraries. You can use + the returned object in any subdirectory you want, Meson will make the + paths work automatically. Note that this function call itself does not add the directories into the search path, since there is no global search path. For something @@ -16,8 +16,18 @@ description: | [[executable]], which adds current source and build directories to include path. - Each directory given is converted to two include paths: one that is - relative to the source root and one relative to the build root. + Each given directory is converted to a build-root-relative include + search path (if `build` is `true`) and/or a source-root-relative + path (if `source` is `true`). + + The default values for `build` and `source` are `true`. It is + invalid for both to be `false`. + + It is invalid to use absolute paths with _only_ 'build' `true`, + since asking for build-relative-only absolute include directories is + meaningless or at least a user misunderstanding. However, absolute + search paths are allowed if `source` is `true`. If `build` is also + `true`, any absolute paths will only be added once. example: | For example, with the following source tree layout in @@ -70,3 +80,20 @@ kwargs: they will be used with the `-isystem` compiler argument rather than `-I` on compilers that support this flag (in practice everything except Visual Studio). + + build: + type: bool + default: true + since: 1.3.0 + description: | + Specifies whether the resultant include search directories should be + treated as build-relative include search directories. + + source: + type: bool + default: true + since: 1.3.0 + description: | + Specifies whether the resultant include search directories should be + treated as source-relative include search directories, while also + allowing absolute directory paths. diff --git a/mesonbuild/backend/ninjabackend.py b/mesonbuild/backend/ninjabackend.py index 3d3eefdcc733..c8e3104ba9d7 100644 --- a/mesonbuild/backend/ninjabackend.py +++ b/mesonbuild/backend/ninjabackend.py @@ -2839,27 +2839,59 @@ def generate_llvm_ir_compile(self, target, src): self.add_build(element) return (rel_obj, rel_src) + # Returns a list of include dir arg strings. + # If 'd' is an absolute path, then there's just one include dir arg. + # If 'build_relative' and 'source_relative' then the list contains the + # src-relative dir and the build-relative dir... IN THIS ORDER - + # [build-relative, src-relative] + # This is because - + # + # - We know that this function is used with CompilerArgs.__iadd_ (+=) + # which has very particular behaviour. + # + # - With include dirs, CompilerArgs '+=' reverses the input sequence and + # prepends the reverse of that again to its internal commands list, so - + # commands += [a] + # commands += [b] + # gives [b,a,...], which is different from - + # commands += [a,b] + # which gives [a,b,...] + # + # Because of this, returning any build-relative path before a src-relative + # path means the build-relative path will come earlier in the final command + # args and so take precedence, which is what we want. + # + # Because this is internal, it's reasonable to assume all 'd' paths have + # checked we don't use absolute paths with ONLY 'build_relative' paths. @lru_cache(maxsize=None) - def generate_inc_dir(self, compiler: 'Compiler', d: str, basedir: str, is_system: bool) -> \ - T.Tuple['ImmutableListProtocol[str]', 'ImmutableListProtocol[str]']: + def _generate_inc_dir(self, compiler: 'Compiler', d: str, basedir: str, + is_system: bool, build_relative: bool, + source_relative: bool) -> \ + T.List['ImmutableListProtocol[str]']: # Avoid superfluous '/.' at the end of paths when d is '.' if d not in ('', '.'): expdir = os.path.normpath(os.path.join(basedir, d)) else: expdir = basedir - srctreedir = os.path.normpath(os.path.join(self.build_to_src, expdir)) - sargs = compiler.get_include_args(srctreedir, is_system) + + inc_dirs: T.List[T.List[str]] = [] + + is_builddir_suitable = build_relative and not os.path.isabs(d) + # There may be include dirs where a build directory has not been # created for some source dir. For example if someone does this: # # inc = include_directories('foo/bar/baz') # # But never subdir()s into the actual dir. - if os.path.isdir(os.path.join(self.environment.get_build_dir(), expdir)): - bargs = compiler.get_include_args(expdir, is_system) - else: - bargs = [] - return (sargs, bargs) + if is_builddir_suitable and os.path.isdir(os.path.join(self.environment.get_build_dir(), expdir)): + inc_dirs += compiler.get_include_args(expdir, is_system) + + if source_relative: + srctreedir = os.path.normpath(os.path.join(self.build_to_src, expdir)) + inc_dirs += compiler.get_include_args(srctreedir, is_system) + + return inc_dirs def _generate_single_compile(self, target: build.BuildTarget, compiler: 'Compiler', is_generated: bool = False) -> 'CompilerArgs': @@ -2909,10 +2941,8 @@ def _generate_single_compile_target_args(self, target: build.BuildTarget, compil # -Ipath will add to begin of array. And without reverse # flags will be added in reversed order. for d in reversed(i.get_incdirs()): - # Add source subdir first so that the build subdir overrides it - (compile_obj, includeargs) = self.generate_inc_dir(compiler, d, basedir, i.is_system) - commands += compile_obj - commands += includeargs + commands += self._generate_inc_dir(compiler, d, basedir, i.is_system, + i.build_relative, i.source_relative) for d in i.get_extra_build_dirs(): commands += compiler.get_include_args(d, i.is_system) # Add per-target compile args, f.ex, `c_args : ['-DFOO']`. We set these diff --git a/mesonbuild/backend/vs2010backend.py b/mesonbuild/backend/vs2010backend.py index bb72594c4d59..b0e273106047 100644 --- a/mesonbuild/backend/vs2010backend.py +++ b/mesonbuild/backend/vs2010backend.py @@ -999,7 +999,7 @@ def split_link_args(args): other.append(arg) return lpaths, libs, other - def _get_cl_compiler(self, target): + def _get_cl_compiler(self, target) -> compilers.Compiler: for lang, c in target.compilers.items(): if lang in {'c', 'cpp'}: return c @@ -1022,19 +1022,25 @@ def _prettyprint_vcxproj_xml(self, tree: ET.ElementTree, ofname: str) -> None: replace_if_different(ofname, ofname_tmp) # Returns: (target_args,file_args), (target_defines,file_defines), (target_inc_dirs,file_inc_dirs) - def get_args_defines_and_inc_dirs(self, target, compiler, generated_files_include_dirs, proj_to_src_root, proj_to_src_dir, build_args): + def get_args_defines_and_inc_dirs(self, target: build.BuildTarget, compiler: compilers.Compiler, + generated_files_include_dirs: T.List[str], proj_to_src_root: str, + proj_to_src_dir: str, build_args: T.List[str]) -> \ + T.Tuple[ + T.Tuple[T.List[str], T.Dict[str, CompilerArgs]], + T.Tuple[T.List[str], T.Dict[str, list]], + T.Tuple[T.List[str], T.Dict[str, list]]]: # Arguments, include dirs, defines for all files in the current target - target_args = [] - target_defines = [] - target_inc_dirs = [] + target_args: T.List[str] = [] + target_defines: T.List[str] = [] + target_inc_dirs: T.List[str] = [] # Arguments, include dirs, defines passed to individual files in # a target; perhaps because the args are language-specific # # file_args is also later split out into defines and include_dirs in # case someone passed those in there file_args: T.Dict[str, CompilerArgs] = {l: c.compiler_args() for l, c in target.compilers.items()} - file_defines = {l: [] for l in target.compilers} - file_inc_dirs = {l: [] for l in target.compilers} + file_defines: T.Dict[str, list] = {l: [] for l in target.compilers} + file_inc_dirs: T.Dict[str, list] = {l: [] for l in target.compilers} # The order in which these compile args are added must match # generate_single_compile() and generate_basic_compiler_args() for l, comp in target.compilers.items(): @@ -1081,13 +1087,21 @@ def get_args_defines_and_inc_dirs(self, target, compiler, generated_files_includ # reversed is used to keep order of includes for i in reversed(d.get_incdirs()): curdir = os.path.join(d.get_curdir(), i) - try: + if d.source_relative: # Add source subdir first so that the build subdir overrides it args.append('-I' + os.path.join(proj_to_src_root, curdir)) # src dir - args.append('-I' + self.relpath(curdir, target.subdir)) # build dir - except ValueError: - # Include is on different drive - args.append('-I' + os.path.normpath(curdir)) + + # We assert that we don't have any absolute paths with ONLY build-relative + # include dirs but since we allow abs paths in conjunction with source + # and/or build-relative include dirs, any abs paths will already be added + # via the 'source_relative' step above, so don't duplicate - + if d.build_relative and not os.path.isabs(i): + try: + args.append('-I' + self.relpath(curdir, target.subdir)) # build dir + except ValueError: # From 'relpath'... + # ... if curdir and target.subdir are different drives + args.append('-I' + os.path.normpath(curdir)) + for i in d.get_extra_build_dirs(): curdir = os.path.join(d.get_curdir(), i) args.append('-I' + self.relpath(curdir, target.subdir)) # build dir diff --git a/mesonbuild/build.py b/mesonbuild/build.py index c58447b55db8..e6e850aa2eb8 100644 --- a/mesonbuild/build.py +++ b/mesonbuild/build.py @@ -377,11 +377,24 @@ class IncludeDirs(HoldableObject): # Interpreter has validated that all given directories # actually exist. extra_build_dirs: T.List[str] = field(default_factory=list) + build_relative: bool = True + source_relative: bool = True def __repr__(self) -> str: r = '<{} {}/{}>' return r.format(self.__class__.__name__, self.curdir, self.incdirs) + def __post_init__(self): + assert self.build_relative or self.source_relative, \ + 'IncludeDirs must enable build_relative, or source_relative, or both' + + # It's meaningless to explicitly specify build-relative include dirs but + # with absolute paths. This is in addition to the same validation on + # user-supplied include dirs, since we also instantiate some internal + # IncludeDirs elsewhere, which we can prevent breaking this expectation. + assert self.source_relative or not any(os.path.isabs(d) for d in self.incdirs), \ + 'build-relative-only IncludeDirs with absolute paths is not expected.' + def get_curdir(self) -> str: return self.curdir @@ -399,10 +412,20 @@ def to_string_list(self, sourcedir: str, builddir: str) -> T.List[str]: be added if this is unset :returns: A list of strings (without compiler argument) """ + strlist: T.List[str] = [] - for idir in self.incdirs: - strlist.append(os.path.join(sourcedir, self.curdir, idir)) - strlist.append(os.path.join(builddir, self.curdir, idir)) + if self.source_relative: + strlist.extend(os.path.join(sourcedir, self.curdir, idir) for idir in self.incdirs) + + if self.build_relative: + if self.source_relative: + # Only append relative incdirs paths to the builddir, since any absolute paths + # will already have been appended above. + strlist.extend(os.path.join(builddir, self.curdir, idir) for idir in self.incdirs if not os.path.isabs(idir)) + else: + # We can assume they're relative paths that can all be appended + strlist.extend(os.path.join(builddir, self.curdir, idir) for idir in self.incdirs) + return strlist @dataclass(eq=False) diff --git a/mesonbuild/interpreter/interpreter.py b/mesonbuild/interpreter/interpreter.py index a5c8a5a2d4aa..b0401c3a432e 100644 --- a/mesonbuild/interpreter/interpreter.py +++ b/mesonbuild/interpreter/interpreter.py @@ -2781,12 +2781,18 @@ def extract_incdirs(self, kwargs, key: str = 'include_directories') -> T.List[bu return result @typed_pos_args('include_directories', varargs=str) - @typed_kwargs('include_directories', KwargInfo('is_system', bool, default=False)) + @typed_kwargs( + 'include_directories', + KwargInfo('is_system', bool, default=False), + KwargInfo('build', bool, default=True, since='1.3.0'), + KwargInfo('source', bool, default=True, since='1.3.0'), + ) def func_include_directories(self, node: mparser.BaseNode, args: T.Tuple[T.List[str]], kwargs: 'kwtypes.FuncIncludeDirectories') -> build.IncludeDirs: - return self.build_incdir_object(args[0], kwargs['is_system']) + return self.build_incdir_object(args[0], kwargs['is_system'], kwargs['build'], kwargs['source']) - def build_incdir_object(self, incdir_strings: T.List[str], is_system: bool = False) -> build.IncludeDirs: + def build_incdir_object(self, incdir_strings: T.List[str], is_system: bool = False, + build_relative: bool = True, source_relative: bool = True) -> build.IncludeDirs: if not isinstance(is_system, bool): raise InvalidArguments('Is_system must be boolean.') src_root = self.environment.get_source_dir() @@ -2850,7 +2856,20 @@ def build_incdir_object(self, incdir_strings: T.List[str], is_system: bool = Fal absdir_build = os.path.join(absbase_build, a) if not os.path.isdir(absdir_src) and not os.path.isdir(absdir_build): raise InvalidArguments(f'Include dir {a} does not exist.') - i = build.IncludeDirs(self.subdir, incdir_strings, is_system) + + if not (build_relative or source_relative): + raise InvalidArguments('include_directories must use \'build\'-relative, ' + '\'source\'-relative, or both.') + + if build_relative and not source_relative: + for d in incdir_strings: + if os.path.isabs(d): + raise InvalidArguments( + f'Absolute paths ({d}) in include_directories() with only ' + '\'build\'-relative use is not expected.') + + i = build.IncludeDirs(self.subdir, incdir_strings, is_system, + build_relative=build_relative, source_relative=source_relative) return i @typed_pos_args('add_test_setup', str) diff --git a/mesonbuild/interpreter/kwargs.py b/mesonbuild/interpreter/kwargs.py index 35b8fb65bcf7..d6e26a5994fe 100644 --- a/mesonbuild/interpreter/kwargs.py +++ b/mesonbuild/interpreter/kwargs.py @@ -159,6 +159,9 @@ class FuncImportModule(ExtractRequired): class FuncIncludeDirectories(TypedDict): is_system: bool + build: bool + source: bool + class FuncAddLanguages(ExtractRequired): diff --git a/test cases/common/267 include_directories relative/meson.build b/test cases/common/267 include_directories relative/meson.build new file mode 100644 index 000000000000..b34461e5faec --- /dev/null +++ b/test cases/common/267 include_directories relative/meson.build @@ -0,0 +1,49 @@ +project('include dirs type tests', 'cpp', meson_version: '>=1.3.0' ) + +fs = import('fs') + +# Copy src header, with a modification, to build dir so we can test finding it though +# 'src-relative', 'build-relative', and 'src-and-build-relative' use types. +configure_file( input: 'whereareyoufindingme.h', + output: 'whereareyoufindingme.h', + configuration: {'SRC_DIR': 'build dir'} + ) + +# Test that src-relative uses the unadulerated src header +exe_src = executable( + 'prog_src', + 'src/main.cpp', + implicit_include_directories: false, + include_directories: include_directories('.', build: false, source: true)) +test('src-relative', exe_src, args: ['Hello from @SRC_DIR@']) + +fs = import('fs') + +# Test that build-relative uses the tweaked header, copied to the build dir. +exe_build = executable( + 'prog_build', + 'src/main.cpp', + implicit_include_directories: false, + include_directories: include_directories('.', build: true, source: false)) +test('build-relative', exe_build, args: ['Hello from build dir']) + +# Check 'src-and-build-relative' by including one header that's only found under the src and one that only found under the build dir. +subdir('moreheaders') +exe_both = executable( + 'prog_both', + 'src/main2.cpp', + implicit_include_directories: false, + include_directories: include_directories('moreheaders', build: true, source: true), + dependencies: build_only_h_dep, + ) +test('both', exe_both, args: ['Src-only','Build-only']) + +# Finally check the same but through ensuring that unspecified 'build' and 'source' defaults to both true +exe_both_default = executable( + 'prog_both_default', + 'src/main2.cpp', + implicit_include_directories: false, + include_directories: include_directories('moreheaders'), + dependencies: build_only_h_dep, + ) +test('both default', exe_both_default, args: ['Src-only','Build-only']) diff --git a/test cases/common/267 include_directories relative/moreheaders/build_only._h b/test cases/common/267 include_directories relative/moreheaders/build_only._h new file mode 100644 index 000000000000..5bcadba41704 --- /dev/null +++ b/test cases/common/267 include_directories relative/moreheaders/build_only._h @@ -0,0 +1 @@ +#define BUILD_ONLY_MSG "Build-only" \ No newline at end of file diff --git a/test cases/common/267 include_directories relative/moreheaders/meson.build b/test cases/common/267 include_directories relative/moreheaders/meson.build new file mode 100644 index 000000000000..7207eaada4da --- /dev/null +++ b/test cases/common/267 include_directories relative/moreheaders/meson.build @@ -0,0 +1,7 @@ +# It's a shame configure_file(..., copy: true) has been deprecated. +# It was simpler for this use-case and would do the copying at setup/configure-time rather than at build-time, +# which better fits our usage. + +fs = import('fs') +build_only_h = fs.copyfile('build_only._h','build_only.h') +build_only_h_dep = declare_dependency(sources: build_only_h) \ No newline at end of file diff --git a/test cases/common/267 include_directories relative/moreheaders/src_only.h b/test cases/common/267 include_directories relative/moreheaders/src_only.h new file mode 100644 index 000000000000..0b77839f2b4b --- /dev/null +++ b/test cases/common/267 include_directories relative/moreheaders/src_only.h @@ -0,0 +1 @@ +#define SRC_ONLY_MSG "Src-only" \ No newline at end of file diff --git a/test cases/common/267 include_directories relative/src/main.cpp b/test cases/common/267 include_directories relative/src/main.cpp new file mode 100644 index 000000000000..cc753948aeab --- /dev/null +++ b/test cases/common/267 include_directories relative/src/main.cpp @@ -0,0 +1,15 @@ +#include +#include +#include "whereareyoufindingme.h" + +int main(int argc, char* argv[]) +{ + if (argc != 2) + { + std::puts("No input string to compare with: " MSG_FROM_HEADER); + return 1; + } + + std::puts(MSG_FROM_HEADER); + return std::strcmp(MSG_FROM_HEADER, argv[1]); +} diff --git a/test cases/common/267 include_directories relative/src/main2.cpp b/test cases/common/267 include_directories relative/src/main2.cpp new file mode 100644 index 000000000000..fc78a0f9b68c --- /dev/null +++ b/test cases/common/267 include_directories relative/src/main2.cpp @@ -0,0 +1,19 @@ +#include +#include +#include "src_only.h" +#include "build_only.h" + +int main(int argc, char* argv[]) +{ + if (argc != 3) + { + std::puts("Expect 2 args to compare with: " SRC_ONLY_MSG ", " BUILD_ONLY_MSG); + return 1; + } + + std::puts(SRC_ONLY_MSG ", " BUILD_ONLY_MSG); + if (std::strcmp(SRC_ONLY_MSG, argv[1])==0 && std::strcmp(BUILD_ONLY_MSG, argv[2])==0) + return 0; + + return 1; +} diff --git a/test cases/common/267 include_directories relative/whereareyoufindingme.h b/test cases/common/267 include_directories relative/whereareyoufindingme.h new file mode 100644 index 000000000000..516e02af2cce --- /dev/null +++ b/test cases/common/267 include_directories relative/whereareyoufindingme.h @@ -0,0 +1 @@ +#define MSG_FROM_HEADER "Hello from @SRC_DIR@" diff --git a/unittests/allplatformstests.py b/unittests/allplatformstests.py index acab026e954c..cc5a96a2a3df 100644 --- a/unittests/allplatformstests.py +++ b/unittests/allplatformstests.py @@ -4821,6 +4821,12 @@ def test_configure_same_noop(self): olddata = newdata oldmtime = newmtime + def test_include_dirs_relative(self): + testdir = os.path.join(self.common_test_dir, '267 include_directories relative') + self.init(testdir) + self.build() + self.run_tests() + def test_c_cpp_stds(self): testdir = os.path.join(self.unit_test_dir, '114 c cpp stds') self.init(testdir)