cpp runfiles library: precompute envvars map

Compute the list of environment variables when
creating the runfiles library instead of computing
it on-demand.

Since we already have the manifest and directory
path from Runfiles::PathsFrom, creating the vector
is cheap, though we have to store it even if we
don't use it later.

In a subsequent change I'll merge the manifest-
and directory-based implementations into
RunfilesImpl, and collapse RunfilesImpl into the
Runfiles base class. The point of that is to make
the runfiles library be able to handle both the
directory-based and the manifest-based case
simultaneously, dependening on what is available
to it in the filesystem.

See https://github.com/bazelbuild/bazel/issues/4460

Change-Id: I56310423528df2d0f7494f45904150193368e2d6

Closes #5218.

Change-Id: I73d1f44611c4b0a73c41162319d0ba7a8a4ae6d7
PiperOrigin-RevId: 197543865

3 files changed, 52 insertions, 50 deletions

diff --git a/tools/cpp/runfiles/runfiles.cc b/tools/cpp/runfiles/runfiles.cc
index 4032ed95a1..9aff900a81 100644
--- a/tools/cpp/runfiles/runfiles.cc
+++ b/tools/cpp/runfiles/runfiles.cc
@@ -88,9 +88,15 @@ class RunfilesImpl : public Runfiles {
   // Runfiles::Rlocation for requirements.
   virtual string RlocationChecked(const string& path) const = 0;
 
+  const vector<pair<string, string> >& EnvVars() const { return envvars_; }
+
  protected:
-  RunfilesImpl() {}
+  RunfilesImpl(const vector<pair<string, string> >&& envvars)
+      : envvars_(std::move(envvars)) {}
   virtual ~RunfilesImpl() {}
+
+ private:
+  const vector<pair<string, string> > envvars_;
 };
 
 // Runfiles implementation that parses a runfiles-manifest to look up runfiles.
@@ -99,21 +105,25 @@ class ManifestBased : public RunfilesImpl {
   // 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, string* error);
+  static ManifestBased* Create(const string& manifest_path,
+                               const vector<pair<string, string> >&& envvars,
+                               string* error);
 
-  vector<pair<string, string> > EnvVars() const override;
   string RlocationChecked(const string& path) const override;
 
  private:
-  ManifestBased(const string& manifest_path, map<string, string>&& runfiles_map)
-      : manifest_path_(manifest_path), runfiles_map_(runfiles_map) {}
+  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;
 
-  string RunfilesDir() const;
   static bool ParseManifest(const string& path, map<string, string>* result,
                             string* error);
 
@@ -124,9 +134,10 @@ class ManifestBased : public RunfilesImpl {
 // Runfiles implementation that appends runfiles paths to the runfiles root.
 class DirectoryBased : public RunfilesImpl {
  public:
-  DirectoryBased(string runfiles_path)
-      : runfiles_path_(std::move(runfiles_path)) {}
-  vector<pair<string, string> > EnvVars() const override;
+  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:
@@ -172,10 +183,17 @@ Runfiles* RunfilesImpl::Create(const string& argv0,
     return nullptr;
   }
 
+  const vector<pair<string, string> > envvars = {
+      {"RUNFILES_MANIFEST_FILE", manifest},
+      {"RUNFILES_DIR", directory},
+      // TODO(laszlocsomor): remove JAVA_RUNFILES once the Java launcher can
+      // pick up RUNFILES_DIR.
+      {"JAVA_RUNFILES", directory}};
+
   if (!manifest.empty()) {
-    return ManifestBased::Create(manifest, error);
+    return ManifestBased::Create(manifest, std::move(envvars), error);
   } else {
-    return new DirectoryBased(directory);
+    return new DirectoryBased(directory, std::move(envvars));
   }
 }
 
@@ -217,11 +235,13 @@ string RunfilesImpl::Rlocation(const string& path) const {
   return RlocationChecked(path);
 }
 
-ManifestBased* ManifestBased::Create(const string& manifest_path,
-                                     string* error) {
+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))
+             ? new ManifestBased(manifest_path, std::move(runfiles),
+                                 std::move(envvars))
              : nullptr;
 }
 
@@ -230,26 +250,6 @@ string ManifestBased::RlocationChecked(const string& path) const {
   return std::move(value == runfiles_map_.end() ? string() : value->second);
 }
 
-vector<pair<string, string> > ManifestBased::EnvVars() const {
-  return std::move(vector<pair<string, string> >(
-      {std::make_pair("RUNFILES_MANIFEST_FILE", manifest_path_),
-       // TODO(laszlocsomor): remove JAVA_RUNFILES once the Java launcher can
-       // pick up RUNFILES_DIR.
-       std::make_pair("JAVA_RUNFILES", RunfilesDir())}));
-}
-
-string ManifestBased::RunfilesDir() const {
-  const auto pos1 = manifest_path_.size() - 9;   // "_MANIFEST"
-  const auto pos2 = manifest_path_.size() - 18;  // ".runfiles_manifest"
-  if (manifest_path_.rfind("/MANIFEST") == pos1 ||
-      manifest_path_.rfind("\\MANIFEST") == pos1 ||
-      manifest_path_.rfind(".runfiles_manifest") == pos2) {
-    return std::move(manifest_path_.substr(0, pos1));  // remove ".MANIFEST"
-  } else {
-    return std::move(string());
-  }
-}
-
 bool ManifestBased::ParseManifest(const string& path,
                                   map<string, string>* result, string* error) {
   std::ifstream stm(path);
@@ -288,14 +288,6 @@ string DirectoryBased::RlocationChecked(const string& path) const {
   return std::move(runfiles_path_ + "/" + path);
 }
 
-vector<pair<string, string> > DirectoryBased::EnvVars() const {
-  return std::move(vector<pair<string, string> >(
-      {std::make_pair("RUNFILES_DIR", runfiles_path_),
-       // TODO(laszlocsomor): remove JAVA_RUNFILES once the Java launcher can
-       // pick up RUNFILES_DIR.
-       std::make_pair("JAVA_RUNFILES", runfiles_path_)}));
-}
-
 }  // namespace
 
 namespace testing {
@@ -326,7 +318,11 @@ Runfiles* Runfiles::Create(const string& argv0, string* error) {
 
 Runfiles* Runfiles::CreateManifestBased(const string& manifest_path,
                                         string* error) {
-  return ManifestBased::Create(manifest_path, error);
+  return ManifestBased::Create(manifest_path,
+                               {{"RUNFILES_MANIFEST_FILE", manifest_path},
+                                {"RUNFILES_DIR", ""},
+                                {"JAVA_RUNFILES", ""}},
+                               error);
 }
 
 Runfiles* Runfiles::CreateDirectoryBased(const string& directory_path,
@@ -334,7 +330,10 @@ Runfiles* Runfiles::CreateDirectoryBased(const string& directory_path,
   // 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);
+  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,
diff --git a/tools/cpp/runfiles/runfiles.h b/tools/cpp/runfiles/runfiles.h
index 32c761ffed..b6d26c6387 100644
--- a/tools/cpp/runfiles/runfiles.h
+++ b/tools/cpp/runfiles/runfiles.h
@@ -136,7 +136,8 @@ class Runfiles {
   // The caller should set the returned key-value pairs in the environment of
   // subprocesses in case those subprocesses are also Bazel-built binaries that
   // need to use runfiles.
-  virtual std::vector<std::pair<std::string, std::string> > EnvVars() const = 0;
+  virtual const std::vector<std::pair<std::string, std::string> >& EnvVars()
+      const = 0;
 
   // Computes the path of the runfiles manifest and the runfiles directory.
   //
diff --git a/tools/cpp/runfiles/runfiles_test.cc b/tools/cpp/runfiles/runfiles_test.cc
index 40c405b666..9b8f61cf43 100644
--- a/tools/cpp/runfiles/runfiles_test.cc
+++ b/tools/cpp/runfiles/runfiles_test.cc
@@ -288,9 +288,10 @@ TEST_F(RunfilesTest, ManifestBasedRunfilesEnvVars) {
     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()},
-         {"JAVA_RUNFILES", expected_runfiles_dir}});
+    vector<pair<string, string> > expected = {
+        {"RUNFILES_MANIFEST_FILE", mf->Path()},
+        {"RUNFILES_DIR", ""},
+        {"JAVA_RUNFILES", ""}};
     EXPECT_EQ(r->EnvVars(), expected) << " (suffix=\"" << suffixes[i] << "\")";
   }
 }
@@ -349,8 +350,9 @@ TEST_F(RunfilesTest, DirectoryBasedRunfilesEnvVars) {
   ASSERT_NE(r, nullptr);
   EXPECT_TRUE(error.empty());
 
-  vector<pair<string, string> > expected(
-      {{"RUNFILES_DIR", "runfiles/dir"}, {"JAVA_RUNFILES", "runfiles/dir"}});
+  vector<pair<string, string> > expected = {{"RUNFILES_MANIFEST_FILE", ""},
+                                            {"RUNFILES_DIR", "runfiles/dir"},
+                                            {"JAVA_RUNFILES", "runfiles/dir"}};
   EXPECT_EQ(r->EnvVars(), expected);
 }