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

3 files changed, 178 insertions, 183 deletions

diff --git a/tools/cpp/runfiles/runfiles.cc b/tools/cpp/runfiles/runfiles.cc
index 9aff900a81..0cdc2a0c21 100644
--- a/tools/cpp/runfiles/runfiles.cc
+++ b/tools/cpp/runfiles/runfiles.cc
@@ -82,71 +82,26 @@ class RunfilesImpl : public Runfiles {
 
   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 @@ Runfiles* RunfilesImpl::Create(const string& argv0,
                                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 @@ Runfiles* RunfilesImpl::Create(const string& argv0,
       // 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 @@ string RunfilesImpl::Rlocation(const string& path) const {
   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 @@ bool ManifestBased::ParseManifest(const string& path,
   return true;
 }
 
-string DirectoryBased::RlocationChecked(const string& path) const {
-  return std::move(runfiles_path_ + "/" + path);
-}
-
 }  // namespace
 
 namespace testing {
@@ -316,26 +269,6 @@ Runfiles* Runfiles::Create(const string& argv0, string* error) {
       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 b6d26c6387..5e9db580bf 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 @@ class Runfiles {
   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 9b8f61cf43..6b84fa51e0 100644
--- a/tools/cpp/runfiles/runfiles_test.cc
+++ b/tools/cpp/runfiles/runfiles_test.cc
@@ -62,6 +62,11 @@ class RunfilesTest : public ::testing::Test {
     ~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 @@ class RunfilesTest : public ::testing::Test {
     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 @@ function<string(const string&)> RunfilesTest::kEnvWithTestSrcdir =
       }
     };
 
+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 @@ TEST_F(RunfilesTest, CreatesManifestBasedRunfilesFromManifestNextToBinary) {
   // 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 @@ TEST_F(RunfilesTest,
   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 @@ TEST_F(RunfilesTest, CreatesManifestBasedRunfilesFromEnvvar) {
         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 @@ TEST_F(RunfilesTest, CreatesManifestBasedRunfilesFromEnvvar) {
   // 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 @@ TEST_F(RunfilesTest, CannotCreateManifestBasedRunfilesDueToBadManifest) {
   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 @@ TEST_F(RunfilesTest, ManifestBasedRunfilesRlocation) {
   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 @@ TEST_F(RunfilesTest, DirectoryBasedRunfilesRlocation) {
   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 @@ TEST_F(RunfilesTest, ManifestBasedRunfilesEnvVars) {
     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());
-
-    // 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] << "\")";
+    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.
+      AssertEnvvars(*r, mf->Path(), "");
+    } else {
+      ASSERT_EQ(r, nullptr) << " (suffix=\"" << suffixes[i] << "\")";
+    }
   }
 }
 
@@ -313,12 +413,14 @@ TEST_F(RunfilesTest, CreatesDirectoryBasedRunfilesFromDirectoryNextToBinary) {
   // 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 @@ TEST_F(RunfilesTest, CreatesDirectoryBasedRunfilesFromEnvvar) {
   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 @@ TEST_F(RunfilesTest, FailsToCreateAnyRunfilesBecauseEnvvarsAreNotDefined) {
   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 {