Revert "Rust: link_output, depend_output and runtime_outputs for dylibs" This reverts commit 0ee833e823f2e11be136728169906d0710bee910. Reason for revert: breaks Chromium build: https://crrev.com/c/5762050 Original change's description: > Rust: link_output, depend_output and runtime_outputs for dylibs > > Ensure that the rust_dylib and rust_cdylib tool() definition > support the `link_output`, `depend_output` and `runtime_outputs` > argument, when generating shared libraries from Rust sources, > just like the `solink` tool used for C++ sources. > > Bug: 377 > Change-Id: I2286f42661b9ebbff5d5b8455c2be49aaf45afa9 > Reviewed-on: https://gn-review.googlesource.com/c/gn/+/17401 > Reviewed-by: Takuto Ikuta <tikuta@google.com> > Commit-Queue: David Turner <digit@google.com> # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 377 Change-Id: I3926c0d744da32fd14397148caf042793755ad25 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/17480 Reviewed-by: Dirk Pranke <dpranke@google.com> Commit-Queue: Dirk Pranke <dpranke@google.com> Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
diff --git a/docs/reference.md b/docs/reference.md index b99c339..a9fad5b 100644 --- a/docs/reference.md +++ b/docs/reference.md
@@ -3890,13 +3890,13 @@ link_output [string with substitutions] depend_output [string with substitutions] - Valid for: "solink", "rust_dylib" or "rust_cdylib" only (optional) + Valid for: "solink" only (optional) - These two files specify which of the outputs from the tool should - be used for linking and dependency tracking. These should match entries - in the "outputs". If unspecified, the first item in the "outputs" array - will be used for all. See "Separate linking and dependencies for shared - libraries" below for more. + These two files specify which of the outputs from the solink tool + should be used for linking and dependency tracking. These should match + entries in the "outputs". If unspecified, the first item in the + "outputs" array will be used for all. See "Separate linking and + dependencies for shared libraries" below for more. On Windows, where the tools produce a .dll shared library and a .lib import library, you will want the first two to be the import library @@ -4142,8 +4142,8 @@ {{solibs}} Extra libraries from shared library dependencies not specified in the {{inputs}}. This is the list of link_output files from shared libraries - (if the solink, rust_dylib and rust_cdylib tools specify a "link_output" - variable separate from the "depend_output"). + (if the solink tool specifies a "link_output" variable separate from + the "depend_output"). These should generally be treated the same as libs by your tool.
diff --git a/src/gn/c_tool.cc b/src/gn/c_tool.cc index e0bea41..767b324 100644 --- a/src/gn/c_tool.cc +++ b/src/gn/c_tool.cc
@@ -220,7 +220,8 @@ if (!ValidateLinkAndDependOutput(depend_output(), "depend_output", err)) { return false; } - if (link_output().empty() != depend_output().empty()) { + if ((!link_output().empty() && depend_output().empty()) || + (link_output().empty() && !depend_output().empty())) { *err = Err(defined_from(), "Both link_output and depend_output should either " "be specified or they should both be empty.");
diff --git a/src/gn/c_tool.h b/src/gn/c_tool.h index 86a60e8..c1b5883 100644 --- a/src/gn/c_tool.h +++ b/src/gn/c_tool.h
@@ -83,6 +83,14 @@ depend_output_ = std::move(dep_out); } + // Other functions ---------------------------------------------------------- + + // Returns true if this tool has separate outputs for dependency tracking + // and linking. + bool has_separate_solink_files() const { + return !link_output_.empty() || !depend_output_.empty(); + } + private: // Initialization functions ------------------------------------------------- // @@ -92,8 +100,9 @@ bool ValidateOutputSubstitution(const Substitution* sub_type) const; bool ValidateRuntimeOutputs(Err* err); // Validates either link_output or depend_output. To generalize to either, - // pass the associated pattern, and the variable name that should appear in - // error messages. + // pass + // the associated pattern, and the variable name that should appear in error + // messages. bool ValidateLinkAndDependOutput(const SubstitutionPattern& pattern, const char* variable_name, Err* err);
diff --git a/src/gn/function_toolchain.cc b/src/gn/function_toolchain.cc index a97a19c..1df3845 100644 --- a/src/gn/function_toolchain.cc +++ b/src/gn/function_toolchain.cc
@@ -509,13 +509,13 @@ link_output [string with substitutions] depend_output [string with substitutions] - Valid for: "solink", "rust_dylib" or "rust_cdylib" only (optional) + Valid for: "solink" only (optional) - These two files specify which of the outputs from the tool should - be used for linking and dependency tracking. These should match entries - in the "outputs". If unspecified, the first item in the "outputs" array - will be used for all. See "Separate linking and dependencies for shared - libraries" below for more. + These two files specify which of the outputs from the solink tool + should be used for linking and dependency tracking. These should match + entries in the "outputs". If unspecified, the first item in the + "outputs" array will be used for all. See "Separate linking and + dependencies for shared libraries" below for more. On Windows, where the tools produce a .dll shared library and a .lib import library, you will want the first two to be the import library @@ -760,8 +760,8 @@ {{solibs}} Extra libraries from shared library dependencies not specified in the {{inputs}}. This is the list of link_output files from shared libraries - (if the solink, rust_dylib and rust_cdylib tools specify a "link_output" - variable separate from the "depend_output"). + (if the solink tool specifies a "link_output" variable separate from + the "depend_output"). These should generally be treated the same as libs by your tool.
diff --git a/src/gn/function_toolchain_unittest.cc b/src/gn/function_toolchain_unittest.cc index 91d2235..74cbef9 100644 --- a/src/gn/function_toolchain_unittest.cc +++ b/src/gn/function_toolchain_unittest.cc
@@ -117,100 +117,6 @@ } } -TEST_F(FunctionToolchain, RustRuntimeOutputs) { - TestWithScope setup; - - // These runtime outputs are a subset of the outputs so are OK. - { - TestParseInput input( - R"(toolchain("good") { - tool("rust_dylib") { - command = "rust_dylib" - outputs = [ "foo" ] - runtime_outputs = [ "foo" ] - } - })"); - ASSERT_FALSE(input.has_error()); - - Err err; - input.parsed()->Execute(setup.scope(), &err); - ASSERT_FALSE(err.has_error()) << err.message(); - - // It should have generated a toolchain. - ASSERT_EQ(1u, setup.items().size()); - const Toolchain* toolchain = setup.items()[0]->AsToolchain(); - ASSERT_TRUE(toolchain); - - // The toolchain should have a link tool with the two outputs. - const Tool* link = toolchain->GetTool(RustTool::kRsToolDylib); - ASSERT_TRUE(link); - ASSERT_EQ(1u, link->outputs().list().size()); - EXPECT_EQ("foo", link->outputs().list()[0].AsString()); - ASSERT_EQ(1u, link->runtime_outputs().list().size()); - EXPECT_EQ("foo", link->runtime_outputs().list()[0].AsString()); - } - - // This one is not a subset so should throw an error. - { - TestParseInput input( - R"(toolchain("bad") { - tool("rust_dylib") { - outputs = [ "foo" ] - runtime_outputs = [ "bar" ] - } - })"); - ASSERT_FALSE(input.has_error()); - - Err err; - input.parsed()->Execute(setup.scope(), &err); - ASSERT_TRUE(err.has_error()) << err.message(); - } -} - -TEST_F(FunctionToolchain, RustLinkDependAndRuntimeOutputs) { - TestWithScope setup; - - // These runtime outputs are a subset of the outputs so are OK. - { - TestParseInput input( - R"(toolchain("good") { - tool("rust_dylib") { - command = "rust_dylib" - outputs = [ "interface", "lib", "unstripped", "stripped" ] - depend_output = "interface" - link_output = "lib" - runtime_outputs = [ "stripped" ] - } - })"); - ASSERT_FALSE(input.has_error()); - - Err err; - input.parsed()->Execute(setup.scope(), &err); - ASSERT_FALSE(err.has_error()) << err.message(); - - // It should have generated a toolchain. - ASSERT_EQ(1u, setup.items().size()); - const Toolchain* toolchain = setup.items()[0]->AsToolchain(); - ASSERT_TRUE(toolchain); - - // The toolchain should have a link tool with the two outputs. - const Tool* link = toolchain->GetTool(RustTool::kRsToolDylib); - ASSERT_TRUE(link); - ASSERT_EQ(4u, link->outputs().list().size()); - EXPECT_EQ("interface", link->outputs().list()[0].AsString()); - EXPECT_EQ("lib", link->outputs().list()[1].AsString()); - EXPECT_EQ("unstripped", link->outputs().list()[2].AsString()); - EXPECT_EQ("stripped", link->outputs().list()[3].AsString()); - ASSERT_EQ(1u, link->runtime_outputs().list().size()); - EXPECT_EQ("stripped", link->runtime_outputs().list()[0].AsString()); - - const RustTool* rust_tool = link->AsRust(); - ASSERT_TRUE(rust_tool); - EXPECT_EQ("interface", rust_tool->depend_output().AsString()); - EXPECT_EQ("lib", rust_tool->link_output().AsString()); - } -} - TEST_F(FunctionToolchain, Command) { TestWithScope setup;
diff --git a/src/gn/rust_tool.cc b/src/gn/rust_tool.cc index 1b0898c..f799d73 100644 --- a/src/gn/rust_tool.cc +++ b/src/gn/rust_tool.cc
@@ -54,56 +54,6 @@ void RustTool::SetComplete() { SetToolComplete(); - link_output_.FillRequiredTypes(&substitution_bits_); - depend_output_.FillRequiredTypes(&substitution_bits_); -} - -bool RustTool::ValidateRuntimeOutputs(Err* err) { - if (runtime_outputs().list().empty()) - return true; // Empty is always OK. - - if (name_ != kRsToolDylib && name_ != kRsToolCDylib) { - *err = Err( - defined_from(), "This tool specifies runtime_outputs.", - "This is only valid for linker tools (rust_staticlib doesn't count)."); - return false; - } - - for (const SubstitutionPattern& pattern : runtime_outputs().list()) { - if (!IsPatternInOutputList(outputs(), pattern)) { - *err = Err(defined_from(), "This tool's runtime_outputs is bad.", - "It must be a subset of the outputs. The bad one is:\n " + - pattern.AsString()); - return false; - } - } - return true; -} - -// Validates either link_output or depend_output. To generalize to either, pass -// the associated pattern, and the variable name that should appear in error -// messages. -bool RustTool::ValidateLinkAndDependOutput(const SubstitutionPattern& pattern, - const char* variable_name, - Err* err) { - if (pattern.empty()) - return true; // Empty is always OK. - - // It should only be specified for certain tool types. - if (name_ != kRsToolDylib && name_ != kRsToolCDylib) { - *err = Err(defined_from(), - "This tool specifies a " + std::string(variable_name) + ".", - "This is only valid for solink and solink_module tools."); - return false; - } - - if (!IsPatternInOutputList(outputs(), pattern)) { - *err = Err(defined_from(), "This tool's link_output is bad.", - "It must match one of the outputs."); - return false; - } - - return true; } std::string_view RustTool::GetSysroot() const { @@ -181,31 +131,6 @@ !ReadString(scope, "dynamic_link_switch", &dynamic_link_switch_, err)) { return false; } - - if (name_ == kRsToolDylib || name_ == kRsToolCDylib) { - if (!ReadPattern(scope, "link_output", &link_output_, err) || - !ReadPattern(scope, "depend_output", &depend_output_, err)) { - return false; - } - - // Validate link_output and depend_output. - if (!ValidateLinkAndDependOutput(link_output(), "link_output", err)) { - return false; - } - if (!ValidateLinkAndDependOutput(depend_output(), "depend_output", err)) { - return false; - } - if (link_output().empty() != depend_output().empty()) { - *err = Err(defined_from(), - "Both link_output and depend_output should either " - "be specified or they should both be empty."); - return false; - } - } - - if (!ValidateRuntimeOutputs(err)) { - return false; - } } return true;
diff --git a/src/gn/rust_tool.h b/src/gn/rust_tool.h index 76e8bf1..857e205 100644 --- a/src/gn/rust_tool.h +++ b/src/gn/rust_tool.h
@@ -52,19 +52,6 @@ dynamic_link_switch_ = std::move(s); } - // Should match files in the outputs() if nonempty. - const SubstitutionPattern& link_output() const { return link_output_; } - void set_link_output(SubstitutionPattern link_out) { - DCHECK(!complete_); - link_output_ = std::move(link_out); - } - - const SubstitutionPattern& depend_output() const { return depend_output_; } - void set_depend_output(SubstitutionPattern dep_out) { - DCHECK(!complete_); - depend_output_ = std::move(dep_out); - } - private: std::string rust_sysroot_; std::string dynamic_link_switch_; @@ -75,22 +62,6 @@ SubstitutionList* field, Err* err); - // Initialization functions ------------------------------------------------- - // - // Initialization methods used by InitTool(). If successful, will set the - // field and return true, otherwise will return false. Must be called before - // SetComplete(). - bool ValidateRuntimeOutputs(Err* err); - // Validates either link_output or depend_output. To generalize to either, - // pass the associated pattern, and the variable name that should appear in - // error messages. - bool ValidateLinkAndDependOutput(const SubstitutionPattern& pattern, - const char* variable_name, - Err* err); - - SubstitutionPattern link_output_; - SubstitutionPattern depend_output_; - RustTool(const RustTool&) = delete; RustTool& operator=(const RustTool&) = delete; };
diff --git a/src/gn/target.cc b/src/gn/target.cc index fb8dbef..b6c6ca0 100644 --- a/src/gn/target.cc +++ b/src/gn/target.cc
@@ -14,7 +14,6 @@ #include "gn/deps_iterator.h" #include "gn/filesystem_utils.h" #include "gn/functions.h" -#include "gn/rust_tool.h" #include "gn/scheduler.h" #include "gn/substitution_writer.h" #include "gn/tool.h" @@ -826,56 +825,42 @@ this, tool, tool->outputs().list()[0]); break; case RUST_PROC_MACRO: - case SHARED_LIBRARY: { + case SHARED_LIBRARY: CHECK(tool->outputs().list().size() >= 1); check_tool_outputs = true; - - const SubstitutionPattern* link_output_ptr = nullptr; - const SubstitutionPattern* depend_output_ptr = nullptr; - const SubstitutionList* runtime_outputs_ptr = nullptr; - if (const CTool* ctool = tool->AsC()) { - link_output_ptr = - ctool->link_output().empty() ? nullptr : &ctool->link_output(); - depend_output_ptr = - ctool->depend_output().empty() ? nullptr : &ctool->depend_output(); - runtime_outputs_ptr = &ctool->runtime_outputs(); - } else if (const RustTool* rust_tool = tool->AsRust()) { - link_output_ptr = rust_tool->link_output().empty() - ? nullptr - : &rust_tool->link_output(); - depend_output_ptr = rust_tool->depend_output().empty() - ? nullptr - : &rust_tool->depend_output(); - runtime_outputs_ptr = &rust_tool->runtime_outputs(); - } - - if (!link_output_ptr && !depend_output_ptr) { + if (ctool->link_output().empty() && ctool->depend_output().empty()) { + // Default behavior, use the first output file for both. + link_output_file_ = dependency_output_file_ = + SubstitutionWriter::ApplyPatternToLinkerAsOutputFile( + this, tool, tool->outputs().list()[0]); + } else { + // Use the tool-specified ones. + if (!ctool->link_output().empty()) { + link_output_file_ = + SubstitutionWriter::ApplyPatternToLinkerAsOutputFile( + this, tool, ctool->link_output()); + } + if (!ctool->depend_output().empty()) { + dependency_output_file_ = + SubstitutionWriter::ApplyPatternToLinkerAsOutputFile( + this, tool, ctool->depend_output()); + } + } + if (tool->runtime_outputs().list().empty()) { + // Default to the link output for the runtime output. + runtime_outputs_.push_back(link_output_file_); + } else { + SubstitutionWriter::ApplyListToLinkerAsOutputFile( + this, tool, tool->runtime_outputs(), &runtime_outputs_); + } + } else if (tool->AsRust()) { // Default behavior, use the first output file for both. link_output_file_ = dependency_output_file_ = SubstitutionWriter::ApplyPatternToLinkerAsOutputFile( this, tool, tool->outputs().list()[0]); - } else { - if (link_output_ptr) { - link_output_file_ = - SubstitutionWriter::ApplyPatternToLinkerAsOutputFile( - this, tool, *link_output_ptr); - } - if (depend_output_ptr) { - dependency_output_file_ = - SubstitutionWriter::ApplyPatternToLinkerAsOutputFile( - this, tool, *depend_output_ptr); - } - } - if (!runtime_outputs_ptr || runtime_outputs_ptr->list().empty()) { - // Default to the link output for the runtime output. - runtime_outputs_.push_back(link_output_file_); - } else { - SubstitutionWriter::ApplyListToLinkerAsOutputFile( - this, tool, *runtime_outputs_ptr, &runtime_outputs_); } break; - } case UNKNOWN: default: NOTREACHED();
diff --git a/src/gn/target_unittest.cc b/src/gn/target_unittest.cc index 5cc2182..8af8f6f 100644 --- a/src/gn/target_unittest.cc +++ b/src/gn/target_unittest.cc
@@ -553,48 +553,6 @@ EXPECT_EQ("./liba.so", target.runtime_outputs()[0].value()); } -TEST_F(TargetTest, RustLinkAndDepOutputs) { - TestWithScope setup; - Err err; - - Toolchain toolchain(setup.settings(), Label(SourceDir("//tc/"), "tc")); - - std::unique_ptr<Tool> tool = Tool::CreateTool(RustTool::kRsToolDylib); - RustTool* rust_tool = tool->AsRust(); - rust_tool->set_output_prefix("lib"); - rust_tool->set_default_output_extension(".so"); - - const char kLinkPattern[] = - "{{root_out_dir}}/{{target_output_name}}{{output_extension}}"; - SubstitutionPattern link_output = - SubstitutionPattern::MakeForTest(kLinkPattern); - rust_tool->set_link_output(link_output); - - const char kDependPattern[] = - "{{root_out_dir}}/{{target_output_name}}{{output_extension}}.TOC"; - SubstitutionPattern depend_output = - SubstitutionPattern::MakeForTest(kDependPattern); - rust_tool->set_depend_output(depend_output); - - rust_tool->set_outputs( - SubstitutionList::MakeForTest(kLinkPattern, kDependPattern)); - - toolchain.SetTool(std::move(tool)); - - Target target(setup.settings(), Label(SourceDir("//a/"), "a")); - target.source_types_used().Set(SourceFile::SOURCE_RS); - target.rust_values().set_crate_type(RustValues::CRATE_DYLIB); - target.set_output_type(Target::SHARED_LIBRARY); - target.SetToolchain(&toolchain); - ASSERT_TRUE(target.OnResolved(&err)); - - EXPECT_EQ("./liba.so", target.link_output_file().value()); - EXPECT_EQ("./liba.so.TOC", target.dependency_output_file().value()); - - ASSERT_EQ(1u, target.runtime_outputs().size()); - EXPECT_EQ("./liba.so", target.runtime_outputs()[0].value()); -} - // Tests that runtime_outputs works without an explicit link_output for // solink tools. // @@ -651,59 +609,6 @@ EXPECT_EQ("//out/Debug/a.pdb", computed_outputs[2].value()); } -TEST_F(TargetTest, RustRuntimeOuputs) { - TestWithScope setup; - Err err; - - Toolchain toolchain(setup.settings(), Label(SourceDir("//tc/"), "tc")); - - std::unique_ptr<Tool> tool = Tool::CreateTool(RustTool::kRsToolCDylib); - RustTool* rust_tool = tool->AsRust(); - rust_tool->set_output_prefix(""); - rust_tool->set_default_output_extension(".dll"); - - // Say the linker makes a DLL< an import library, and a symbol file we want - // to treat as a runtime output. - const char kLibPattern[] = - "{{root_out_dir}}/{{target_output_name}}{{output_extension}}.lib"; - const char kDllPattern[] = - "{{root_out_dir}}/{{target_output_name}}{{output_extension}}"; - const char kPdbPattern[] = "{{root_out_dir}}/{{target_output_name}}.pdb"; - SubstitutionPattern pdb_pattern = - SubstitutionPattern::MakeForTest(kPdbPattern); - - rust_tool->set_outputs( - SubstitutionList::MakeForTest(kLibPattern, kDllPattern, kPdbPattern)); - - // Say we only want the DLL and symbol file treaded as runtime outputs. - rust_tool->set_runtime_outputs( - SubstitutionList::MakeForTest(kDllPattern, kPdbPattern)); - - toolchain.SetTool(std::move(tool)); - - Target target(setup.settings(), Label(SourceDir("//a/"), "a")); - target.source_types_used().Set(SourceFile::SOURCE_RS); - target.rust_values().set_crate_type(RustValues::CRATE_CDYLIB); - target.set_output_type(Target::SHARED_LIBRARY); - target.SetToolchain(&toolchain); - ASSERT_TRUE(target.OnResolved(&err)); - - EXPECT_EQ("./a.dll.lib", target.link_output_file().value()); - EXPECT_EQ("./a.dll.lib", target.dependency_output_file().value()); - - ASSERT_EQ(2u, target.runtime_outputs().size()); - EXPECT_EQ("./a.dll", target.runtime_outputs()[0].value()); - EXPECT_EQ("./a.pdb", target.runtime_outputs()[1].value()); - - // Test GetOutputsAsSourceFiles(). - std::vector<SourceFile> computed_outputs; - EXPECT_TRUE(target.GetOutputsAsSourceFiles(LocationRange(), true, - &computed_outputs, &err)); - ASSERT_EQ(3u, computed_outputs.size()); - EXPECT_EQ("//out/Debug/a.dll.lib", computed_outputs[0].value()); - EXPECT_EQ("//out/Debug/a.dll", computed_outputs[1].value()); - EXPECT_EQ("//out/Debug/a.pdb", computed_outputs[2].value()); -} // Tests Target::GetOutputFilesForSource for binary targets (these require a // tool definition). Also tests GetOutputsAsSourceFiles() for source sets. TEST_F(TargetTest, GetOutputFilesForSource_Binary) {