win: Use relative path for python in ninja files if possible On Windows, `BuildSettings::python_path` returns the absolute path to the script executable, even if it is within the root directory. This makes the ninja files non-portable because they include the absolute path on the machine where `gn` was run. This commit makes it such that if python is within the root directory, we will use a relative path to it from the build directory when generating the ninja rules. This will allow us to setup siso rules to execute python scripts on remote machines for Windows. Note that we need to resolve the python path back to an absolute path when executing scripts within GN. Change-Id: I1a1c2895efd868fb4cc6c9b8e2c61167bfe59211 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/20240 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: David Turner <digit@google.com> Reviewed-by: David Turner <digit@google.com>
diff --git a/src/gn/build_settings.cc b/src/gn/build_settings.cc index 6a26495..6d66376 100644 --- a/src/gn/build_settings.cc +++ b/src/gn/build_settings.cc
@@ -17,6 +17,8 @@ root_path_utf8_(other.root_path_utf8_), secondary_source_path_(other.secondary_source_path_), python_path_(other.python_path_), + python_path_is_relative_to_build_dir_( + other.python_path_is_relative_to_build_dir_), ninja_required_version_(other.ninja_required_version_), build_config_file_(other.build_config_file_), arg_file_template_path_(other.arg_file_template_path_), @@ -41,6 +43,20 @@ secondary_source_path_ = GetFullPath(d).NormalizePathSeparatorsTo('/'); } +void BuildSettings::SetPythonPath(base::FilePath p) { + python_path_ = std::move(p); + python_path_is_relative_to_build_dir_ = false; + + // If the provided python path is absolute and is within the root source tree, + // make it relative to the build directory. This helps keep generated ninja + // files free of absolute paths when possible. + if (python_path_.IsAbsolute() && root_path_.IsParent(python_path_)) { + python_path_ = UTF8ToFilePath( + RebasePath(FilePathToUTF8(python_path_), build_dir_, root_path_utf8_)); + python_path_is_relative_to_build_dir_ = true; + } +} + void BuildSettings::SetBuildDir(const SourceDir& d) { build_dir_ = d; }
diff --git a/src/gn/build_settings.h b/src/gn/build_settings.h index e21851a..6c48284 100644 --- a/src/gn/build_settings.h +++ b/src/gn/build_settings.h
@@ -60,7 +60,10 @@ // Path of the python executable to run scripts with. base::FilePath python_path() const { return python_path_; } - void set_python_path(const base::FilePath& p) { python_path_ = p; } + void SetPythonPath(base::FilePath p); + bool python_path_is_relative_to_build_dir() const { + return python_path_is_relative_to_build_dir_; + } // Required Ninja version. const Version& ninja_required_version() const { @@ -149,6 +152,7 @@ std::string root_path_utf8_; base::FilePath secondary_source_path_; base::FilePath python_path_; + bool python_path_is_relative_to_build_dir_ = false; // See 40045b9 for the reason behind using 1.7.2 as the default version. Version ninja_required_version_{1, 7, 2};
diff --git a/src/gn/function_exec_script.cc b/src/gn/function_exec_script.cc index 6f8bdf5..01cdb66 100644 --- a/src/gn/function_exec_script.cc +++ b/src/gn/function_exec_script.cc
@@ -182,11 +182,17 @@ // that the arguments will be passed through exactly as specified. cmdline.SetParseSwitches(false); + base::FilePath startup_dir = + build_settings->GetFullPath(build_settings->build_dir()); + // If an interpreter path is set, initialize it as the first entry and // pass script_path as the first argument. Otherwise, set the // program to script_path directly. - const base::FilePath& interpreter_path = build_settings->python_path(); + base::FilePath interpreter_path = build_settings->python_path(); if (!interpreter_path.empty()) { + if (build_settings->python_path_is_relative_to_build_dir()) { + interpreter_path = startup_dir.Append(interpreter_path); + } cmdline.SetProgram(interpreter_path); cmdline.AppendArgPath(script_path); } else { @@ -218,8 +224,6 @@ begin_exec = TicksNow(); } - base::FilePath startup_dir = - build_settings->GetFullPath(build_settings->build_dir()); // The first time a build is run, no targets will have been written so the // build output directory won't exist. We need to make sure it does before // running any scripts with this as its startup directory, although it will
diff --git a/src/gn/invoke_python.cc b/src/gn/invoke_python.cc index c54ad72..3f5385f 100644 --- a/src/gn/invoke_python.cc +++ b/src/gn/invoke_python.cc
@@ -20,7 +20,12 @@ const base::FilePath& output_path, bool quiet, Err* err) { - const base::FilePath& python_path = build_settings->python_path(); + base::FilePath startup_dir = + build_settings->GetFullPath(build_settings->build_dir()); + base::FilePath python_path = build_settings->python_path(); + if (build_settings->python_path_is_relative_to_build_dir()) { + python_path = startup_dir.Append(python_path); + } base::CommandLine cmdline(python_path); cmdline.AppendArg("--"); cmdline.AppendArgPath(python_script_path); @@ -28,8 +33,6 @@ if (!python_script_extra_args.empty()) { cmdline.AppendArg(python_script_extra_args); } - base::FilePath startup_dir = - build_settings->GetFullPath(build_settings->build_dir()); std::string output; std::string stderr_output;
diff --git a/src/gn/json_project_writer_unittest.cc b/src/gn/json_project_writer_unittest.cc index 4b58e2c..770e7cf 100644 --- a/src/gn/json_project_writer_unittest.cc +++ b/src/gn/json_project_writer_unittest.cc
@@ -36,7 +36,7 @@ target.action_values().outputs() = SubstitutionList::MakeForTest("//out/Debug/output1.out"); - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); std::vector<const Target*> targets; targets.push_back(&target); @@ -539,7 +539,7 @@ target.action_values().outputs() = SubstitutionList::MakeForTest("//out/Debug/{{source_name_part}}.out"); - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); std::vector<const Target*> targets; targets.push_back(&target);
diff --git a/src/gn/ninja_action_target_writer_unittest.cc b/src/gn/ninja_action_target_writer_unittest.cc index 9b7c76f..8bcea93 100644 --- a/src/gn/ninja_action_target_writer_unittest.cc +++ b/src/gn/ninja_action_target_writer_unittest.cc
@@ -54,7 +54,7 @@ target.SetToolchain(setup.toolchain()); ASSERT_TRUE(target.OnResolved(&err)); - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); std::ostringstream out; @@ -96,7 +96,7 @@ target.SetToolchain(setup.toolchain()); ASSERT_TRUE(target.OnResolved(&err)); - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); std::ostringstream out; @@ -138,7 +138,7 @@ target.SetToolchain(setup.toolchain()); ASSERT_TRUE(target.OnResolved(&err)); - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); std::ostringstream out; @@ -195,7 +195,7 @@ target.SetToolchain(setup.toolchain()); ASSERT_TRUE(target.OnResolved(&err)); - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); std::ostringstream out; @@ -266,7 +266,7 @@ target.SetToolchain(setup.toolchain()); ASSERT_TRUE(target.OnResolved(&err)); - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); std::ostringstream out; @@ -333,7 +333,7 @@ target.config_values().inputs().push_back(SourceFile("//foo/included.txt")); - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); setup.build_settings()->set_ninja_required_version(Version{1, 9, 0}); @@ -391,7 +391,7 @@ target.action_values().outputs() = SubstitutionList::MakeForTest("//out/Debug/{{source_name_part}}.out"); - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); std::ostringstream out; @@ -449,7 +449,7 @@ target.action_values().outputs() = SubstitutionList::MakeForTest("//out/Debug/{{source_name_part}}.out"); - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); std::ostringstream out; @@ -478,7 +478,7 @@ Err err; TestWithScope setup; - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); Target dep(setup.settings(), Label(SourceDir("//foo/"), "dep")); @@ -552,7 +552,7 @@ Err err; TestWithScope setup; - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); Config farcfg(setup.settings(), Label(SourceDir("//foo/"), "farcfg")); @@ -626,7 +626,7 @@ target.SetToolchain(setup.toolchain()); ASSERT_TRUE(target.OnResolved(&err)); - setup.build_settings()->set_python_path( + setup.build_settings()->SetPythonPath( base::FilePath(FILE_PATH_LITERAL("/Program Files/python"))); std::ostringstream out;
diff --git a/src/gn/setup.cc b/src/gn/setup.cc index 49830ae..b29e2ff 100644 --- a/src/gn/setup.cc +++ b/src/gn/setup.cc
@@ -829,7 +829,7 @@ if (cmdline.HasSwitch(switches::kScriptExecutable)) { auto script_executable = cmdline.GetSwitchValuePath(switches::kScriptExecutable); - build_settings_.set_python_path(ProcessFileExtensions(script_executable)); + build_settings_.SetPythonPath(ProcessFileExtensions(script_executable)); } else if (value) { if (!value->VerifyTypeIs(Value::STRING, err)) { return false; @@ -846,7 +846,7 @@ return false; } } - build_settings_.set_python_path(python_path); + build_settings_.SetPythonPath(std::move(python_path)); } else { #if defined(OS_WIN) base::FilePath python_path = @@ -857,9 +857,9 @@ "just \"python.exe\""); python_path = base::FilePath(u"python.exe"); } - build_settings_.set_python_path(python_path); + build_settings_.SetPythonPath(std::move(python_path)); #else - build_settings_.set_python_path(base::FilePath("python")); + build_settings_.SetPythonPath(base::FilePath("python")); #endif } return true;
diff --git a/src/gn/setup_unittest.cc b/src/gn/setup_unittest.cc index d9ace51..547c282 100644 --- a/src/gn/setup_unittest.cc +++ b/src/gn/setup_unittest.cc
@@ -410,3 +410,145 @@ EXPECT_TRUE(dependency_includes_file("build_defines.gni")); EXPECT_TRUE(dependency_includes_file("params.gni")); } + +TEST_F(SetupTest, AbsolutePythonPathInsideRootDir) { + base::CommandLine cmdline(base::CommandLine::NO_PROGRAM); + + const char kDotfileContents[] = R"( +buildconfig = "//BUILDCONFIG.gn" +script_executable = "" +)"; + + // Create a temp directory containing a .gn file and a BUILDCONFIG.gn file, + // pass it as --root. + base::ScopedTempDir in_temp_dir; + ASSERT_TRUE(in_temp_dir.CreateUniqueTempDir()); + base::FilePath in_path = in_temp_dir.GetPath(); + base::FilePath dot_gn_name = in_path.Append(FILE_PATH_LITERAL(".gn")); + WriteFile(dot_gn_name, kDotfileContents); + + base::FilePath absolute_bin_dir = in_path.Append(FILE_PATH_LITERAL("bin")); + base::FilePath script_executable = + absolute_bin_dir.Append(FILE_PATH_LITERAL("python")); + + WriteFile(in_path.Append(FILE_PATH_LITERAL("BUILDCONFIG.gn")), ""); + cmdline.AppendSwitchPath(switches::kRoot, in_path); + cmdline.AppendSwitchPath(switches::kScriptExecutable, script_executable); + + base::FilePath build_dir = in_path.Append(FILE_PATH_LITERAL("out")); + + // Run setup and check that python_path_is_relative_to_build_dir is true and + // python_path is relative to the build directory. + Setup setup; + Err err; + EXPECT_TRUE( + setup.DoSetupWithErr(FilePathToUTF8(build_dir), true, cmdline, &err)); + EXPECT_TRUE(setup.build_settings().python_path_is_relative_to_build_dir()); + EXPECT_EQ(setup.build_settings().python_path(), + base::FilePath(FILE_PATH_LITERAL("../bin/python"))); +} + +TEST_F(SetupTest, AbsolutePythonPathOutsideRootDir) { + base::CommandLine cmdline(base::CommandLine::NO_PROGRAM); + + const char kDotfileContents[] = R"( +buildconfig = "//BUILDCONFIG.gn" +script_executable = "" +)"; + + // Create a temp directory containing a .gn file and a BUILDCONFIG.gn file, + // pass it as --root. + base::ScopedTempDir in_temp_dir; + ASSERT_TRUE(in_temp_dir.CreateUniqueTempDir()); + base::FilePath in_path = in_temp_dir.GetPath(); + base::FilePath dot_gn_name = in_path.Append(FILE_PATH_LITERAL(".gn")); + WriteFile(dot_gn_name, kDotfileContents); + + base::ScopedTempDir other_temp_dir; + ASSERT_TRUE(other_temp_dir.CreateUniqueTempDir()); + base::FilePath absolute_bin_dir = + other_temp_dir.GetPath().Append(FILE_PATH_LITERAL("bin")); + base::FilePath script_executable = + absolute_bin_dir.Append(FILE_PATH_LITERAL("python")); + + WriteFile(in_path.Append(FILE_PATH_LITERAL("BUILDCONFIG.gn")), ""); + cmdline.AppendSwitchPath(switches::kRoot, in_path); + cmdline.AppendSwitchPath(switches::kScriptExecutable, script_executable); + + base::FilePath build_dir = in_path.Append(FILE_PATH_LITERAL("out")); + + // Run setup and check that python_path_is_relative_to_build_dir is false and + // python_path is unchanged (apart from normalization). + Setup setup; + Err err; + EXPECT_TRUE( + setup.DoSetupWithErr(FilePathToUTF8(build_dir), true, cmdline, &err)); + EXPECT_FALSE(setup.build_settings().python_path_is_relative_to_build_dir()); + EXPECT_EQ(setup.build_settings().python_path(), + script_executable.NormalizePathSeparatorsTo('/')); +} + +TEST_F(SetupTest, RelativePythonPath) { + base::CommandLine cmdline(base::CommandLine::NO_PROGRAM); + + const char kDotfileContents[] = R"( +buildconfig = "//BUILDCONFIG.gn" +script_executable = "" +)"; + + // Create a temp directory containing a .gn file and a BUILDCONFIG.gn file, + // pass it as --root. + base::ScopedTempDir in_temp_dir; + ASSERT_TRUE(in_temp_dir.CreateUniqueTempDir()); + base::FilePath in_path = in_temp_dir.GetPath(); + base::FilePath dot_gn_name = in_path.Append(FILE_PATH_LITERAL(".gn")); + WriteFile(dot_gn_name, kDotfileContents); + + base::FilePath relative_bin_dir(FILE_PATH_LITERAL("bin")); + base::FilePath script_executable = + relative_bin_dir.Append(FILE_PATH_LITERAL("python")); + +#if defined(OS_WIN) + // On Windows, if script executable is a relative path, then it must exist in + // either the current directory or PATH with a `.exe` or `.bat` extension, + // otherwise `FindWindowsPython` fails. + base::FilePath original_cwd; + base::GetCurrentDirectory(&original_cwd); + + // Switch to another temporary directory for the test. + base::ScopedTempDir cwd_temp_dir; + ASSERT_TRUE(cwd_temp_dir.CreateUniqueTempDir()); + + base::FilePath absolute_script_executable = cwd_temp_dir.GetPath() + .Append(script_executable) + .ReplaceExtension(u".exe"); + ASSERT_TRUE(base::CreateDirectoryAndGetError( + absolute_script_executable.DirName(), nullptr)); + WriteFile(absolute_script_executable, ""); + base::SetCurrentDirectory(cwd_temp_dir.GetPath()); +#endif + + WriteFile(in_path.Append(FILE_PATH_LITERAL("BUILDCONFIG.gn")), ""); + cmdline.AppendSwitchPath(switches::kRoot, in_path); + cmdline.AppendSwitchPath(switches::kScriptExecutable, script_executable); + + base::FilePath build_dir = in_path.Append(FILE_PATH_LITERAL("out")); + + // Run setup and check that python_path_is_relative_to_build_dir is false and + // python_path is made absolute on Windows, and unchanged on other platforms. + Setup setup; + Err err; + EXPECT_TRUE( + setup.DoSetupWithErr(FilePathToUTF8(build_dir), true, cmdline, &err)); + EXPECT_FALSE(setup.build_settings().python_path_is_relative_to_build_dir()); + +#if defined(OS_WIN) + EXPECT_EQ(setup.build_settings().python_path(), + absolute_script_executable.NormalizePathSeparatorsTo('/')); + + // Change back to the original cwd. + base::SetCurrentDirectory(original_cwd); +#else + EXPECT_EQ(setup.build_settings().python_path(), script_executable); +#endif +}