From f0dbb878337082d3f581874c12e6df2f4659a464 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 20 Jan 2012 18:10:40 -0500 Subject: Per Richard, replace LLProcessLauncher with LLProcess. LLProcessLauncher had the somewhat fuzzy mandate of (1) accumulating parameters with which to launch a child process and (2) sometimes tracking the lifespan of the ensuing child process. But a valid LLProcessLauncher object might or might not have ever been associated with an actual child process. LLProcess specifically tracks a child process. In effect, it's a fairly thin wrapper around a process HANDLE (on Windows) or pid_t (elsewhere), with lifespan management thrown in. A static LLProcess::create() method launches a new child; create() accepts an LLSD bundle with child parameters. So building up a parameter bundle is deferred to LLSD rather than conflated with the process management object. Reconcile all known LLProcessLauncher consumers in the viewer code base, notably the class unit tests. --- indra/newview/llexternaleditor.cpp | 73 +++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 36 deletions(-) (limited to 'indra/newview/llexternaleditor.cpp') diff --git a/indra/newview/llexternaleditor.cpp b/indra/newview/llexternaleditor.cpp index ed1d7e860a..ba58cd8067 100644 --- a/indra/newview/llexternaleditor.cpp +++ b/indra/newview/llexternaleditor.cpp @@ -29,6 +29,9 @@ #include "lltrans.h" #include "llui.h" +#include "llprocess.h" +#include "llsdutil.h" +#include // static const std::string LLExternalEditor::sFilenameMarker = "%s"; @@ -45,19 +48,8 @@ LLExternalEditor::EErrorCode LLExternalEditor::setCommand(const std::string& env return EC_NOT_SPECIFIED; } - // Add the filename marker if missing. - if (cmd.find(sFilenameMarker) == std::string::npos) - { - cmd += " \"" + sFilenameMarker + "\""; - llinfos << "Adding the filename marker (" << sFilenameMarker << ")" << llendl; - } - string_vec_t tokens; - if (tokenize(tokens, cmd) < 2) // 2 = bin + at least one arg (%s) - { - llwarns << "Error parsing editor command" << llendl; - return EC_PARSE_ERROR; - } + tokenize(tokens, cmd); // Check executable for existence. std::string bin_path = tokens[0]; @@ -68,51 +60,60 @@ LLExternalEditor::EErrorCode LLExternalEditor::setCommand(const std::string& env } // Save command. - mProcess.setExecutable(bin_path); - mArgs.clear(); + mProcessParams["executable"] = bin_path; + mProcessParams["args"].clear(); for (size_t i = 1; i < tokens.size(); ++i) { - if (i > 1) mArgs += " "; - mArgs += "\"" + tokens[i] + "\""; + mProcessParams["args"].append(tokens[i]); + } + + // Add the filename marker if missing. + if (cmd.find(sFilenameMarker) == std::string::npos) + { + mProcessParams["args"].append(sFilenameMarker); + llinfos << "Adding the filename marker (" << sFilenameMarker << ")" << llendl; + } + + llinfos << "Setting command [" << bin_path; + BOOST_FOREACH(const std::string& arg, llsd::inArray(mProcessParams["args"])) + { + llcont << " \"" << arg << "\""; } - llinfos << "Setting command [" << bin_path << " " << mArgs << "]" << llendl; + llcont << "]" << llendl; return EC_SUCCESS; } LLExternalEditor::EErrorCode LLExternalEditor::run(const std::string& file_path) { - std::string args = mArgs; - if (mProcess.getExecutable().empty() || args.empty()) + if (mProcessParams["executable"].asString().empty() || ! mProcessParams["args"].size()) { llwarns << "Editor command not set" << llendl; return EC_NOT_SPECIFIED; } - // Substitute the filename marker in the command with the actual passed file name. - LLStringUtil::replaceString(args, sFilenameMarker, file_path); - - // Split command into separate tokens. - string_vec_t tokens; - tokenize(tokens, args); + // Copy params block so we can replace sFilenameMarker + LLSD params(mProcessParams); - // Set process arguments taken from the command. - mProcess.clearArguments(); - for (string_vec_t::const_iterator arg_it = tokens.begin(); arg_it != tokens.end(); ++arg_it) + // Substitute the filename marker in the command with the actual passed file name. + LLSD& args(params["args"]); + for (LLSD::array_iterator ai(args.beginArray()), aend(args.endArray()); ai != aend; ++ai) { - mProcess.addArgument(*arg_it); + std::string sarg(*ai); + LLStringUtil::replaceString(sarg, sFilenameMarker, file_path); + *ai = sarg; } // Run the editor. - llinfos << "Running editor command [" << mProcess.getExecutable() + " " + args << "]" << llendl; - int result = mProcess.launch(); - if (result == 0) + llinfos << "Running editor command [" << params["executable"]; + BOOST_FOREACH(const std::string& arg, llsd::inArray(params["args"])) { - // Prevent killing the process in destructor (will add it to the zombies list). - mProcess.orphan(); + llcont << " \"" << arg << "\""; } - - return result == 0 ? EC_SUCCESS : EC_FAILED_TO_RUN; + llcont << "]" << llendl; + // Prevent killing the process in destructor. + params["autokill"] = false; + return LLProcess::create(params) ? EC_SUCCESS : EC_FAILED_TO_RUN; } // static -- cgit v1.2.3 From 47d94757075e338c480ba4d7d24948242a85a9bb Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sat, 21 Jan 2012 11:45:15 -0500 Subject: Convert LLProcess consumers from LLSD to LLProcess::Params block. Using a Params block gives compile-time checking against attribute typos. One might inadvertently set myLLSD["autofill"] = false and only discover it when things behave strangely at runtime; but trying to set myParams.autofill will produce a compile error. However, it's excellent that the same LLProcess::create() method can accept either LLProcess::Params or a properly-constructed LLSD block. --- indra/newview/llexternaleditor.cpp | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) (limited to 'indra/newview/llexternaleditor.cpp') diff --git a/indra/newview/llexternaleditor.cpp b/indra/newview/llexternaleditor.cpp index ba58cd8067..3dfebad958 100644 --- a/indra/newview/llexternaleditor.cpp +++ b/indra/newview/llexternaleditor.cpp @@ -60,22 +60,22 @@ LLExternalEditor::EErrorCode LLExternalEditor::setCommand(const std::string& env } // Save command. - mProcessParams["executable"] = bin_path; - mProcessParams["args"].clear(); + mProcessParams = LLProcess::Params(); + mProcessParams.executable = bin_path; for (size_t i = 1; i < tokens.size(); ++i) { - mProcessParams["args"].append(tokens[i]); + mProcessParams.args.add(tokens[i]); } // Add the filename marker if missing. if (cmd.find(sFilenameMarker) == std::string::npos) { - mProcessParams["args"].append(sFilenameMarker); + mProcessParams.args.add(sFilenameMarker); llinfos << "Adding the filename marker (" << sFilenameMarker << ")" << llendl; } llinfos << "Setting command [" << bin_path; - BOOST_FOREACH(const std::string& arg, llsd::inArray(mProcessParams["args"])) + BOOST_FOREACH(const std::string& arg, mProcessParams.args) { llcont << " \"" << arg << "\""; } @@ -86,33 +86,36 @@ LLExternalEditor::EErrorCode LLExternalEditor::setCommand(const std::string& env LLExternalEditor::EErrorCode LLExternalEditor::run(const std::string& file_path) { - if (mProcessParams["executable"].asString().empty() || ! mProcessParams["args"].size()) + // LLInitParams type wrappers don't seem to have empty() or size() + // methods; try determining emptiness by comparing begin/end iterators. + if (std::string(mProcessParams.executable).empty() || + (mProcessParams.args.begin() == mProcessParams.args.end())) { llwarns << "Editor command not set" << llendl; return EC_NOT_SPECIFIED; } // Copy params block so we can replace sFilenameMarker - LLSD params(mProcessParams); + LLProcess::Params params; + params.executable = mProcessParams.executable; // Substitute the filename marker in the command with the actual passed file name. - LLSD& args(params["args"]); - for (LLSD::array_iterator ai(args.beginArray()), aend(args.endArray()); ai != aend; ++ai) + BOOST_FOREACH(const std::string& arg, mProcessParams.args) { - std::string sarg(*ai); - LLStringUtil::replaceString(sarg, sFilenameMarker, file_path); - *ai = sarg; + std::string fixed(arg); + LLStringUtil::replaceString(fixed, sFilenameMarker, file_path); + params.args.add(fixed); } // Run the editor. - llinfos << "Running editor command [" << params["executable"]; - BOOST_FOREACH(const std::string& arg, llsd::inArray(params["args"])) + llinfos << "Running editor command [" << std::string(params.executable); + BOOST_FOREACH(const std::string& arg, params.args) { llcont << " \"" << arg << "\""; } llcont << "]" << llendl; // Prevent killing the process in destructor. - params["autokill"] = false; + params.autokill = false; return LLProcess::create(params) ? EC_SUCCESS : EC_FAILED_TO_RUN; } -- cgit v1.2.3 From aa1bbe3277842a9a6e7db5227b35f1fbea50b7a6 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sun, 22 Jan 2012 10:58:16 -0500 Subject: Make LLProcess::Params streamable; use that in LLExternalEditor. --- indra/newview/llexternaleditor.cpp | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) (limited to 'indra/newview/llexternaleditor.cpp') diff --git a/indra/newview/llexternaleditor.cpp b/indra/newview/llexternaleditor.cpp index 3dfebad958..0d3ed0ba35 100644 --- a/indra/newview/llexternaleditor.cpp +++ b/indra/newview/llexternaleditor.cpp @@ -74,12 +74,7 @@ LLExternalEditor::EErrorCode LLExternalEditor::setCommand(const std::string& env llinfos << "Adding the filename marker (" << sFilenameMarker << ")" << llendl; } - llinfos << "Setting command [" << bin_path; - BOOST_FOREACH(const std::string& arg, mProcessParams.args) - { - llcont << " \"" << arg << "\""; - } - llcont << "]" << llendl; + llinfos << "Setting command [" << mProcessParams << "]" << llendl; return EC_SUCCESS; } @@ -108,12 +103,7 @@ LLExternalEditor::EErrorCode LLExternalEditor::run(const std::string& file_path) } // Run the editor. - llinfos << "Running editor command [" << std::string(params.executable); - BOOST_FOREACH(const std::string& arg, params.args) - { - llcont << " \"" << arg << "\""; - } - llcont << "]" << llendl; + llinfos << "Running editor command [" << params << "]" << llendl; // Prevent killing the process in destructor. params.autokill = false; return LLProcess::create(params) ? EC_SUCCESS : EC_FAILED_TO_RUN; -- cgit v1.2.3 From 748d1b311fdecf123df40bd7d22dd7e19afaca84 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sun, 22 Jan 2012 11:56:38 -0500 Subject: Add LLProcess logging on launch(), kill(), isRunning(). Much as I dislike viewer log spam, seems to me starting a child process, killing it and observing its termination are noteworthy events. New logging makes LLExternalEditor launch message redundant; removed. --- indra/newview/llexternaleditor.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'indra/newview/llexternaleditor.cpp') diff --git a/indra/newview/llexternaleditor.cpp b/indra/newview/llexternaleditor.cpp index 0d3ed0ba35..561b87618c 100644 --- a/indra/newview/llexternaleditor.cpp +++ b/indra/newview/llexternaleditor.cpp @@ -102,9 +102,7 @@ LLExternalEditor::EErrorCode LLExternalEditor::run(const std::string& file_path) params.args.add(fixed); } - // Run the editor. - llinfos << "Running editor command [" << params << "]" << llendl; - // Prevent killing the process in destructor. + // Run the editor. Prevent killing the process in destructor. params.autokill = false; return LLProcess::create(params) ? EC_SUCCESS : EC_FAILED_TO_RUN; } -- cgit v1.2.3 From 199e875210435cbc914e80bf3eb6be6c985fce1c Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 23 Jan 2012 17:04:18 -0500 Subject: Use LLProcess::Params::args::empty() instead of comparing iterators. --- indra/newview/llexternaleditor.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'indra/newview/llexternaleditor.cpp') diff --git a/indra/newview/llexternaleditor.cpp b/indra/newview/llexternaleditor.cpp index 561b87618c..db482f023e 100644 --- a/indra/newview/llexternaleditor.cpp +++ b/indra/newview/llexternaleditor.cpp @@ -81,10 +81,7 @@ LLExternalEditor::EErrorCode LLExternalEditor::setCommand(const std::string& env LLExternalEditor::EErrorCode LLExternalEditor::run(const std::string& file_path) { - // LLInitParams type wrappers don't seem to have empty() or size() - // methods; try determining emptiness by comparing begin/end iterators. - if (std::string(mProcessParams.executable).empty() || - (mProcessParams.args.begin() == mProcessParams.args.end())) + if (std::string(mProcessParams.executable).empty() || mProcessParams.args.empty()) { llwarns << "Editor command not set" << llendl; return EC_NOT_SPECIFIED; -- cgit v1.2.3 From 4a7848148e886676dd24bfcf4f50db06bffb28da Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sun, 26 Feb 2012 07:21:51 -0500 Subject: Add TODOs for getTokens() to known places that scan command lines. Lacking time to properly test new LLStringUtil::getTokens() against the present (different!) command-line scanners in LLExternalEditor::tokenize() and LLCommandLineParser::parseCommandLineString(), just annotate as future work the goal of unifying them... SIGH. --- indra/newview/llexternaleditor.cpp | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'indra/newview/llexternaleditor.cpp') diff --git a/indra/newview/llexternaleditor.cpp b/indra/newview/llexternaleditor.cpp index db482f023e..9480e54809 100644 --- a/indra/newview/llexternaleditor.cpp +++ b/indra/newview/llexternaleditor.cpp @@ -119,6 +119,12 @@ std::string LLExternalEditor::getErrorMessage(EErrorCode code) return LLTrans::getString("Unknown"); } +// TODO: +// - Unit-test this with tests like LLStringUtil::getTokens() (the +// command-line overload that supports quoted tokens) +// - Unless there are significant semantic differences, eliminate this method +// and use LLStringUtil::getTokens() instead. + // static size_t LLExternalEditor::tokenize(string_vec_t& tokens, const std::string& str) { -- cgit v1.2.3 From bf6182daa8b4d7cea79310547f71d7a3155e17b0 Mon Sep 17 00:00:00 2001 From: Graham Madarasz Date: Fri, 29 Mar 2013 07:50:08 -0700 Subject: Update Mac and Windows breakpad builds to latest --- indra/newview/llexternaleditor.cpp | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 indra/newview/llexternaleditor.cpp (limited to 'indra/newview/llexternaleditor.cpp') diff --git a/indra/newview/llexternaleditor.cpp b/indra/newview/llexternaleditor.cpp old mode 100644 new mode 100755 -- cgit v1.2.3