Fix output directories for generated source files.
NOTE: This is a re-land of [2] since another way to fix the issue
could not be found.
This CL fixes an annoying issue where the local build directory path
leaks into the generated Ninja files when {{source_out_dir}} or
even {{source_gen_dir}} are expanded.
In the case of a generated source file, its path will be source-absolute
but located in the build directory, for example:
//out/Debug/gen/header.h
//out/Debug/toolchain/gen/src/source.cc
Before this CL, the computed output directory would leak the 'out/Debug'
prefix, i.e. compute an output path, relative to `out/Debug` as:
obj/out/Debug/gen/
obj/out/Debug/toolchain/gen/src/
After this CL, the build directory prefix is replaced with the
"BUILD_DIR" string literal, and the result becomes:
obj/BUILD_DIR/gen/
obj/BUILD_DIR/toolchain/gen/src
This makes the commands more hermetic, especially with regards to remote
builds which implement content-based caching, using the command hash as
a key. It is also similar to what GN does for absolute paths, where it
uses the ABS_PATH literal to create target-specific directories for
sources that have an absolute path.
I tested that I could still build Fuchsia, Chromium and Pigweed with
this patch (as long as Pigwedd's build system is fixed with [1] to
support upstream GN).
Note that this CL is required to unblock the GN auto-roller for the
Fuchsia project: without it, the output directory leak checks will fail
the builds since the latest Rust-related changes adds a ton of
`-Lnative=<path-with-build-directory>` to a very large number of
Rust link commands.
[1] https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/90080
[2] https://gn-review.googlesource.com/c/gn/+/13240
Change-Id: I85b7798b25f282085864a6cb58a7eecb85648881
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/13580
Reviewed-by: Brett Wilson <brettw@chromium.org>
Commit-Queue: David Turner <digit@google.com>
diff --git a/src/gn/filesystem_utils.cc b/src/gn/filesystem_utils.cc
index 6de3fbd..7a7078e 100644
--- a/src/gn/filesystem_utils.cc
+++ b/src/gn/filesystem_utils.cc
@@ -1025,10 +1025,25 @@
OutputFile result = GetBuildDirAsOutputFile(context, type);
if (source_dir.is_source_absolute()) {
- // The source dir is source-absolute, so we trim off the two leading
- // slashes to append to the toolchain object directory.
- result.value().append(&source_dir.value()[2],
- source_dir.value().size() - 2);
+ std::string_view build_dir = context.build_settings->build_dir().value();
+ std::string_view source_dir_path = source_dir.value();
+ if (source_dir_path.substr(0, build_dir.size()) == build_dir) {
+ // The source dir is source-absolute, but in the build directory
+ // (e.g. `//out/Debug/gen/src/foo.cc` or
+ // `//out/Debug/toolchain1/gen/foo.cc`), which happens for generated
+ // sources. In this case, remove the build directory prefix, and replace
+ // it with `BUILD_DIR`. This will create results like `obj/BUILD_DIR/gen`
+ // or `toolchain2/obj/BUILD_DIR/toolchain1/gen` which look surprising,
+ // but guarantee unicity.
+ result.value().append("BUILD_DIR/");
+ result.value().append(&source_dir_path[build_dir.size()],
+ source_dir_path.size() - build_dir.size());
+ } else {
+ // The source dir is source-absolute, so we trim off the two leading
+ // slashes to append to the toolchain object directory.
+ result.value().append(&source_dir.value()[2],
+ source_dir.value().size() - 2);
+ }
} else {
// System-absolute.
AppendFixedAbsolutePathSuffix(context.build_settings, source_dir, &result);
diff --git a/src/gn/ninja_c_binary_target_writer_unittest.cc b/src/gn/ninja_c_binary_target_writer_unittest.cc
index 6056c36..9476193 100644
--- a/src/gn/ninja_c_binary_target_writer_unittest.cc
+++ b/src/gn/ninja_c_binary_target_writer_unittest.cc
@@ -405,18 +405,18 @@
"target_out_dir = obj/foo\n"
"target_output_name = gen_obj\n"
"\n"
- "build obj/out/Debug/gen_obj.generated.o: cxx generated.cc"
+ "build obj/BUILD_DIR/gen_obj.generated.o: cxx generated.cc"
" || obj/foo/generate.stamp\n"
" source_file_part = generated.cc\n"
" source_name_part = generated\n"
"\n"
- "build obj/foo/gen_obj.stamp: stamp obj/out/Debug/gen_obj.generated.o"
+ "build obj/foo/gen_obj.stamp: stamp obj/BUILD_DIR/gen_obj.generated.o"
// The order-only dependency here is strictly unnecessary since the
// sources list this as an order-only dep.
" || obj/foo/generate.stamp\n";
std::string obj_str = obj_out.str();
- EXPECT_EQ(obj_expected, obj_str);
+ EXPECT_EQ(std::string(obj_expected), obj_str);
// A shared library depends on gen_obj, having corresponding header for
// generated obj.
@@ -442,7 +442,7 @@
"target_output_name = libgen_lib\n"
"\n"
"\n"
- "build ./libgen_lib.so: solink obj/out/Debug/gen_obj.generated.o"
+ "build ./libgen_lib.so: solink obj/BUILD_DIR/gen_obj.generated.o"
// The order-only dependency here is strictly unnecessary since
// obj/out/Debug/gen_obj.generated.o has dependency to
// obj/foo/gen_obj.stamp
diff --git a/src/gn/substitution_writer_unittest.cc b/src/gn/substitution_writer_unittest.cc
index fc3c446..eaa521a 100644
--- a/src/gn/substitution_writer_unittest.cc
+++ b/src/gn/substitution_writer_unittest.cc
@@ -45,6 +45,12 @@
SourceFile result = SubstitutionWriter::ApplyPatternToSource(
nullptr, setup.settings(), pattern, SourceFile("//foo/bar/myfile.txt"));
ASSERT_EQ("//out/Debug/gen/foo/bar/myfile.tmp", result.value());
+
+ result = SubstitutionWriter::ApplyPatternToSource(
+ nullptr, setup.settings(), pattern,
+ SourceFile("//out/Debug/gen/generated_file.cc"));
+ ASSERT_EQ("//out/Debug/gen/BUILD_DIR/gen/generated_file.tmp", result.value())
+ << result.value();
}
TEST(SubstitutionWriter, ApplyPatternToSourceAsOutputFile) {
