Forbid Rust source_sets. These are not currently used by either Fuchsia or Chromium, as current best practice is to use groups instead. It's not clear what the purpose of a Rust source_set would be, since only the crate_root is fed to rustc anyway; any other sources just act as implicit dependencies. A subsequent commit will allow Rust targets to depend upon C++ source sets, and that change is much less invasive if Rust source sets are not possible. Change-Id: I28f5c931c793aa80fbf7116519cc22ae3631efc2 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/8240 Reviewed-by: Brett Wilson <brettw@chromium.org> Commit-Queue: Brett Wilson <brettw@chromium.org>
diff --git a/src/gn/functions_target.cc b/src/gn/functions_target.cc index 5ce0901..0864db5 100644 --- a/src/gn/functions_target.cc +++ b/src/gn/functions_target.cc
@@ -740,8 +740,7 @@ const char kSourceSet_Help[] = R"(source_set: Declare a source set target. - The language of a source_set target is determined by the extensions present - in its sources. + Only C-language source sets are supported at the moment. C-language source_sets @@ -766,13 +765,6 @@ not from the intermediate targets." There is no way to express this concept when linking multiple static libraries into a shared library. -Rust-language source_sets - - A Rust source set is a collection of sources that get passed along to the - final target that depends on it. No compilation is performed, and the source - files are simply added as dependencies on the eventual rustc invocation that - would produce a binary. - Variables )" CONFIG_VALUES_VARS_HELP DEPS_VARS DEPENDENT_CONFIG_VARS GENERAL_TARGET_VARS;
diff --git a/src/gn/ninja_binary_target_writer.cc b/src/gn/ninja_binary_target_writer.cc index 1a3e9b9..cde6be6 100644 --- a/src/gn/ninja_binary_target_writer.cc +++ b/src/gn/ninja_binary_target_writer.cc
@@ -196,9 +196,49 @@ void NinjaBinaryTargetWriter::AddSourceSetFiles( const Target* source_set, UniqueVector<OutputFile>* obj_files) const { - // Just add all sources to the list. + std::vector<OutputFile> tool_outputs; // Prevent allocation in loop. + + // Compute object files for all sources. Only link the first output from + // the tool if there are more than one. for (const auto& source : source_set->sources()) { - obj_files->push_back(OutputFile(settings_->build_settings(), source)); + const char* tool_name = Tool::kToolNone; + if (source_set->GetOutputFilesForSource(source, &tool_name, &tool_outputs)) + obj_files->push_back(tool_outputs[0]); + } + + // Add MSVC precompiled header object files. GCC .gch files are not object + // files so they are omitted. + if (source_set->config_values().has_precompiled_headers()) { + if (source_set->source_types_used().Get(SourceFile::SOURCE_C)) { + const CTool* tool = source_set->toolchain()->GetToolAsC(CTool::kCToolCc); + if (tool && tool->precompiled_header_type() == CTool::PCH_MSVC) { + GetPCHOutputFiles(source_set, CTool::kCToolCc, &tool_outputs); + obj_files->Append(tool_outputs.begin(), tool_outputs.end()); + } + } + if (source_set->source_types_used().Get(SourceFile::SOURCE_CPP)) { + const CTool* tool = source_set->toolchain()->GetToolAsC(CTool::kCToolCxx); + if (tool && tool->precompiled_header_type() == CTool::PCH_MSVC) { + GetPCHOutputFiles(source_set, CTool::kCToolCxx, &tool_outputs); + obj_files->Append(tool_outputs.begin(), tool_outputs.end()); + } + } + if (source_set->source_types_used().Get(SourceFile::SOURCE_M)) { + const CTool* tool = + source_set->toolchain()->GetToolAsC(CTool::kCToolObjC); + if (tool && tool->precompiled_header_type() == CTool::PCH_MSVC) { + GetPCHOutputFiles(source_set, CTool::kCToolObjC, &tool_outputs); + obj_files->Append(tool_outputs.begin(), tool_outputs.end()); + } + } + if (source_set->source_types_used().Get(SourceFile::SOURCE_MM)) { + const CTool* tool = + source_set->toolchain()->GetToolAsC(CTool::kCToolObjCxx); + if (tool && tool->precompiled_header_type() == CTool::PCH_MSVC) { + GetPCHOutputFiles(source_set, CTool::kCToolObjCxx, &tool_outputs); + obj_files->Append(tool_outputs.begin(), tool_outputs.end()); + } + } } }
diff --git a/src/gn/ninja_binary_target_writer.h b/src/gn/ninja_binary_target_writer.h index 7539ebb..69f5b50 100644 --- a/src/gn/ninja_binary_target_writer.h +++ b/src/gn/ninja_binary_target_writer.h
@@ -65,8 +65,8 @@ void WriteLibs(std::ostream& out, const Tool* tool); void WriteFrameworks(std::ostream& out, const Tool* tool); - virtual void AddSourceSetFiles(const Target* source_set, - UniqueVector<OutputFile>* obj_files) const; + void AddSourceSetFiles(const Target* source_set, + UniqueVector<OutputFile>* obj_files) const; // Cached version of the prefix used for rule types for this toolchain. std::string rule_prefix_;
diff --git a/src/gn/ninja_c_binary_target_writer.cc b/src/gn/ninja_c_binary_target_writer.cc index ebc2d17..5fa646f 100644 --- a/src/gn/ninja_c_binary_target_writer.cc +++ b/src/gn/ninja_c_binary_target_writer.cc
@@ -639,54 +639,3 @@ } return true; } - -// Appends the object files generated by the given source set to the given -// output vector. -void NinjaCBinaryTargetWriter::AddSourceSetFiles( - const Target* source_set, - UniqueVector<OutputFile>* obj_files) const { - std::vector<OutputFile> tool_outputs; // Prevent allocation in loop. - - // Compute object files for all sources. Only link the first output from - // the tool if there are more than one. - for (const auto& source : source_set->sources()) { - const char* tool_name = Tool::kToolNone; - if (source_set->GetOutputFilesForSource(source, &tool_name, &tool_outputs)) - obj_files->push_back(tool_outputs[0]); - } - - // Add MSVC precompiled header object files. GCC .gch files are not object - // files so they are omitted. - if (source_set->config_values().has_precompiled_headers()) { - if (source_set->source_types_used().Get(SourceFile::SOURCE_C)) { - const CTool* tool = source_set->toolchain()->GetToolAsC(CTool::kCToolCc); - if (tool && tool->precompiled_header_type() == CTool::PCH_MSVC) { - GetPCHOutputFiles(source_set, CTool::kCToolCc, &tool_outputs); - obj_files->Append(tool_outputs.begin(), tool_outputs.end()); - } - } - if (source_set->source_types_used().Get(SourceFile::SOURCE_CPP)) { - const CTool* tool = source_set->toolchain()->GetToolAsC(CTool::kCToolCxx); - if (tool && tool->precompiled_header_type() == CTool::PCH_MSVC) { - GetPCHOutputFiles(source_set, CTool::kCToolCxx, &tool_outputs); - obj_files->Append(tool_outputs.begin(), tool_outputs.end()); - } - } - if (source_set->source_types_used().Get(SourceFile::SOURCE_M)) { - const CTool* tool = - source_set->toolchain()->GetToolAsC(CTool::kCToolObjC); - if (tool && tool->precompiled_header_type() == CTool::PCH_MSVC) { - GetPCHOutputFiles(source_set, CTool::kCToolObjC, &tool_outputs); - obj_files->Append(tool_outputs.begin(), tool_outputs.end()); - } - } - if (source_set->source_types_used().Get(SourceFile::SOURCE_MM)) { - const CTool* tool = - source_set->toolchain()->GetToolAsC(CTool::kCToolObjCxx); - if (tool && tool->precompiled_header_type() == CTool::PCH_MSVC) { - GetPCHOutputFiles(source_set, CTool::kCToolObjCxx, &tool_outputs); - obj_files->Append(tool_outputs.begin(), tool_outputs.end()); - } - } - } -}
diff --git a/src/gn/ninja_c_binary_target_writer.h b/src/gn/ninja_c_binary_target_writer.h index 55cc0fc..ee6b6ac 100644 --- a/src/gn/ninja_c_binary_target_writer.h +++ b/src/gn/ninja_c_binary_target_writer.h
@@ -22,11 +22,6 @@ void Run() override; - protected: - // Adds source_set files to the list of object files. - void AddSourceSetFiles(const Target* source_set, - UniqueVector<OutputFile>* obj_files) const override; - private: using OutputFileSet = std::set<OutputFile>;
diff --git a/src/gn/ninja_rust_binary_target_writer.cc b/src/gn/ninja_rust_binary_target_writer.cc index 8a907bb..abe903f 100644 --- a/src/gn/ninja_rust_binary_target_writer.cc +++ b/src/gn/ninja_rust_binary_target_writer.cc
@@ -122,72 +122,68 @@ // -Ldependency rustdeps, and non-public source_sets get passed in as normal // source files UniqueVector<OutputFile> deps; - AddSourceSetFiles(target_, &deps); - if (target_->output_type() == Target::SOURCE_SET) { - WriteSharedVars(target_->toolchain()->substitution_bits()); - WriteSourceSetStamp(deps.vector()); - } else { - WriteCompilerVars(); - UniqueVector<const Target*> linkable_deps; - UniqueVector<const Target*> non_linkable_deps; - UniqueVector<const Target*> framework_deps; - GetDeps(&deps, &linkable_deps, &non_linkable_deps, &framework_deps); + DCHECK(target_->output_type() != Target::SOURCE_SET); + WriteCompilerVars(); + UniqueVector<const Target*> linkable_deps; + UniqueVector<const Target*> non_linkable_deps; + UniqueVector<const Target*> framework_deps; + GetDeps(&deps, &linkable_deps, &non_linkable_deps, &framework_deps); + AppendSourcesToImplicitDeps(&deps); - if (!input_dep.value().empty()) - order_only_deps.push_back(input_dep); + if (!input_dep.value().empty()) + order_only_deps.push_back(input_dep); - std::vector<OutputFile> rustdeps; - std::vector<OutputFile> nonrustdeps; - for (const auto* framework_dep : framework_deps) { - order_only_deps.push_back(framework_dep->dependency_output_file()); - } - for (const auto* non_linkable_dep : non_linkable_deps) { - if (non_linkable_dep->source_types_used().RustSourceUsed() && - non_linkable_dep->output_type() != Target::SOURCE_SET) { - rustdeps.push_back(non_linkable_dep->dependency_output_file()); - } - order_only_deps.push_back(non_linkable_dep->dependency_output_file()); - } - for (const auto* linkable_dep : linkable_deps) { - if (linkable_dep->source_types_used().RustSourceUsed()) { - rustdeps.push_back(linkable_dep->link_output_file()); - } else { - nonrustdeps.push_back(linkable_dep->link_output_file()); - } - deps.push_back(linkable_dep->dependency_output_file()); - } - - // Rust libraries specified by paths. - for (ConfigValuesIterator iter(target_); !iter.done(); iter.Next()) { - const ConfigValues& cur = iter.cur(); - for (const auto& e : cur.externs()) { - if (e.second.is_source_file()) { - deps.push_back( - OutputFile(settings_->build_settings(), e.second.source_file())); - } - } - } - - std::vector<OutputFile> transitive_rustlibs; - for (const auto* dep : - target_->rust_values().transitive_libs().GetOrdered()) { - if (dep->source_types_used().RustSourceUsed()) { - transitive_rustlibs.push_back(dep->dependency_output_file()); - } - } - - std::vector<OutputFile> tool_outputs; - SubstitutionWriter::ApplyListToLinkerAsOutputFile( - target_, tool_, tool_->outputs(), &tool_outputs); - WriteCompilerBuildLine(target_->rust_values().crate_root(), deps.vector(), - order_only_deps, tool_->name(), tool_outputs); - - std::vector<const Target*> extern_deps(linkable_deps.vector()); - std::copy(non_linkable_deps.begin(), non_linkable_deps.end(), - std::back_inserter(extern_deps)); - WriteExterns(extern_deps); - WriteRustdeps(transitive_rustlibs, rustdeps, nonrustdeps); + std::vector<OutputFile> rustdeps; + std::vector<OutputFile> nonrustdeps; + for (const auto* framework_dep : framework_deps) { + order_only_deps.push_back(framework_dep->dependency_output_file()); } + for (const auto* non_linkable_dep : non_linkable_deps) { + if (non_linkable_dep->source_types_used().RustSourceUsed() && + non_linkable_dep->output_type() != Target::SOURCE_SET) { + rustdeps.push_back(non_linkable_dep->dependency_output_file()); + } + order_only_deps.push_back(non_linkable_dep->dependency_output_file()); + } + for (const auto* linkable_dep : linkable_deps) { + if (linkable_dep->source_types_used().RustSourceUsed()) { + rustdeps.push_back(linkable_dep->link_output_file()); + } else { + nonrustdeps.push_back(linkable_dep->link_output_file()); + } + deps.push_back(linkable_dep->dependency_output_file()); + } + + // Rust libraries specified by paths. + for (ConfigValuesIterator iter(target_); !iter.done(); iter.Next()) { + const ConfigValues& cur = iter.cur(); + for (const auto& e : cur.externs()) { + if (e.second.is_source_file()) { + deps.push_back( + OutputFile(settings_->build_settings(), e.second.source_file())); + } + } + } + + std::vector<OutputFile> transitive_rustlibs; + for (const auto* dep : + target_->rust_values().transitive_libs().GetOrdered()) { + if (dep->source_types_used().RustSourceUsed()) { + transitive_rustlibs.push_back(dep->dependency_output_file()); + } + } + + std::vector<OutputFile> tool_outputs; + SubstitutionWriter::ApplyListToLinkerAsOutputFile( + target_, tool_, tool_->outputs(), &tool_outputs); + WriteCompilerBuildLine(target_->rust_values().crate_root(), deps.vector(), + order_only_deps, tool_->name(), tool_outputs); + + std::vector<const Target*> extern_deps(linkable_deps.vector()); + std::copy(non_linkable_deps.begin(), non_linkable_deps.end(), + std::back_inserter(extern_deps)); + WriteExterns(extern_deps); + WriteRustdeps(transitive_rustlibs, rustdeps, nonrustdeps); } void NinjaRustBinaryTargetWriter::WriteCompilerVars() { @@ -205,6 +201,18 @@ WriteSharedVars(subst); } +void NinjaRustBinaryTargetWriter::AppendSourcesToImplicitDeps( + UniqueVector<OutputFile>* deps) const { + // Only the crate_root file needs to be given to rustc as input. + // Any other 'sources' are just implicit deps. + // Most Rust targets won't bother specifying the "sources =" line + // because it is handled sufficiently by crate_root and the generation + // of depfiles by rustc. But for those which do... + for (const auto& source : target_->sources()) { + deps->push_back(OutputFile(settings_->build_settings(), source)); + } +} + void NinjaRustBinaryTargetWriter::WriteExterns( const std::vector<const Target*>& deps) { out_ << " externs =";
diff --git a/src/gn/ninja_rust_binary_target_writer.h b/src/gn/ninja_rust_binary_target_writer.h index cd845d5..4d6e146 100644 --- a/src/gn/ninja_rust_binary_target_writer.h +++ b/src/gn/ninja_rust_binary_target_writer.h
@@ -26,9 +26,10 @@ const std::vector<OutputFile>& order_only_deps); void WriteExterns(const std::vector<const Target*>& deps); void WriteRustdeps(const std::vector<OutputFile>& transitive_rustdeps, - const std::vector<OutputFile>& rustdeps, - const std::vector<OutputFile>& nonrustdeps); + const std::vector<OutputFile>& rustdeps, + const std::vector<OutputFile>& nonrustdeps); void WriteEdition(); + void AppendSourcesToImplicitDeps(UniqueVector<OutputFile>* deps) const; const RustTool* tool_;
diff --git a/src/gn/ninja_rust_binary_target_writer_unittest.cc b/src/gn/ninja_rust_binary_target_writer_unittest.cc index ded290a..70a7d00 100644 --- a/src/gn/ninja_rust_binary_target_writer_unittest.cc +++ b/src/gn/ninja_rust_binary_target_writer_unittest.cc
@@ -26,39 +26,13 @@ target.sources().push_back(SourceFile("//foo/main.rs")); target.source_types_used().Set(SourceFile::SOURCE_RS); target.SetToolchain(setup.toolchain()); - ASSERT_TRUE(target.OnResolved(&err)); - - // Source set itself. - { - std::ostringstream out; - NinjaRustBinaryTargetWriter writer(&target, out); - writer.Run(); - - const char expected[] = - "root_out_dir = .\n" - "target_out_dir = obj/foo\n" - "target_output_name = bar\n" - "\n" - "build obj/foo/bar.stamp: stamp ../../foo/input1.rs " - "../../foo/main.rs\n"; - std::string out_str = out.str(); - EXPECT_EQ(expected, out_str) << out_str; - } + ASSERT_FALSE(target.OnResolved(&err)); } TEST_F(NinjaRustBinaryTargetWriterTest, RustExecutable) { Err err; TestWithScope setup; - Target source_set(setup.settings(), Label(SourceDir("//foo/"), "sources")); - source_set.set_output_type(Target::SOURCE_SET); - source_set.visibility().SetPublic(); - source_set.sources().push_back(SourceFile("//foo/input1.rs")); - source_set.sources().push_back(SourceFile("//foo/input2.rs")); - source_set.source_types_used().Set(SourceFile::SOURCE_RS); - source_set.SetToolchain(setup.toolchain()); - ASSERT_TRUE(source_set.OnResolved(&err)); - Target target(setup.settings(), Label(SourceDir("//foo/"), "bar")); target.set_output_type(Target::EXECUTABLE); target.visibility().SetPublic(); @@ -68,7 +42,6 @@ target.source_types_used().Set(SourceFile::SOURCE_RS); target.rust_values().set_crate_root(main); target.rust_values().crate_name() = "foo_bar"; - target.private_deps().push_back(LabelTargetPair(&source_set)); target.SetToolchain(setup.toolchain()); ASSERT_TRUE(target.OnResolved(&err)); @@ -89,8 +62,7 @@ "target_output_name = bar\n" "\n" "build ./foo_bar: rust_bin ../../foo/main.rs | ../../foo/input3.rs " - "../../foo/main.rs ../../foo/input1.rs ../../foo/input2.rs || " - "obj/foo/sources.stamp\n" + "../../foo/main.rs\n" " externs =\n" " rustdeps =\n"; std::string out_str = out.str(); @@ -495,15 +467,6 @@ Err err; TestWithScope setup; - Target source_set(setup.settings(), Label(SourceDir("//foo/"), "sources")); - source_set.set_output_type(Target::SOURCE_SET); - source_set.visibility().SetPublic(); - source_set.sources().push_back(SourceFile("//foo/input1.rs")); - source_set.sources().push_back(SourceFile("//foo/input2.rs")); - source_set.source_types_used().Set(SourceFile::SOURCE_RS); - source_set.SetToolchain(setup.toolchain()); - ASSERT_TRUE(source_set.OnResolved(&err)); - Target target(setup.settings(), Label(SourceDir("//foo/"), "bar")); target.set_output_type(Target::EXECUTABLE); target.visibility().SetPublic(); @@ -515,7 +478,6 @@ target.set_output_dir(SourceDir("//out/Debug/foo/")); target.rust_values().set_crate_root(main); target.rust_values().crate_name() = "foo_bar"; - target.private_deps().push_back(LabelTargetPair(&source_set)); target.SetToolchain(setup.toolchain()); ASSERT_TRUE(target.OnResolved(&err)); @@ -536,8 +498,7 @@ "target_output_name = bar\n" "\n" "build ./foo_bar.exe: rust_bin ../../foo/main.rs | ../../foo/input3.rs " - "../../foo/main.rs ../../foo/input1.rs ../../foo/input2.rs || " - "obj/foo/sources.stamp\n" + "../../foo/main.rs\n" " externs =\n" " rustdeps =\n"; std::string out_str = out.str();
diff --git a/src/gn/target.cc b/src/gn/target.cc index 93ad7c0..ef91167 100644 --- a/src/gn/target.cc +++ b/src/gn/target.cc
@@ -383,6 +383,8 @@ if (!FillOutputFiles(err)) return false; + if (!CheckSourceSetLanguages(err)) + return false; if (!CheckVisibility(err)) return false; if (!CheckTestonly(err)) @@ -934,6 +936,17 @@ return true; } +bool Target::CheckSourceSetLanguages(Err* err) const { + if (output_type() == Target::SOURCE_SET && + source_types_used().RustSourceUsed()) { + *err = Err(defined_from(), "source_set contained Rust code.", + label().GetUserVisibleName(false) + + " has Rust code. Only C/C++ source_sets are supported."); + return false; + } + return true; +} + bool Target::CheckTestonly(Err* err) const { // If the current target is marked testonly, it can include both testonly // and non-testonly targets, so there's nothing to check.
diff --git a/src/gn/target.h b/src/gn/target.h index 700859c..44f7e17 100644 --- a/src/gn/target.h +++ b/src/gn/target.h
@@ -407,6 +407,7 @@ bool CheckAssertNoDeps(Err* err) const; void CheckSourcesGenerated() const; void CheckSourceGenerated(const SourceFile& source) const; + bool CheckSourceSetLanguages(Err* err) const; OutputType output_type_ = UNKNOWN; std::string output_name_;