From 6914ff6113f1667308293fe37e1ba4a4dcba850f Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 5 Jun 2023 21:51:28 -0400 Subject: SL-18837: Use Boost.Filesystem for NamedTempFile, instead of APR. --- indra/test/namedtempfile.h | 136 +++++++++++---------------------------------- indra/test/test.cpp | 10 ++-- 2 files changed, 38 insertions(+), 108 deletions(-) (limited to 'indra/test') diff --git a/indra/test/namedtempfile.h b/indra/test/namedtempfile.h index 7d59cad32c..927e2b4fd9 100644 --- a/indra/test/namedtempfile.h +++ b/indra/test/namedtempfile.h @@ -13,15 +13,15 @@ #define LL_NAMEDTEMPFILE_H #include "llerror.h" -#include "llapr.h" -#include "apr_file_io.h" +#include "stringize.h" #include -#include -#include -#include #include +#include +#include +#include #include #include +#include /** * Create a text file with specified content "somewhere in the @@ -31,41 +31,36 @@ 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); - } - - // Disambiguate when passing string literal - NamedTempFile(const std::string& pfx, const char* content, apr_pool_t* pool=gAPRPoolp): - mPool(pool) - { - 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 Streamer; + typedef std::function 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; } + virtual std::string getName() const { return mPath.native(); } 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'; @@ -73,92 +68,40 @@ public: } 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; + boost::filesystem::path tempname{ boost::filesystem::temp_directory_path() }; + // unique_path() recommended model + tempname += stringize(pfx, "%%%%-%%%%-%%%%-%%%%", sfx); + mPath = boost::filesystem::unique_path(tempname); + 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 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 +118,7 @@ public: { return ext; } - return std::string(".") + ext; + return "." + ext; } static std::string remove_dot(const std::string& ext) @@ -187,19 +130,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..04f32831b7 100644 --- a/indra/test/test.cpp +++ b/indra/test/test.cpp @@ -97,10 +97,10 @@ public: class RecordToTempFile : public LLError::Recorder, public boost::noncopyable { public: - RecordToTempFile(apr_pool_t* pPool) + RecordToTempFile() : LLError::Recorder(), boost::noncopyable(), - mTempFile("log", "", pPool), + mTempFile("log", ""), mFile(mTempFile.getName().c_str()) { } @@ -141,11 +141,11 @@ private: class LLReplayLogReal: public LLReplayLog, public boost::noncopyable { public: - LLReplayLogReal(LLError::ELevel level, apr_pool_t* pool) + LLReplayLogReal(LLError::ELevel level) : LLReplayLog(), boost::noncopyable(), mOldSettings(LLError::saveAndResetSettings()), - mRecorder(new RecordToTempFile(pool)) + mRecorder(new RecordToTempFile()) { LLError::setFatalFunction(wouldHaveCrashed); LLError::setDefaultLevel(level); @@ -624,7 +624,7 @@ int main(int argc, char **argv) if (LOGFAIL && *LOGFAIL) { LLError::ELevel level = LLError::decodeLevel(LOGFAIL); - replayer.reset(new LLReplayLogReal(level, gAPRPoolp)); + replayer.reset(new LLReplayLogReal(level)); } } LLError::setFatalFunction(wouldHaveCrashed); -- cgit v1.3 From aab769e9d7a9b11256572f8adb69d586d9ff8d3d Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 5 Jun 2023 23:24:34 -0400 Subject: SL-18837: Make NamedTempFile revert to boost::function from std::function, since some consumers still use (e.g.) boost::phoenix::placeholders::arg1 to generate an inline callable. --- indra/test/namedtempfile.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'indra/test') diff --git a/indra/test/namedtempfile.h b/indra/test/namedtempfile.h index 927e2b4fd9..df7495fc13 100644 --- a/indra/test/namedtempfile.h +++ b/indra/test/namedtempfile.h @@ -15,10 +15,10 @@ #include "llerror.h" #include "stringize.h" #include -#include +#include #include #include -#include +#include #include #include #include @@ -41,7 +41,7 @@ public: // Function that accepts an ostream ref and (presumably) writes stuff to // it, e.g.: // (boost::phoenix::placeholders::arg1 << "the value is " << 17 << '\n') - typedef std::function Streamer; + typedef boost::function Streamer; NamedTempFile(const std::string_view& pfx, const Streamer& func, -- cgit v1.3 From 26ca3e14d623e4094dde76ad88e3da2a209483b5 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 5 Jun 2023 23:46:43 -0400 Subject: SL-18837: NamedTempFile must still disambiguate string literals. --- indra/test/namedtempfile.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'indra/test') diff --git a/indra/test/namedtempfile.h b/indra/test/namedtempfile.h index df7495fc13..acfc048b7a 100644 --- a/indra/test/namedtempfile.h +++ b/indra/test/namedtempfile.h @@ -38,6 +38,15 @@ public: createFile(pfx, [&content](std::ostream& out){ out << content; }, sfx); } + // 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, [&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') @@ -99,6 +108,11 @@ public: NamedTempFile(remove_dot(ext), content, ensure_dot(ext)) {} + // Disambiguate when passing string literal + NamedExtTempFile(const std::string& ext, const char* content): + NamedTempFile(remove_dot(ext), content, ensure_dot(ext)) + {} + NamedExtTempFile(const std::string& ext, const Streamer& func): NamedTempFile(remove_dot(ext), func, ensure_dot(ext)) {} -- cgit v1.3 From 6516c9d07db42beba5ba9c0c41a33925794a249c Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 6 Jun 2023 07:44:42 -0400 Subject: SL-18837: NamedTempFile back to std::function, use boost::phoenix << It seems the problem addressed by aab769e wasn't some synergy between Boost.Phoenix and Boost.Function, but rather the lack of a Phoenix header file introducing operator<<(). --- indra/llcommon/tests/llleap_test.cpp | 1 + indra/llcommon/tests/llsdserialize_test.cpp | 1 + indra/test/namedtempfile.h | 4 ++-- 3 files changed, 4 insertions(+), 2 deletions(-) (limited to 'indra/test') diff --git a/indra/llcommon/tests/llleap_test.cpp b/indra/llcommon/tests/llleap_test.cpp index 99fd073dd2..0c91db3e24 100644 --- a/indra/llcommon/tests/llleap_test.cpp +++ b/indra/llcommon/tests/llleap_test.cpp @@ -19,6 +19,7 @@ // external library headers #include #include +#include // operator<<() // other Linden headers #include "../test/lltut.h" #include "../test/namedtempfile.h" diff --git a/indra/llcommon/tests/llsdserialize_test.cpp b/indra/llcommon/tests/llsdserialize_test.cpp index d7c11c5021..a0b8519508 100644 --- a/indra/llcommon/tests/llsdserialize_test.cpp +++ b/indra/llcommon/tests/llsdserialize_test.cpp @@ -50,6 +50,7 @@ typedef U32 uint32_t; #include "boost/bind.hpp" #include "boost/phoenix/bind/bind_function.hpp" #include "boost/phoenix/core/argument.hpp" +#include using namespace boost::phoenix; #include "../llsd.h" diff --git a/indra/test/namedtempfile.h b/indra/test/namedtempfile.h index acfc048b7a..84b62a0945 100644 --- a/indra/test/namedtempfile.h +++ b/indra/test/namedtempfile.h @@ -15,10 +15,10 @@ #include "llerror.h" #include "stringize.h" #include -#include #include #include #include +#include #include #include #include @@ -50,7 +50,7 @@ public: // 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 Streamer; + typedef std::function Streamer; NamedTempFile(const std::string_view& pfx, const Streamer& func, -- cgit v1.3 From 1db7ac7139adf505be12308fd7ba2920f5beecde Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 6 Jun 2023 10:02:57 -0400 Subject: SL-18837: Make NamedTempFile name template compatible with Python. The recommended template uses hyphens; change to underscores to be valid Python temp module names. --- indra/test/namedtempfile.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'indra/test') diff --git a/indra/test/namedtempfile.h b/indra/test/namedtempfile.h index 84b62a0945..8cd6c990dc 100644 --- a/indra/test/namedtempfile.h +++ b/indra/test/namedtempfile.h @@ -83,8 +83,9 @@ protected: { // Create file in a temporary place. boost::filesystem::path tempname{ boost::filesystem::temp_directory_path() }; - // unique_path() recommended model - tempname += stringize(pfx, "%%%%-%%%%-%%%%-%%%%", sfx); + // unique_path() recommended template, but with underscores instead of + // hyphens: some use cases involve temporary Python scripts + tempname += stringize(pfx, "%%%%_%%%%_%%%%_%%%%", sfx); mPath = boost::filesystem::unique_path(tempname); boost::filesystem::ofstream out{ mPath }; -- cgit v1.3 From e63c571d1336e3c521e1fc3a5e27bb77fc667790 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 6 Jun 2023 13:34:01 -0400 Subject: SL-18837: On Windows, NamedTempFile must convert from wstring --- indra/test/namedtempfile.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'indra/test') diff --git a/indra/test/namedtempfile.h b/indra/test/namedtempfile.h index 8cd6c990dc..da4fec97c8 100644 --- a/indra/test/namedtempfile.h +++ b/indra/test/namedtempfile.h @@ -13,6 +13,7 @@ #define LL_NAMEDTEMPFILE_H #include "llerror.h" +#include "llstring.h" #include "stringize.h" #include #include @@ -64,7 +65,8 @@ public: boost::filesystem::remove(mPath); } - virtual std::string getName() const { return mPath.native(); } + // On Windows, path::native() returns a wstring + std::string getName() const { return ll_convert(mPath.native()); } void peep() { -- cgit v1.3 From 004150a5305d0df06c52a51a0df3ac26dd4a63cd Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 6 Jun 2023 16:27:43 -0400 Subject: SL-18837: Concat path part with / rather than +=. Using concatenation appends the intended filename to the parent directory name, instead of putting the filename in the parent directory. --- indra/test/namedtempfile.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'indra/test') diff --git a/indra/test/namedtempfile.h b/indra/test/namedtempfile.h index da4fec97c8..c215c50f3f 100644 --- a/indra/test/namedtempfile.h +++ b/indra/test/namedtempfile.h @@ -84,10 +84,12 @@ protected: const std::string_view& sfx) { // Create file in a temporary place. - boost::filesystem::path tempname{ boost::filesystem::temp_directory_path() }; - // unique_path() recommended template, but with underscores instead of - // hyphens: some use cases involve temporary Python scripts - tempname += stringize(pfx, "%%%%_%%%%_%%%%_%%%%", sfx); + boost::filesystem::path tempname{ + boost::filesystem::temp_directory_path() / + // unique_path() recommended template, but with underscores + // instead of hyphens: some use cases involve temporary Python + // scripts + stringize(pfx, "%%%%_%%%%_%%%%_%%%%", sfx) }; mPath = boost::filesystem::unique_path(tempname); boost::filesystem::ofstream out{ mPath }; -- cgit v1.3 From f54c1215676f26480d88b4588bb0eeb9c05f50d9 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 7 Jul 2023 10:06:02 -0400 Subject: SL-18837: Try putting generated Python scripts in RUNNER_TEMP dir. The claim is that the Windows Python interpreter is integrated somehow with the OS such that a command line that tries to run Python with a script that "looks suspicious" (i.e. in a system temp directory) fails with "Access denied" without even loading the interpreter. At least that theory would explain the "Access denied" errors we've been getting trying to run Python scripts generated into the system temp directory by our integration tests. Our hope is that generating such scripts into the GitHub RUNNER_TEMP directory will work better. As this test is specific to Windows, don't even bother running Mac builds. --- .github/workflows/build.yaml | 6 +++--- indra/test/namedtempfile.h | 17 ++++++++++++----- 2 files changed, 15 insertions(+), 8 deletions(-) (limited to 'indra/test') diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 574a83246c..dc8f9f15cd 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -19,9 +19,9 @@ jobs: exclude: - runner: windows-large developer_dir: "/Applications/Xcode_14.0.1.app/Contents/Developer" - ## nat 2023-06-29: until we've resolved the !@#$%! Windows Python - ## permissions problem, don't even bother running Windows builds. - - runner: windows-large + ## nat 2023-07-07: trying to resolve the Windows Python permissions + ## problem; don't bother running Mac builds. + - runner: macos-12-xl runs-on: ${{ matrix.runner }} env: AUTOBUILD_ADDRSIZE: ${{ matrix.addrsize }} diff --git a/indra/test/namedtempfile.h b/indra/test/namedtempfile.h index c215c50f3f..4343361f41 100644 --- a/indra/test/namedtempfile.h +++ b/indra/test/namedtempfile.h @@ -84,12 +84,19 @@ protected: const std::string_view& sfx) { // Create file in a temporary place. + // 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{ - boost::filesystem::temp_directory_path() / - // unique_path() recommended template, but with underscores - // instead of hyphens: some use cases involve temporary Python - // scripts - stringize(pfx, "%%%%_%%%%_%%%%_%%%%", sfx) }; + // 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) }; mPath = boost::filesystem::unique_path(tempname); boost::filesystem::ofstream out{ mPath }; -- cgit v1.3 From e933ace53b24b732d4111169e3c5964a8591a29e Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 7 Jul 2023 14:07:12 -0400 Subject: SL-18837: Try to bypass Windows perm problem with Python indirection. --- indra/llcommon/tests/llprocess_test.cpp | 41 ++++++++++++++++++------------ indra/llcommon/tests/test_python_script.py | 20 +++++++++++++++ indra/test/namedtempfile.h | 22 +++++++++------- 3 files changed, 58 insertions(+), 25 deletions(-) create mode 100644 indra/llcommon/tests/test_python_script.py (limited to 'indra/test') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 4adb8d872a..af99e97d66 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -124,6 +124,17 @@ void waitfor(LLProcess::handle h, const std::string& desc, int timeout=60) i < timeout); } +namespace { + +// find test helper, a sibling of this file +// nat 2023-07-07: we're currently using Boost 1.81, but +// path::replace_filename() (which is exactly what we need here) doesn't +// arrive until Boost 1.82. +auto test_python_script{ + (boost::filesystem::path(__FILE__).remove_filename() / "test_python_script.py").string() }; + +} + /** * Construct an LLProcess to run a Python script. */ @@ -145,6 +156,7 @@ struct PythonProcessLauncher mParams.desc = desc + " script"; mParams.executable = PYTHON; + mParams.args.add(test_python_script); mParams.args.add(mScript.getName()); } @@ -214,30 +226,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; }; /***************************************************************************** @@ -390,6 +398,7 @@ namespace tut // Have to have a named copy of this std::string so its c_str() value // will persist. std::string scriptname(script.getName()); + argv.push_back(test_python_script.c_str()); argv.push_back(scriptname.c_str()); argv.push_back(NULL); diff --git a/indra/llcommon/tests/test_python_script.py b/indra/llcommon/tests/test_python_script.py new file mode 100644 index 0000000000..c0c8661aa9 --- /dev/null +++ b/indra/llcommon/tests/test_python_script.py @@ -0,0 +1,20 @@ +#!/usr/bin/env python3 +"""\ +@file test_python_script.py +@author Nat Goodspeed +@date 2023-07-07 +@brief Work around a problem running Python within integration tests on GitHub + Windows runners. + +$LicenseInfo:firstyear=2023&license=viewerlgpl$ +Copyright (c) 2023, Linden Research, Inc. +$/LicenseInfo$ +""" + +import os +import sys + +# use pop() so that if the referenced script in turn looks at sys.argv, it +# will see its arguments rather than its own filename +_script = sys.argv.pop(1) +exec(open(_script).read()) diff --git a/indra/test/namedtempfile.h b/indra/test/namedtempfile.h index 4343361f41..525a35000d 100644 --- a/indra/test/namedtempfile.h +++ b/indra/test/namedtempfile.h @@ -65,8 +65,7 @@ public: boost::filesystem::remove(mPath); } - // On Windows, path::native() returns a wstring - std::string getName() const { return ll_convert(mPath.native()); } + std::string getName() const { return mPath.string(); } void peep() { @@ -78,12 +77,9 @@ public: std::cout << "---\n"; } -protected: - void createFile(const std::string_view& pfx, - const Streamer& func, - const std::string_view& sfx) + static boost::filesystem::path temp_path(const std::string_view& pfx="", + const std::string_view& sfx="") { - // Create file in a temporary place. // 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"); @@ -97,9 +93,17 @@ protected: // with underscores instead of hyphens: some use cases involve // temporary Python scripts tempdir / stringize(pfx, "%%%%_%%%%_%%%%_%%%%", sfx) }; - mPath = boost::filesystem::unique_path(tempname); - boost::filesystem::ofstream out{ mPath }; + return boost::filesystem::unique_path(tempname); + } +protected: + void createFile(const std::string_view& pfx, + const Streamer& func, + const std::string_view& sfx) + { + // Create file in a temporary place. + mPath = temp_path(pfx, sfx); + boost::filesystem::ofstream out{ mPath }; // Write desired content. func(out); } -- cgit v1.3 From eb8458587537e06df23447db56b9910a0d4e451e Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 8 Sep 2023 09:50:38 -0400 Subject: SL-18837: NamedTempFile must be binary mode on Windows. --- indra/test/namedtempfile.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'indra/test') diff --git a/indra/test/namedtempfile.h b/indra/test/namedtempfile.h index 525a35000d..3a994ae798 100644 --- a/indra/test/namedtempfile.h +++ b/indra/test/namedtempfile.h @@ -70,7 +70,7 @@ public: void peep() { std::cout << "File '" << mPath << "' contains:\n"; - boost::filesystem::ifstream reader(mPath); + boost::filesystem::ifstream reader(mPath, std::ios::binary); std::string line; while (std::getline(reader, line)) std::cout << line << '\n'; @@ -103,7 +103,7 @@ protected: { // Create file in a temporary place. mPath = temp_path(pfx, sfx); - boost::filesystem::ofstream out{ mPath }; + boost::filesystem::ofstream out{ mPath, std::ios::binary }; // Write desired content. func(out); } -- cgit v1.3 From c7546ea65e55143ff3d2d82d8c289bbac7fffe0f Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 8 Sep 2023 14:14:09 -0400 Subject: SL-18837: Make llsdserialize_test debug output conditional. Move hexdump() and hexmix() stream formatters to new hexdump.h for potential use by other tests. In toPythonUsing() helper function, add a temp file to receive Python script debug output, and direct debug output to that file. On test failure, dump the contents of that file to the log. Give NamedTempFile::peep() an optional target std::ostream; refactor implementation as peep_via() that accepts a callable to process each text line. Add operator<<() to stream the contents of a NamedTempFile object to ostream -- but don't use that with LL_DEBUGS(), as it flattens the file contents into a single log line. Instead add peep_log(), which streams each individual text line to LL_DEBUGS(). --- indra/llcommon/tests/llsdserialize_test.cpp | 202 +++++++++++----------------- indra/test/hexdump.h | 97 +++++++++++++ indra/test/namedtempfile.h | 25 +++- 3 files changed, 193 insertions(+), 131 deletions(-) create mode 100644 indra/test/hexdump.h (limited to 'indra/test') diff --git a/indra/llcommon/tests/llsdserialize_test.cpp b/indra/llcommon/tests/llsdserialize_test.cpp index ca63e74c6c..ac40125f75 100644 --- a/indra/llcommon/tests/llsdserialize_test.cpp +++ b/indra/llcommon/tests/llsdserialize_test.cpp @@ -52,12 +52,12 @@ typedef U32 uint32_t; #include "llformat.h" #include "llmemorystream.h" +#include "../test/hexdump.h" #include "../test/lltut.h" #include "../test/namedtempfile.h" #include "stringize.h" -#include +#include "StringVec.h" #include -#include typedef std::function FormatterFunction; typedef std::function ParserFunction; @@ -67,81 +67,6 @@ std::vector string_to_vector(const std::string& str) return std::vector(str.begin(), str.end()); } -// Format a given byte string as 2-digit hex values, no separators -// Usage: std::cout << hexdump(somestring) << ... -class hexdump -{ -public: - hexdump(const char* data, size_t len): - hexdump(reinterpret_cast(data), len) - {} - - hexdump(const U8* data, size_t len): - mData(data, data + len) - {} - - hexdump(const hexdump&) = delete; - hexdump& operator=(const hexdump&) = delete; - - friend std::ostream& operator<<(std::ostream& out, const hexdump& self) - { - auto oldfmt{ out.flags() }; - auto oldfill{ out.fill() }; - out.setf(std::ios_base::hex, std::ios_base::basefield); - out.fill('0'); - for (auto c : self.mData) - { - out << std::setw(2) << unsigned(c); - } - out.setf(oldfmt, std::ios_base::basefield); - out.fill(oldfill); - return out; - } - -private: - std::vector mData; -}; - -// Format a given byte string as a mix of printable characters and, for each -// non-printable character, "\xnn" -// Usage: std::cout << hexmix(somestring) << ... -class hexmix -{ -public: - hexmix(const char* data, size_t len): - mData(data, len) - {} - - hexmix(const hexmix&) = delete; - hexmix& operator=(const hexmix&) = delete; - - friend std::ostream& operator<<(std::ostream& out, const hexmix& self) - { - auto oldfmt{ out.flags() }; - auto oldfill{ out.fill() }; - out.setf(std::ios_base::hex, std::ios_base::basefield); - out.fill('0'); - for (auto c : self.mData) - { - // std::isprint() must be passed an unsigned char! - if (std::isprint(static_cast(c))) - { - out << c; - } - else - { - out << "\\x" << std::setw(2) << unsigned(c); - } - } - out.setf(oldfmt, std::ios_base::basefield); - out.fill(oldfill); - return out; - } - -private: - std::string mData; -}; - namespace tut { struct sd_xml_data @@ -1868,16 +1793,12 @@ namespace tut // helper for TestPythonCompatible static std::string import_llsd("import os.path\n" "import sys\n" - "try:\n" - // new freestanding llsd package - " import llsd\n" - "except ImportError:\n" - // older llbase.llsd module - " from llbase import llsd\n"); + "import llsd\n"); // helper for TestPythonCompatible - template - void python(const std::string& desc, const CONTENT& script, int expect=0) + template + void python_expect(const std::string& desc, const CONTENT& script, int expect=0, + ARGS&&... args) { auto PYTHON(LLStringUtil::getenv("PYTHON")); ensure("Set $PYTHON to the Python interpreter", !PYTHON.empty()); @@ -1888,7 +1809,8 @@ namespace tut std::string q("\""); std::string qPYTHON(q + PYTHON + q); std::string qscript(q + scriptfile.getName() + q); - int rc = _spawnl(_P_WAIT, PYTHON.c_str(), qPYTHON.c_str(), qscript.c_str(), NULL); + int rc = _spawnl(_P_WAIT, PYTHON.c_str(), qPYTHON.c_str(), qscript.c_str(), + std::forward(args)..., NULL); if (rc == -1) { char buffer[256]; @@ -1904,6 +1826,10 @@ namespace tut LLProcess::Params params; params.executable = PYTHON; params.args.add(scriptfile.getName()); + for (const std::string& arg : StringVec{ std::forward(args)... }) + { + params.args.add(arg); + } LLProcessPtr py(LLProcess::create(params)); ensure(STRINGIZE("Couldn't launch " << desc << " script"), bool(py)); // Implementing timeout would mean messing with alarm() and @@ -1938,6 +1864,14 @@ namespace tut #endif } + // helper for TestPythonCompatible + template + void python(const std::string& desc, const CONTENT& script, ARGS&&... args) + { + // plain python() expects rc 0 + python_expect(desc, script, 0, std::forward(args)...); + } + struct TestPythonCompatible { TestPythonCompatible() {} @@ -1952,10 +1886,10 @@ namespace tut void TestPythonCompatibleObject::test<1>() { set_test_name("verify python()"); - python("hello", - "import sys\n" - "sys.exit(17)\n", - 17); // expect nonzero rc + python_expect("hello", + "import sys\n" + "sys.exit(17)\n", + 17); // expect nonzero rc } template<> template<> @@ -1986,14 +1920,14 @@ namespace tut auto buffstr{ buffer.str() }; int bufflen{ static_cast(buffstr.length()) }; out.write(reinterpret_cast(&bufflen), sizeof(bufflen)); - LL_DEBUGS("topy") << "Wrote length: " - << hexdump(reinterpret_cast(&bufflen), - sizeof(bufflen)) - << LL_ENDL; + LL_DEBUGS() << "Wrote length: " + << hexdump(reinterpret_cast(&bufflen), + sizeof(bufflen)) + << LL_ENDL; out.write(buffstr.c_str(), buffstr.length()); - LL_DEBUGS("topy") << "Wrote data: " - << hexmix(buffstr.c_str(), buffstr.length()) - << LL_ENDL; + LL_DEBUGS() << "Wrote data: " + << hexmix(buffstr.c_str(), buffstr.length()) + << LL_ENDL; } } @@ -2033,36 +1967,50 @@ namespace tut (std::ostream& out) { writeLLSDArray(serialize, out, cdata); }); - python("read C++ " + desc, - [&](std::ostream& out){ out << - import_llsd << - "from functools import partial\n" - "import io\n" - "import struct\n" - "lenformat = struct.Struct('i')\n" - "def parse_each(inf):\n" - " for rawlen in iter(partial(inf.read, lenformat.size), b''):\n" - " print('Read length:', ''.join(('%02x' % b) for b in rawlen))\n" - " len = lenformat.unpack(rawlen)[0]\n" - // Since llsd.parse() has no max_bytes argument, instead of - // passing the input stream directly to parse(), read the item - // into a distinct bytes object and parse that. - " data = inf.read(len)\n" - " print('Read data: ', repr(data))\n" - " try:\n" - " frombytes = llsd.parse(data)\n" - " except llsd.LLSDParseError as err:\n" - " print(f'*** {err}')\n" - " print(f'Bad content:\\n{data!r}')\n" - " raise\n" - // Also try parsing from a distinct stream. - " stream = io.BytesIO(data)\n" - " fromstream = llsd.parse(stream)\n" - " assert frombytes == fromstream\n" - " yield frombytes\n" - << pydata << - // Don't forget raw-string syntax for Windows pathnames. - "verify(parse_each(open(r'" << file.getName() << "', 'rb')))\n";}); + // 'debug' starts empty because it's intended as an output file + NamedTempFile debug("debug", ""); + + try + { + python("read C++ " + desc, + [&](std::ostream& out){ out << + import_llsd << + "from functools import partial\n" + "import io\n" + "import struct\n" + "lenformat = struct.Struct('i')\n" + "def parse_each(inf):\n" + " for rawlen in iter(partial(inf.read, lenformat.size), b''):\n" + " print('Read length:', ''.join(('%02x' % b) for b in rawlen),\n" + " file=debug)\n" + " len = lenformat.unpack(rawlen)[0]\n" + // Since llsd.parse() has no max_bytes argument, instead of + // passing the input stream directly to parse(), read the item + // into a distinct bytes object and parse that. + " data = inf.read(len)\n" + " print('Read data: ', repr(data), file=debug)\n" + " try:\n" + " frombytes = llsd.parse(data)\n" + " except llsd.LLSDParseError as err:\n" + " print(f'*** {err}')\n" + " print(f'Bad content:\\n{data!r}')\n" + " raise\n" + // Also try parsing from a distinct stream. + " stream = io.BytesIO(data)\n" + " fromstream = llsd.parse(stream)\n" + " assert frombytes == fromstream\n" + " yield frombytes\n" + << pydata << + // Don't forget raw-string syntax for Windows pathnames. + "debug = open(r'" << debug.getName() << "', 'w')\n" + "verify(parse_each(open(r'" << file.getName() << "', 'rb')))\n";}); + } + catch (const failure&) + { + LL_DEBUGS() << "Script debug output:" << LL_ENDL; + debug.peep_log(); + throw; + } } template<> template<> diff --git a/indra/test/hexdump.h b/indra/test/hexdump.h new file mode 100644 index 0000000000..dd7cbaaa3c --- /dev/null +++ b/indra/test/hexdump.h @@ -0,0 +1,97 @@ +/** + * @file hexdump.h + * @author Nat Goodspeed + * @date 2023-09-08 + * @brief Provide hexdump() and hexmix() ostream formatters + * + * $LicenseInfo:firstyear=2023&license=viewerlgpl$ + * Copyright (c) 2023, Linden Research, Inc. + * $/LicenseInfo$ + */ + +#if ! defined(LL_HEXDUMP_H) +#define LL_HEXDUMP_H + +#include +#include +#include +#include + +// Format a given byte string as 2-digit hex values, no separators +// Usage: std::cout << hexdump(somestring) << ... +class hexdump +{ +public: + hexdump(const std::string_view& data): + hexdump(data.data(), data.length()) + {} + + hexdump(const char* data, size_t len): + hexdump(reinterpret_cast(data), len) + {} + + hexdump(const unsigned char* data, size_t len): + mData(data, data + len) + {} + + friend std::ostream& operator<<(std::ostream& out, const hexdump& self) + { + auto oldfmt{ out.flags() }; + auto oldfill{ out.fill() }; + out.setf(std::ios_base::hex, std::ios_base::basefield); + out.fill('0'); + for (auto c : self.mData) + { + out << std::setw(2) << unsigned(c); + } + out.setf(oldfmt, std::ios_base::basefield); + out.fill(oldfill); + return out; + } + +private: + std::vector mData; +}; + +// Format a given byte string as a mix of printable characters and, for each +// non-printable character, "\xnn" +// Usage: std::cout << hexmix(somestring) << ... +class hexmix +{ +public: + hexmix(const std::string_view& data): + mData(data) + {} + + hexmix(const char* data, size_t len): + mData(data, len) + {} + + friend std::ostream& operator<<(std::ostream& out, const hexmix& self) + { + auto oldfmt{ out.flags() }; + auto oldfill{ out.fill() }; + out.setf(std::ios_base::hex, std::ios_base::basefield); + out.fill('0'); + for (auto c : self.mData) + { + // std::isprint() must be passed an unsigned char! + if (std::isprint(static_cast(c))) + { + out << c; + } + else + { + out << "\\x" << std::setw(2) << unsigned(c); + } + } + out.setf(oldfmt, std::ios_base::basefield); + out.fill(oldfill); + return out; + } + +private: + std::string mData; +}; + +#endif /* ! defined(LL_HEXDUMP_H) */ diff --git a/indra/test/namedtempfile.h b/indra/test/namedtempfile.h index 3a994ae798..ad14cebbd1 100644 --- a/indra/test/namedtempfile.h +++ b/indra/test/namedtempfile.h @@ -67,14 +67,31 @@ public: std::string getName() const { return mPath.string(); } - void peep() + template + void peep_via(CALLABLE&& callable) const { - std::cout << "File '" << mPath << "' contains:\n"; + std::forward(callable)(stringize("File '", mPath, "' contains:")); boost::filesystem::ifstream reader(mPath, std::ios::binary); std::string line; while (std::getline(reader, line)) - std::cout << line << '\n'; - std::cout << "---\n"; + std::forward(callable)(line); + std::forward(callable)("---"); + } + + void peep_log() const + { + peep_via([](const std::string& line){ LL_DEBUGS() << line << LL_ENDL; }); + } + + void peep(std::ostream& out=std::cout) const + { + peep_via([&out](const std::string& line){ out << line << '\n'; }); + } + + friend std::ostream& operator<<(std::ostream& out, const NamedTempFile& self) + { + self.peep(out); + return out; } static boost::filesystem::path temp_path(const std::string_view& pfx="", -- cgit v1.3