runfiles,cc: Runfiles uses manifest AND directory Change the C++ runfiles library so it can use the runfiles manifest (if present) and the runfiles directory (if present) simultaneously. If the Runfiles object fails to look up a runfile from the manifest (or the manifest didn't exist or wasn't found) then it looks it up from the runfiles directory. This change allows using the same binary with and without a runfiles tree, which is often what sets local and remote runs apart. https://github.com/bazelbuild/bazel/issues/4460 Change-Id: Iae879ff084ba084fcd7c111638ddeae4c6754f4f Closes #5235. Change-Id: Iae879ff084ba084fcd7c111638ddeae4c6754f4f PiperOrigin-RevId: 197726412
diff --git a/tools/cpp/runfiles/runfiles.cc b/tools/cpp/runfiles/runfiles.cc index 9aff900..0cdc2a0 100644 --- a/tools/cpp/runfiles/runfiles.cc +++ b/tools/cpp/runfiles/runfiles.cc
@@ -82,71 +82,26 @@ string Rlocation(const string& path) const override; - // Returns the runtime-location of a given runfile. - // - // This method assumes that the caller already validated the `path`. See - // Runfiles::Rlocation for requirements. - virtual string RlocationChecked(const string& path) const = 0; - - const vector<pair<string, string> >& EnvVars() const { return envvars_; } + const vector<pair<string, string> >& EnvVars() const override { + return envvars_; + } protected: - RunfilesImpl(const vector<pair<string, string> >&& envvars) - : envvars_(std::move(envvars)) {} + RunfilesImpl(const map<string, string>&& runfiles_map, + const string&& directory, + const vector<pair<string, string> >&& envvars) + : runfiles_map_(std::move(runfiles_map)), + directory_(std::move(directory)), + envvars_(std::move(envvars)) {} virtual ~RunfilesImpl() {} private: - const vector<pair<string, string> > envvars_; -}; - -// Runfiles implementation that parses a runfiles-manifest to look up runfiles. -class ManifestBased : public RunfilesImpl { - public: - // Returns a new `ManifestBased` instance. - // Reads the file at `manifest_path` to build a map of the runfiles. - // Returns nullptr upon failure. - static ManifestBased* Create(const string& manifest_path, - const vector<pair<string, string> >&& envvars, - string* error); - - string RlocationChecked(const string& path) const override; - - private: - ManifestBased(const string& manifest_path, - const map<string, string>&& runfiles_map, - const vector<pair<string, string> >&& envvars) - : RunfilesImpl(std::move(envvars)), - manifest_path_(manifest_path), - runfiles_map_(std::move(runfiles_map)) {} - - ManifestBased(const ManifestBased&) = delete; - ManifestBased(ManifestBased&&) = delete; - ManifestBased& operator=(const ManifestBased&) = delete; - ManifestBased& operator=(ManifestBased&&) = delete; - static bool ParseManifest(const string& path, map<string, string>* result, string* error); - const string manifest_path_; const map<string, string> runfiles_map_; -}; - -// Runfiles implementation that appends runfiles paths to the runfiles root. -class DirectoryBased : public RunfilesImpl { - public: - DirectoryBased(string runfiles_path, - const vector<pair<string, string> >&& envvars) - : RunfilesImpl(std::move(envvars)), - runfiles_path_(std::move(runfiles_path)) {} - string RlocationChecked(const string& path) const override; - - private: - DirectoryBased(const DirectoryBased&) = delete; - DirectoryBased(DirectoryBased&&) = delete; - DirectoryBased& operator=(const DirectoryBased&) = delete; - DirectoryBased& operator=(DirectoryBased&&) = delete; - - const string runfiles_path_; + const string directory_; + const vector<pair<string, string> > envvars_; }; bool IsReadableFile(const string& path) { @@ -168,12 +123,18 @@ function<string(const string&)> env_lookup, string* error) { string manifest, directory; - if (!Runfiles::PathsFrom( - argv0, env_lookup("RUNFILES_MANIFEST_FILE"), - env_lookup("RUNFILES_DIR"), - [](const string& path) { return IsReadableFile(path); }, - [](const string& path) { return IsDirectory(path); }, &manifest, - &directory)) { + if (!Runfiles::PathsFrom(argv0, env_lookup("RUNFILES_MANIFEST_FILE"), + env_lookup("RUNFILES_DIR"), + [](const string& path) { + return (ends_with(path, "MANIFEST") || + ends_with(path, ".runfiles_manifest")) && + IsReadableFile(path); + }, + [](const string& path) { + return ends_with(path, ".runfiles") && + IsDirectory(path); + }, + &manifest, &directory)) { if (error) { std::ostringstream err; err << "ERROR: " << __FILE__ << "(" << __LINE__ @@ -190,11 +151,15 @@ // pick up RUNFILES_DIR. {"JAVA_RUNFILES", directory}}; + map<string, string> runfiles; if (!manifest.empty()) { - return ManifestBased::Create(manifest, std::move(envvars), error); - } else { - return new DirectoryBased(directory, std::move(envvars)); + if (!ParseManifest(manifest, &runfiles, error)) { + return nullptr; + } } + + return new RunfilesImpl(std::move(runfiles), std::move(directory), + std::move(envvars)); } bool IsAbsolute(const string& path) { @@ -232,26 +197,18 @@ if (IsAbsolute(path)) { return path; } - return RlocationChecked(path); -} - -ManifestBased* ManifestBased::Create( - const string& manifest_path, const vector<pair<string, string> >&& envvars, - string* error) { - map<string, string> runfiles; - return ParseManifest(manifest_path, &runfiles, error) - ? new ManifestBased(manifest_path, std::move(runfiles), - std::move(envvars)) - : nullptr; -} - -string ManifestBased::RlocationChecked(const string& path) const { const auto value = runfiles_map_.find(path); - return std::move(value == runfiles_map_.end() ? string() : value->second); + if (value != runfiles_map_.end()) { + return value->second; + } + if (!directory_.empty()) { + return directory_ + "/" + path; + } + return ""; } -bool ManifestBased::ParseManifest(const string& path, - map<string, string>* result, string* error) { +bool RunfilesImpl::ParseManifest(const string& path, + map<string, string>* result, string* error) { std::ifstream stm(path); if (!stm.is_open()) { if (error) { @@ -284,10 +241,6 @@ return true; } -string DirectoryBased::RlocationChecked(const string& path) const { - return std::move(runfiles_path_ + "/" + path); -} - } // namespace namespace testing { @@ -316,26 +269,6 @@ error); } -Runfiles* Runfiles::CreateManifestBased(const string& manifest_path, - string* error) { - return ManifestBased::Create(manifest_path, - {{"RUNFILES_MANIFEST_FILE", manifest_path}, - {"RUNFILES_DIR", ""}, - {"JAVA_RUNFILES", ""}}, - error); -} - -Runfiles* Runfiles::CreateDirectoryBased(const string& directory_path, - string* error) { - // Note: `error` is intentionally unused because we don't expect any errors - // here. We expect an `error` pointer so that we may use it in the future if - // need be, without having to change the API. - return new DirectoryBased(directory_path, - {{"RUNFILES_MANIFEST_FILE", ""}, - {"RUNFILES_DIR", directory_path}, - {"JAVA_RUNFILES", directory_path}}); -} - bool Runfiles::PathsFrom(const string& argv0, string mf, string dir, function<bool(const string&)> is_runfiles_manifest, function<bool(const string&)> is_runfiles_directory,
diff --git a/tools/cpp/runfiles/runfiles.h b/tools/cpp/runfiles/runfiles.h index b6d26c6..5e9db58 100644 --- a/tools/cpp/runfiles/runfiles.h +++ b/tools/cpp/runfiles/runfiles.h
@@ -36,15 +36,6 @@ // finding appropriate environment variables that tell it where to find the // manifest or directory. See `Runfiles::Create` for more info. // -// If you want to explicitly create a manifest- or directory-based -// implementation, you can do so as follows: -// -// std::unique_ptr<Runfiles> runfiles1( -// Runfiles::CreateManifestBased(path/to/foo.runfiles/MANIFEST", &error)); -// -// std::unique_ptr<Runfiles> runfiles2( -// Runfiles::CreateDirectoryBased(path/to/foo.runfiles", &error)); -// // If you want to start child processes that also need runfiles, you need to set // the right environment variables for them: // @@ -101,18 +92,6 @@ static Runfiles* Create(const std::string& argv0, std::string* error = nullptr); - // Returns a new manifest-based `Runfiles` instance. - // Returns nullptr on error. If `error` is provided, the method prints an - // error message into it. - static Runfiles* CreateManifestBased(const std::string& manifest_path, - std::string* error = nullptr); - - // Returns a new directory-based `Runfiles` instance. - // Returns nullptr on error. If `error` is provided, the method prints an - // error message into it. - static Runfiles* CreateDirectoryBased(const std::string& directory_path, - std::string* error = nullptr); - // Returns the runtime path of a runfile. // // Runfiles are data-dependencies of Bazel-built binaries and tests.
diff --git a/tools/cpp/runfiles/runfiles_test.cc b/tools/cpp/runfiles/runfiles_test.cc index 9b8f61c..6b84fa5 100644 --- a/tools/cpp/runfiles/runfiles_test.cc +++ b/tools/cpp/runfiles/runfiles_test.cc
@@ -62,6 +62,11 @@ ~MockFile(); const string& Path() const { return path_; } + string DirName() const { + string::size_type pos = path_.find_last_of('/'); + return pos == string::npos ? "" : path_.substr(0, pos); + } + private: MockFile(const string& path) : path_(path) {} MockFile(const MockFile&) = delete; @@ -72,6 +77,10 @@ const string path_; }; + void AssertEnvvars(const Runfiles& runfiles, + const string& expected_manifest_file, + const string& expected_directory); + static string GetTemp(); static function<string(const string&)> kEnvWithTestSrcdir; @@ -86,6 +95,16 @@ } }; +void RunfilesTest::AssertEnvvars(const Runfiles& runfiles, + const string& expected_manifest_file, + const string& expected_directory) { + vector<pair<string, string> > expected = { + {"RUNFILES_MANIFEST_FILE", expected_manifest_file}, + {"RUNFILES_DIR", expected_directory}, + {"JAVA_RUNFILES", expected_directory}}; + ASSERT_EQ(runfiles.EnvVars(), expected); +} + string RunfilesTest::GetTemp() { #ifdef COMPILER_MSVC DWORD size = ::GetEnvironmentVariableA("TEST_TMPDIR", NULL, 0); @@ -152,6 +171,7 @@ // We know it's manifest-based because it returns empty string for unknown // paths. EXPECT_EQ(r->Rlocation("unknown"), ""); + AssertEnvvars(*r, mf->Path(), ""); } TEST_F(RunfilesTest, @@ -168,9 +188,8 @@ ASSERT_NE(r, nullptr); EXPECT_TRUE(error.empty()); EXPECT_EQ(r->Rlocation("a/b"), "c/d"); - // We know it's manifest-based because it returns empty string for unknown - // paths. - EXPECT_EQ(r->Rlocation("unknown"), ""); + EXPECT_EQ(r->Rlocation("foo"), argv0 + ".runfiles/foo"); + AssertEnvvars(*r, mf->Path(), argv0 + ".runfiles"); } TEST_F(RunfilesTest, CreatesManifestBasedRunfilesFromEnvvar) { @@ -185,7 +204,7 @@ if (key == "RUNFILES_MANIFEST_FILE") { return mf->Path(); } else if (key == "RUNFILES_DIR") { - return string("ignored when RUNFILES_MANIFEST_FILE has a value"); + return string("non-existent"); } else if (key == "TEST_SRCDIR") { return string("always ignored"); } else { @@ -199,6 +218,7 @@ // We know it's manifest-based because it returns empty string for unknown // paths. EXPECT_EQ(r->Rlocation("unknown"), ""); + AssertEnvvars(*r, mf->Path(), ""); } TEST_F(RunfilesTest, CannotCreateManifestBasedRunfilesDueToBadManifest) { @@ -207,19 +227,38 @@ EXPECT_TRUE(mf != nullptr); string error; - unique_ptr<Runfiles> r(Runfiles::CreateManifestBased(mf->Path(), &error)); + unique_ptr<Runfiles> r( + TestOnly_CreateRunfiles("ignore-argv0", + [&mf](const string& key) { + if (key == "RUNFILES_MANIFEST_FILE") { + return mf->Path(); + } else { + return string(); + } + }, + &error)); ASSERT_EQ(r, nullptr); EXPECT_NE(error.find("bad runfiles manifest entry"), string::npos); EXPECT_NE(error.find("line #2: \"nospace\""), string::npos); } -TEST_F(RunfilesTest, ManifestBasedRunfilesRlocation) { +TEST_F(RunfilesTest, ManifestBasedRunfilesRlocationAndEnvVars) { unique_ptr<MockFile> mf( MockFile::Create("foo" LINE() ".runfiles_manifest", {"a/b c/d"})); EXPECT_TRUE(mf != nullptr); string error; - unique_ptr<Runfiles> r(Runfiles::CreateManifestBased(mf->Path(), &error)); + unique_ptr<Runfiles> r( + TestOnly_CreateRunfiles("ignore-argv0", + [&mf](const string& key) { + if (key == "RUNFILES_MANIFEST_FILE") { + return mf->Path(); + } else { + return string(); + } + }, + &error)); + ASSERT_NE(r, nullptr); EXPECT_TRUE(error.empty()); EXPECT_EQ(r->Rlocation("a/b"), "c/d"); @@ -242,18 +281,31 @@ EXPECT_EQ(r->Rlocation("c:\\Foo"), "c:\\Foo"); } -TEST_F(RunfilesTest, DirectoryBasedRunfilesRlocation) { +TEST_F(RunfilesTest, DirectoryBasedRunfilesRlocationAndEnvVars) { + unique_ptr<MockFile> dummy( + MockFile::Create("foo" LINE() ".runfiles/dummy", {"a/b c/d"})); + EXPECT_TRUE(dummy != nullptr); + string dir = dummy->DirName(); + string error; - unique_ptr<Runfiles> r(Runfiles::CreateDirectoryBased("whatever", &error)); + unique_ptr<Runfiles> r(TestOnly_CreateRunfiles("ignore-argv0", + [dir](const string& key) { + if (key == "RUNFILES_DIR") { + return dir; + } else { + return string(); + } + }, + &error)); ASSERT_NE(r, nullptr); EXPECT_TRUE(error.empty()); - EXPECT_EQ(r->Rlocation("a/b"), "whatever/a/b"); - EXPECT_EQ(r->Rlocation("c/d"), "whatever/c/d"); + EXPECT_EQ(r->Rlocation("a/b"), dir + "/a/b"); + EXPECT_EQ(r->Rlocation("c/d"), dir + "/c/d"); EXPECT_EQ(r->Rlocation(""), ""); - EXPECT_EQ(r->Rlocation("foo"), "whatever/foo"); - EXPECT_EQ(r->Rlocation("foo/"), "whatever/foo/"); - EXPECT_EQ(r->Rlocation("foo/bar"), "whatever/foo/bar"); + EXPECT_EQ(r->Rlocation("foo"), dir + "/foo"); + EXPECT_EQ(r->Rlocation("foo/"), dir + "/foo/"); + EXPECT_EQ(r->Rlocation("foo/bar"), dir + "/foo/bar"); EXPECT_EQ(r->Rlocation("../foo"), ""); EXPECT_EQ(r->Rlocation("foo/.."), ""); EXPECT_EQ(r->Rlocation("foo/../bar"), ""); @@ -266,6 +318,48 @@ EXPECT_EQ(r->Rlocation("/Foo"), "/Foo"); EXPECT_EQ(r->Rlocation("c:/Foo"), "c:/Foo"); EXPECT_EQ(r->Rlocation("c:\\Foo"), "c:\\Foo"); + AssertEnvvars(*r, "", dir); +} + +TEST_F(RunfilesTest, ManifestAndDirectoryBasedRunfilesRlocationAndEnvVars) { + unique_ptr<MockFile> mf( + MockFile::Create("foo" LINE() ".runfiles/MANIFEST", {"a/b c/d"})); + EXPECT_TRUE(mf != nullptr); + string dir = mf->DirName(); + + string error; + unique_ptr<Runfiles> r( + TestOnly_CreateRunfiles("ignore-argv0", + [&mf](const string& key) { + if (key == "RUNFILES_MANIFEST_FILE") { + return mf->Path(); + } else { + return string(); + } + }, + &error)); + + ASSERT_NE(r, nullptr); + EXPECT_TRUE(error.empty()); + EXPECT_EQ(r->Rlocation("a/b"), "c/d"); + EXPECT_EQ(r->Rlocation("c/d"), dir + "/c/d"); + EXPECT_EQ(r->Rlocation(""), ""); + EXPECT_EQ(r->Rlocation("foo"), dir + "/foo"); + EXPECT_EQ(r->Rlocation("foo/"), dir + "/foo/"); + EXPECT_EQ(r->Rlocation("foo/bar"), dir + "/foo/bar"); + EXPECT_EQ(r->Rlocation("../foo"), ""); + EXPECT_EQ(r->Rlocation("foo/.."), ""); + EXPECT_EQ(r->Rlocation("foo/../bar"), ""); + EXPECT_EQ(r->Rlocation("./foo"), ""); + EXPECT_EQ(r->Rlocation("foo/."), ""); + EXPECT_EQ(r->Rlocation("foo/./bar"), ""); + EXPECT_EQ(r->Rlocation("//foo"), ""); + EXPECT_EQ(r->Rlocation("foo//"), ""); + EXPECT_EQ(r->Rlocation("foo//bar"), ""); + EXPECT_EQ(r->Rlocation("/Foo"), "/Foo"); + EXPECT_EQ(r->Rlocation("c:/Foo"), "c:/Foo"); + EXPECT_EQ(r->Rlocation("c:\\Foo"), "c:\\Foo"); + AssertEnvvars(*r, mf->Path(), dir); } TEST_F(RunfilesTest, ManifestBasedRunfilesEnvVars) { @@ -278,21 +372,27 @@ EXPECT_TRUE(mf != nullptr) << " (suffix=\"" << suffixes[i] << "\")"; string error; - unique_ptr<Runfiles> r(Runfiles::CreateManifestBased(mf->Path(), &error)); - ASSERT_NE(r, nullptr) << " (suffix=\"" << suffixes[i] << "\")"; - EXPECT_TRUE(error.empty()); + unique_ptr<Runfiles> r( + TestOnly_CreateRunfiles("ignore-argv0", + [&mf](const string& key) { + if (key == "RUNFILES_MANIFEST_FILE") { + return mf->Path(); + } else { + return string(); + } + }, + &error)); + if (i < 2) { + ASSERT_NE(r, nullptr) << " (suffix=\"" << suffixes[i] << "\")"; + EXPECT_TRUE(error.empty()); - // The object can compute the runfiles directory when i=0 and i=1, but not - // when i>1 because the manifest file's name doesn't end in a well-known - // way. - const string expected_runfiles_dir( - i < 2 ? mf->Path().substr(0, mf->Path().size() - 9 /* "_manifest" */) - : ""); - vector<pair<string, string> > expected = { - {"RUNFILES_MANIFEST_FILE", mf->Path()}, - {"RUNFILES_DIR", ""}, - {"JAVA_RUNFILES", ""}}; - EXPECT_EQ(r->EnvVars(), expected) << " (suffix=\"" << suffixes[i] << "\")"; + // The object can compute the runfiles directory when i=0 and i=1, but not + // when i>1 because the manifest file's name doesn't end in a well-known + // way. + AssertEnvvars(*r, mf->Path(), ""); + } else { + ASSERT_EQ(r, nullptr) << " (suffix=\"" << suffixes[i] << "\")"; + } } } @@ -313,12 +413,14 @@ // We know it's directory-based because it returns some result for unknown // paths. EXPECT_EQ(r->Rlocation("unknown"), argv0 + ".runfiles/unknown"); + AssertEnvvars(*r, "", argv0 + ".runfiles"); } TEST_F(RunfilesTest, CreatesDirectoryBasedRunfilesFromEnvvar) { // We create a directory as a side-effect of creating a mock file. - unique_ptr<MockFile> mf(MockFile::Create(string("foo" LINE() "/dir/dummy"))); - string dir = mf->Path().substr(0, mf->Path().size() - 6); + unique_ptr<MockFile> mf( + MockFile::Create(string("foo" LINE() ".runfiles/dummy"))); + string dir = mf->DirName(); string error; unique_ptr<Runfiles> r( @@ -341,31 +443,12 @@ EXPECT_EQ(r->Rlocation("/Foo"), "/Foo"); EXPECT_EQ(r->Rlocation("c:/Foo"), "c:/Foo"); EXPECT_EQ(r->Rlocation("c:\\Foo"), "c:\\Foo"); -} - -TEST_F(RunfilesTest, DirectoryBasedRunfilesEnvVars) { - string error; - unique_ptr<Runfiles> r( - Runfiles::CreateDirectoryBased("runfiles/dir", &error)); - ASSERT_NE(r, nullptr); - EXPECT_TRUE(error.empty()); - - vector<pair<string, string> > expected = {{"RUNFILES_MANIFEST_FILE", ""}, - {"RUNFILES_DIR", "runfiles/dir"}, - {"JAVA_RUNFILES", "runfiles/dir"}}; - EXPECT_EQ(r->EnvVars(), expected); -} - -TEST_F(RunfilesTest, FailsToCreateManifestBasedBecauseManifestDoesNotExist) { - string error; - unique_ptr<Runfiles> r( - Runfiles::CreateManifestBased("non-existent-file", &error)); - ASSERT_EQ(r, nullptr); - EXPECT_NE(error.find("cannot open runfiles manifest"), string::npos); + AssertEnvvars(*r, "", dir); } TEST_F(RunfilesTest, FailsToCreateAnyRunfilesBecauseEnvvarsAreNotDefined) { - unique_ptr<MockFile> mf(MockFile::Create(string("foo" LINE() "/dir/dummy"))); + unique_ptr<MockFile> mf( + MockFile::Create(string("foo" LINE() ".runfiles/MANIFEST"))); EXPECT_TRUE(mf != nullptr); string error; @@ -387,11 +470,11 @@ EXPECT_TRUE(error.empty()); // We create a directory as a side-effect of creating a mock file. - string dir = mf->Path().substr(0, mf->Path().size() - 6); + mf.reset(MockFile::Create(string("foo" LINE() ".runfiles/dummy"))); r.reset(TestOnly_CreateRunfiles("ignore-argv0", - [dir](const string& key) { + [&mf](const string& key) { if (key == "RUNFILES_DIR") { - return dir; + return mf->DirName(); } else if (key == "TEST_SRCDIR") { return string("always ignored"); } else {