DRTVWR-588: Eliminate APR and Boost.Phoenix from NamedTempFile.

NamedTempFile used to use APR calls to discover a suitable temp directory,
synthesize a temp filename template, generate the unique file, write its
content and ultimately delete it. This required a reference to gAPRPoolp as
the default value of an optional constructor argument in case some usage
demanded an alternative APR memory pool. It also used Boost.Phoenix
placeholders to magically synthesize a callable. Replace APR calls with
Boost.Filesystem; replace Boost.Phoenix with lambdas. Break out unique path
generation logic as static NamedTempFile::temp_path(). In a nod to GitHub
Actions builds, honor RUNNER_TEMP environment variable if set.

test.cpp's RecordToTempFile need no longer pass an apr_pool_t* to
NamedTempFile.

NamedTempFile's constructor now accepts an optional suffix, making subclass
NamedExtTempFile nearly trivial. It no longer needs to create or remove a
symlink, for which it used to use APR calls.

llprocess_test.cpp's NamedTempDir used to use Python's tempfile.mkdtemp() to
create a temp directory, and apr_dir_remove() to destroy it. Replace both with
NamedTempFile::temp_path() and Boost.Filesystem.

Also add diagnostic output for LLProcess test failure. If llprocess_test
cannot launch a child process, notice the APR_LOG environment variable
recognized by our patched apr_suite to engage logging, and report the contents
of that file.

3 files changed, 111 insertions, 119 deletions

diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp
index 81449b4a42..c48d913069 100644
--- a/indra/llcommon/tests/llprocess_test.cpp
+++ b/indra/llcommon/tests/llprocess_test.cpp
@@ -151,8 +151,38 @@ struct PythonProcessLauncher
     /// Launch Python script; verify that it launched
     void launch()
     {
-        mPy = LLProcess::create(mParams);
-        tut::ensure(STRINGIZE("Couldn't launch " << mDesc << " script"), bool(mPy));
+        try
+        {
+            mPy = LLProcess::create(mParams);
+            tut::ensure(STRINGIZE("Couldn't launch " << mDesc << " script"), bool(mPy));
+        }
+        catch (const tut::failure&)
+        {
+            // On Windows, if APR_LOG is set, our version of APR's
+            // apr_create_proc() logs to the specified file. If this test
+            // failed, try to report that log.
+            const char* APR_LOG = getenv("APR_LOG");
+            if (APR_LOG && *APR_LOG)
+            {
+                std::ifstream inf(APR_LOG);
+                if (! inf.is_open())
+                {
+                    LL_WARNS() << "Couldn't open '" << APR_LOG << "'" << LL_ENDL;
+                }
+                else
+                {
+                    LL_WARNS() << "==============================" << LL_ENDL;
+                    LL_WARNS() << "From '" << APR_LOG << "':" << LL_ENDL;
+                    std::string line;
+                    while (std::getline(inf, line))
+                    {
+                        LL_WARNS() << line << LL_ENDL;
+                    }
+                    LL_WARNS() << "==============================" << LL_ENDL;
+                }
+            }
+            throw;
+        }
     }
 
     /// Run Python script and wait for it to complete.
@@ -214,30 +244,26 @@ static std::string python_out(const std::string& desc, const CONTENT& script)
 class NamedTempDir: public boost::noncopyable
 {
 public:
-    // Use python() function to create a temp directory: I've found
-    // nothing in either Boost.Filesystem or APR quite like Python's
-    // tempfile.mkdtemp().
-    // Special extra bonus: on Mac, mkdtemp() reports a pathname
-    // starting with /var/folders/something, whereas that's really a
-    // symlink to /private/var/folders/something. Have to use
-    // realpath() to compare properly.
     NamedTempDir():
-        mPath(python_out("mkdtemp()",
-                         "from __future__ import with_statement\n"
-                         "import os.path, sys, tempfile\n"
-                         "with open(sys.argv[1], 'w') as f:\n"
-                         "    f.write(os.path.normcase(os.path.normpath(os.path.realpath(tempfile.mkdtemp()))))\n"))
-    {}
+        mPath(NamedTempFile::temp_path()),
+        mCreated(boost::filesystem::create_directories(mPath))
+    {
+        mPath = boost::filesystem::canonical(mPath);
+    }
 
     ~NamedTempDir()
     {
-        aprchk(apr_dir_remove(mPath.c_str(), gAPRPoolp));
+        if (mCreated)
+        {
+            boost::filesystem::remove_all(mPath);
+        }
     }
 
-    std::string getName() const { return mPath; }
+    std::string getName() const { return mPath.string(); }
 
 private:
-    std::string mPath;
+    boost::filesystem::path mPath;
+    bool mCreated;
 };
 
 /*****************************************************************************
@@ -565,7 +591,13 @@ namespace tut
                                  "    f.write(os.path.normcase(os.path.normpath(os.getcwd())))\n");
         // Before running, call setWorkingDirectory()
         py.mParams.cwd = tempdir.getName();
-        ensure_equals("os.getcwd()", py.run_read(), tempdir.getName());
+        std::string expected{ tempdir.getName() };
+#if LL_WINDOWS
+        // SIGH, don't get tripped up by "C:" != "c:" --
+        // but on the Mac, using tolower() fails because "/users" != "/Users"!
+        expected = utf8str_tolower(expected);
+#endif
+        ensure_equals("os.getcwd()", py.run_read(), expected);
     }
 
     template<> template<>
diff --git a/indra/test/namedtempfile.h b/indra/test/namedtempfile.h
index 7d59cad32c..525a35000d 100644
--- a/indra/test/namedtempfile.h
+++ b/indra/test/namedtempfile.h
@@ -13,15 +13,16 @@
 #define LL_NAMEDTEMPFILE_H
 
 #include "llerror.h"
-#include "llapr.h"
-#include "apr_file_io.h"
+#include "llstring.h"
+#include "stringize.h"
 #include <string>
-#include <boost/function.hpp>
-#include <boost/phoenix/core/argument.hpp>
-#include <boost/phoenix/operator/bitwise.hpp>
+#include <boost/filesystem.hpp>
+#include <boost/filesystem/fstream.hpp>
 #include <boost/noncopyable.hpp>
+#include <functional>
 #include <iostream>
 #include <sstream>
+#include <string_view>
 
 /**
  * Create a text file with specified content "somewhere in the
@@ -31,134 +32,106 @@ class NamedTempFile: public boost::noncopyable
 {
     LOG_CLASS(NamedTempFile);
 public:
-    NamedTempFile(const std::string& pfx, const std::string& content, apr_pool_t* pool=gAPRPoolp):
-        mPool(pool)
+    NamedTempFile(const std::string_view& pfx,
+                  const std::string_view& content,
+                  const std::string_view& sfx=std::string_view(""))
     {
-        createFile(pfx, boost::phoenix::placeholders::arg1 << content);
+        createFile(pfx, [&content](std::ostream& out){ out << content; }, sfx);
     }
 
-    // Disambiguate when passing string literal
-    NamedTempFile(const std::string& pfx, const char* content, apr_pool_t* pool=gAPRPoolp):
-        mPool(pool)
+    // Disambiguate when passing string literal -- unclear why a string
+    // literal should be ambiguous wrt std::string_view and Streamer
+    NamedTempFile(const std::string_view& pfx,
+                  const char* content,
+                  const std::string_view& sfx=std::string_view(""))
     {
-        createFile(pfx, boost::phoenix::placeholders::arg1 << content);
+        createFile(pfx, [&content](std::ostream& out){ out << content; }, sfx);
     }
 
     // Function that accepts an ostream ref and (presumably) writes stuff to
     // it, e.g.:
     // (boost::phoenix::placeholders::arg1 << "the value is " << 17 << '\n')
-    typedef boost::function<void(std::ostream&)> Streamer;
+    typedef std::function<void(std::ostream&)> Streamer;
 
-    NamedTempFile(const std::string& pfx, const Streamer& func, apr_pool_t* pool=gAPRPoolp):
-        mPool(pool)
+    NamedTempFile(const std::string_view& pfx,
+                  const Streamer& func,
+                  const std::string_view& sfx=std::string_view(""))
     {
-        createFile(pfx, func);
+        createFile(pfx, func, sfx);
     }
 
     virtual ~NamedTempFile()
     {
-        ll_apr_assert_status(apr_file_remove(mPath.c_str(), mPool));
+        boost::filesystem::remove(mPath);
     }
 
-    virtual std::string getName() const { return mPath; }
+    std::string getName() const { return mPath.string(); }
 
     void peep()
     {
         std::cout << "File '" << mPath << "' contains:\n";
-        std::ifstream reader(mPath.c_str());
+        boost::filesystem::ifstream reader(mPath);
         std::string line;
         while (std::getline(reader, line))
             std::cout << line << '\n';
         std::cout << "---\n";
     }
 
+    static boost::filesystem::path temp_path(const std::string_view& pfx="",
+                                             const std::string_view& sfx="")
+    {
+        // This variable is set by GitHub actions and is the recommended place
+        // to put temp files belonging to an actions job.
+        const char* RUNNER_TEMP = getenv("RUNNER_TEMP");
+        boost::filesystem::path tempdir{
+            // if RUNNER_TEMP is set and not empty
+            (RUNNER_TEMP && *RUNNER_TEMP)?
+            boost::filesystem::path(RUNNER_TEMP) : // use RUNNER_TEMP if available
+            boost::filesystem::temp_directory_path()}; // else canonical temp dir
+        boost::filesystem::path tempname{
+            // use filename template recommended by unique_path() doc, but
+            // with underscores instead of hyphens: some use cases involve
+            // temporary Python scripts
+            tempdir / stringize(pfx, "%%%%_%%%%_%%%%_%%%%", sfx) };
+        return boost::filesystem::unique_path(tempname);
+    }
+
 protected:
-    void createFile(const std::string& pfx, const Streamer& func)
+    void createFile(const std::string_view& pfx,
+                    const Streamer& func,
+                    const std::string_view& sfx)
     {
         // Create file in a temporary place.
-        const char* tempdir = NULL;
-        ll_apr_assert_status(apr_temp_dir_get(&tempdir, mPool));
-
-        // Construct a temp filename template in that directory.
-        char *tempname = NULL;
-        ll_apr_assert_status(apr_filepath_merge(&tempname,
-                                                tempdir,
-                                                (pfx + "XXXXXX").c_str(),
-                                                0,
-                                                mPool));
-
-        // Create a temp file from that template.
-        apr_file_t* fp = NULL;
-        ll_apr_assert_status(apr_file_mktemp(&fp,
-                                             tempname,
-                                             APR_CREATE | APR_WRITE | APR_EXCL,
-                                             mPool));
-        // apr_file_mktemp() alters tempname with the actual name. Not until
-        // now is it valid to capture as our mPath.
-        mPath = tempname;
-
+        mPath = temp_path(pfx, sfx);
+        boost::filesystem::ofstream out{ mPath };
         // Write desired content.
-        std::ostringstream out;
-        // Stream stuff to it.
         func(out);
-
-        std::string data(out.str());
-        apr_size_t writelen(data.length());
-        ll_apr_assert_status(apr_file_write(fp, data.c_str(), &writelen));
-        ll_apr_assert_status(apr_file_close(fp));
-        llassert_always(writelen == data.length());
     }
 
-    std::string mPath;
-    apr_pool_t* mPool;
+    boost::filesystem::path mPath;
 };
 
 /**
  * Create a NamedTempFile with a specified filename extension. This is useful
  * when, for instance, you must be able to use the file in a Python import
  * statement.
- *
- * A NamedExtTempFile actually has two different names. We retain the original
- * no-extension name as a placeholder in the temp directory to ensure
- * uniqueness; to that we link the name plus the desired extension. Naturally,
- * both must be removed on destruction.
  */
 class NamedExtTempFile: public NamedTempFile
 {
     LOG_CLASS(NamedExtTempFile);
 public:
-    NamedExtTempFile(const std::string& ext, const std::string& content, apr_pool_t* pool=gAPRPoolp):
-        NamedTempFile(remove_dot(ext), content, pool),
-        mLink(mPath + ensure_dot(ext))
-    {
-        linkto(mLink);
-    }
+    NamedExtTempFile(const std::string& ext, const std::string_view& content):
+        NamedTempFile(remove_dot(ext), content, ensure_dot(ext))
+    {}
 
     // Disambiguate when passing string literal
-    NamedExtTempFile(const std::string& ext, const char* content, apr_pool_t* pool=gAPRPoolp):
-        NamedTempFile(remove_dot(ext), content, pool),
-        mLink(mPath + ensure_dot(ext))
-    {
-        linkto(mLink);
-    }
+    NamedExtTempFile(const std::string& ext, const char* content):
+        NamedTempFile(remove_dot(ext), content, ensure_dot(ext))
+    {}
 
-    NamedExtTempFile(const std::string& ext, const Streamer& func, apr_pool_t* pool=gAPRPoolp):
-        NamedTempFile(remove_dot(ext), func, pool),
-        mLink(mPath + ensure_dot(ext))
-    {
-        linkto(mLink);
-    }
-
-    virtual ~NamedExtTempFile()
-    {
-        ll_apr_assert_status(apr_file_remove(mLink.c_str(), mPool));
-    }
-
-    // Since the caller has gone to the trouble to create the name with the
-    // extension, that should be the name we return. In this class, mPath is
-    // just a placeholder to ensure that future createFile() calls won't
-    // collide.
-    virtual std::string getName() const { return mLink; }
+    NamedExtTempFile(const std::string& ext, const Streamer& func):
+        NamedTempFile(remove_dot(ext), func, ensure_dot(ext))
+    {}
 
     static std::string ensure_dot(const std::string& ext)
     {
@@ -175,7 +148,7 @@ public:
         {
             return ext;
         }
-        return std::string(".") + ext;
+        return "." + ext;
     }
 
     static std::string remove_dot(const std::string& ext)
@@ -187,19 +160,6 @@ public:
         }
         return ext.substr(found);
     }
-
-private:
-    void linkto(const std::string& path)
-    {
-        // This method assumes that since mPath (without extension) is
-        // guaranteed by apr_file_mktemp() to be unique, then (mPath + any
-        // extension) is also unique. This is likely, though not guaranteed:
-        // files could be created in the same temp directory other than by
-        // this class.
-        ll_apr_assert_status(apr_file_link(mPath.c_str(), path.c_str()));
-    }
-
-    std::string mLink;
 };
 
 #endif /* ! defined(LL_NAMEDTEMPFILE_H) */
diff --git a/indra/test/test.cpp b/indra/test/test.cpp
index bb48216b2b..9dd33c574d 100644
--- a/indra/test/test.cpp
+++ b/indra/test/test.cpp
@@ -100,7 +100,7 @@ public:
 	RecordToTempFile(apr_pool_t* pPool)
 		: LLError::Recorder(),
 		boost::noncopyable(),
-		mTempFile("log", "", pPool),
+		mTempFile("log", ""),
 		mFile(mTempFile.getName().c_str())
 	{
 	}