On Tue, Sep 22, 2026 at 10:12:26PM +0200, Enrico Forestieri wrote:
> > Anyway, I think I lean to clean runCommand from common API, if you willing
> > to do some testing.
>
> Yes, sure.
>
> > My proposal is going in two steps - first can you test whether the
> > following patch works for CAS on windows now with the attached?
>
> Yes, it works, of course.
Ok, thanks. This lands us on the final pair of patches for this set:
- 04d: move the last consumer of runCommand into strict win confinement
- 05: bye bye generic runCommand shell invocation (two more distinct
routes left, I'll come to them later).
Enrico, please look whether those two make sense for you, I could not test.
The things worth looking at in windows build:
- no console flashes at startup (the reason CreateProcess was introduced)
- lyx -dbg info still prints Running: <cygwin root>\bin\mount.exe
--show-cygdrive-prefix
(we did not break that cygdrive probe)
- claude tells me that cygdrive probe failure is not easy to behaviorally
test, so lets leave it the way is is
(Is anyone even using native win lyx + cygwin TeX? We are doing lot of dance
to keep it working...)
- Cygwin itself should hopefully compiles after we kicked popen_read_mode() out.
See the attached,
Pavel
>From 66281cd27b7b9831c9ea0481e4b0924c268b1856 Mon Sep 17 00:00:00 2001
From: Pavel Sanda <[email protected]>
Date: Thu, 17 Sep 2026 11:16:21 +0200
Subject: [PATCH 13/14] Move the cygdrive probe off runCommand
Last runCommand() caller. We can't use argv form as init() runs
before QCoreApplication exists and QProcess is available.
So we move runCommand->runComspec, leave it for _WIN32 only, and drop
parts for other archs. We keep the same windows COMSPEC spawning and
pipeline handling, no change intended.
Deletion of original runCommand comes in the next patch.
The input redirection and the 2>&1 handling not necessary for the
single consumer left, but are kept so that we ship the same code on
Windows without changes.
Don't be deceived, cygdrive query runs in native Windows, not Cygwin.
Tier 01 DiD / argv 04d: cygdrive probe keeps its own spawn code
Assisted-by: Claude Opus 5
---
src/support/os_win32.cpp | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 103 insertions(+), 4 deletions(-)
diff --git a/src/support/os_win32.cpp b/src/support/os_win32.cpp
index a10c17ede0..8ca3051268 100644
--- a/src/support/os_win32.cpp
+++ b/src/support/os_win32.cpp
@@ -32,17 +32,21 @@
#include "support/lassert.h"
+#include <cstdio>
#include <cstdlib>
+#include <cstring> // memset
#include <iostream>
#include <vector>
#include <QString>
#include <io.h>
+#include <fcntl.h> // _O_RDONLY
#include <direct.h> // _getdrive
#include <fileapi.h> // GetFinalPathNameByHandle
#include <shlobj.h> // SHGetFolderPath
#include <windef.h>
+#include <windows.h>
#include <shellapi.h>
#include <shlwapi.h>
@@ -89,6 +93,100 @@ bool windows_style_tex_paths_ = true;
string cygdrive = "/cygdrive";
+
+// Run cmd through COMSPEC and return its standard output in result,
+// the return value tells whether the command exited successfully.
+//
+// Relic of the former cross-platform support::runCommand() and it spawns
+// a shell, so you need be extra careful about safety of the arguments.
+// Preferably do not add new callers of this function - we have generic
+// Systemcall and this is used only in init() before QCoreApplication
+// kicks in.
+bool runComspec(string const & cmd, string & result)
+{
+ result.clear();
+
+ if (verbose)
+ lyxerr << "\nRunning: " << cmd << endl;
+ else
+ LYXERR(Debug::INFO,"Running: " << cmd);
+
+ STARTUPINFO startup;
+ PROCESS_INFORMATION process;
+ SECURITY_ATTRIBUTES security;
+ HANDLE in, out;
+ FILE * inf = 0;
+ bool err2out = false;
+ string command;
+ string const infile = trim(split(cmd, command, '<'), " \"");
+ command = rtrim(command);
+ if (suffixIs(command, "2>&1")) {
+ command = rtrim(command, "2>&1");
+ err2out = true;
+ }
+ string const cmdarg = "/d /c \"" + command + "\"";
+ string const comspec = getEnv("COMSPEC");
+
+ security.nLength = sizeof(SECURITY_ATTRIBUTES);
+ security.bInheritHandle = TRUE;
+ security.lpSecurityDescriptor = NULL;
+
+ if (CreatePipe(&in, &out, &security, 0)) {
+ memset(&startup, 0, sizeof(STARTUPINFO));
+ memset(&process, 0, sizeof(PROCESS_INFORMATION));
+
+ startup.cb = sizeof(STARTUPINFO);
+ startup.dwFlags = STARTF_USESTDHANDLES;
+
+ startup.hStdError = err2out ? out : GetStdHandle(STD_ERROR_HANDLE);
+ startup.hStdInput = infile.empty()
+ ? GetStdHandle(STD_INPUT_HANDLE)
+ : CreateFile(infile.c_str(), GENERIC_READ,
+ FILE_SHARE_READ, &security, OPEN_EXISTING,
+ FILE_ATTRIBUTE_NORMAL, NULL);
+ startup.hStdOutput = out;
+
+ if (startup.hStdInput != INVALID_HANDLE_VALUE &&
+ CreateProcess(comspec.c_str(), (LPTSTR)cmdarg.c_str(),
+ &security, &security, TRUE, CREATE_NO_WINDOW,
+ 0, 0, &startup, &process)) {
+
+ CloseHandle(process.hThread);
+ int fno = _open_osfhandle((intptr_t)in, _O_RDONLY);
+ CloseHandle(out);
+ inf = _fdopen(fno, "r");
+ }
+ }
+
+ if (!inf) {
+ lyxerr << "RunCommand: could not start child process" << endl;
+ return false;
+ }
+
+ int c = fgetc(inf);
+ while (c != EOF) {
+ result += static_cast<char>(c);
+ c = fgetc(inf);
+ }
+
+ WaitForSingleObject(process.hProcess, INFINITE);
+ DWORD pret;
+ BOOL success = GetExitCodeProcess(process.hProcess, &pret);
+ bool valid = (pret == 0) && success;
+ if (!success)
+ pret = -1;
+ if (!infile.empty())
+ CloseHandle(startup.hStdInput);
+ CloseHandle(process.hProcess);
+ if (fclose(inf) != 0)
+ pret = -1;
+
+ if (pret == -1)
+ perror("RunCommand: could not terminate child process");
+
+ return valid;
+}
+
BOOL terminate_handler(DWORD event)
{
if (event == CTRL_CLOSE_EVENT
@@ -221,8 +319,9 @@ void init(int argc, char ** argv[])
RegCloseKey(hkey);
string const mount = string(buf) + "\\bin\\mount.exe";
if (retval == ERROR_SUCCESS && FileName(mount).exists()) {
- cmd_ret const p =
- runCommand(mount + " --show-cygdrive-prefix");
+ string result;
+ bool const valid = runComspec(
+ mount + " --show-cygdrive-prefix", result);
// The output of the mount command is as follows:
// Prefix Type Flags
// /cygdrive system binmode
@@ -230,8 +329,8 @@ void init(int argc, char ** argv[])
// to the outer split which sets cygdrive with its
// contents until the first blank, discarding the
// unneeded return value.
- if (p.valid && prefixIs(p.result, "Prefix"))
- split(split(p.result, '\n'), cygdrive, ' ');
+ if (valid && prefixIs(result, "Prefix"))
+ split(split(result, '\n'), cygdrive, ' ');
}
}
--
2.39.5
>From 2a31a092c1d469a9dccd4c96ac914c40f7dfcf13 Mon Sep 17 00:00:00 2001
From: Pavel Sanda <[email protected]>
Date: Thu, 17 Sep 2026 11:18:49 +0200
Subject: [PATCH 14/14] Remove the popen based runCommand() wrapper
On unix that means we are done with whole class of shell invocations
and its implications.
Two shell calls left on windows via COMSPEC - CAS invocation
and cygdrive - CreateProcess() can't run .bat & QProcess can't
run in init. The commandline has no inputs from prefs or docs,
so it should be safe.
Tier 01 DiD / argv 05: eliminate runCommand shell invocation
Assisted-by: Claude Opus 5
---
configure.ac | 2 +-
development/cmake/configCompiler.h.msvc | 4 -
src/support/filetools.cpp | 117 ------------------------
src/support/filetools.h | 7 --
src/support/os.h | 5 -
src/support/os_cygwin.cpp | 8 --
src/support/os_unix.cpp | 6 --
src/support/os_win32.cpp | 8 --
8 files changed, 1 insertion(+), 156 deletions(-)
diff --git a/configure.ac b/configure.ac
index ae5572fff1..1dae44109a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -182,7 +182,7 @@ AC_TYPE_UID_T
LYX_CHECK_DEF(PATH_MAX, limits.h, [int n = PATH_MAX;])
-AC_CHECK_FUNCS(chmod close _close fork getpid _getpid lockf lstat mkfifo open _open pclose _pclose popen _popen readlink putenv setenv strerror unsetenv)
+AC_CHECK_FUNCS(chmod close _close fork getpid _getpid lockf lstat mkfifo open _open readlink putenv setenv strerror unsetenv)
# Check the form of mkdir()
AC_FUNC_MKDIR
AC_FUNC_SELECT_ARGTYPES
diff --git a/development/cmake/configCompiler.h.msvc b/development/cmake/configCompiler.h.msvc
index 9a9a1a878c..3f71a1c42e 100644
--- a/development/cmake/configCompiler.h.msvc
+++ b/development/cmake/configCompiler.h.msvc
@@ -35,12 +35,8 @@
#define HAVE_OPEN 1
#define HAVE_CHMOD 1
#define HAVE_CLOSE 1
-/* #undef HAVE_POPEN 1 */
-/* #undef HAVE_PCLOSE 1 */
#define HAVE__OPEN 1
#define HAVE__CLOSE 1
-#define HAVE__POPEN 1
-#define HAVE__PCLOSE 1
#define HAVE_GETPID 1
#define HAVE__GETPID 1
#define HAVE_GETTEXT 1 /* TODO this is set manually */
diff --git a/src/support/filetools.cpp b/src/support/filetools.cpp
index d35e4a701b..0fff08f5f8 100644
--- a/src/support/filetools.cpp
+++ b/src/support/filetools.cpp
@@ -63,10 +63,6 @@
#include <QCryptographicHash>
-#if defined (_WIN32)
-#include <io.h>
-#include <windows.h>
-#endif
using namespace std;
@@ -1101,119 +1097,6 @@ bool readLink(FileName const &, FileName &)
#endif
-cmd_ret const runCommand(string const & cmd)
-{
- // FIXME: replace all calls to RunCommand with ForkedCall
- // (if the output is not needed) or the code in ISpell.cpp
- // (if the output is needed).
-
- // One question is if we should use popen or
- // create our own popen based on fork, exec, pipe
- // of course the best would be to have a
- // pstream (process stream), with the
- // variants ipstream, opstream
-
- if (verbose)
- lyxerr << "\nRunning: " << cmd << endl;
- else
- LYXERR(Debug::INFO,"Running: " << cmd);
-
-#if defined (_WIN32)
- STARTUPINFO startup;
- PROCESS_INFORMATION process;
- SECURITY_ATTRIBUTES security;
- HANDLE in, out;
- FILE * inf = 0;
- bool err2out = false;
- string command;
- string const infile = trim(split(cmd, command, '<'), " \"");
- command = rtrim(command);
- if (suffixIs(command, "2>&1")) {
- command = rtrim(command, "2>&1");
- err2out = true;
- }
- string const cmdarg = "/d /c \"" + command + "\"";
- string const comspec = getEnv("COMSPEC");
-
- security.nLength = sizeof(SECURITY_ATTRIBUTES);
- security.bInheritHandle = TRUE;
- security.lpSecurityDescriptor = NULL;
-
- if (CreatePipe(&in, &out, &security, 0)) {
- memset(&startup, 0, sizeof(STARTUPINFO));
- memset(&process, 0, sizeof(PROCESS_INFORMATION));
-
- startup.cb = sizeof(STARTUPINFO);
- startup.dwFlags = STARTF_USESTDHANDLES;
-
- startup.hStdError = err2out ? out : GetStdHandle(STD_ERROR_HANDLE);
- startup.hStdInput = infile.empty()
- ? GetStdHandle(STD_INPUT_HANDLE)
- : CreateFile(infile.c_str(), GENERIC_READ,
- FILE_SHARE_READ, &security, OPEN_EXISTING,
- FILE_ATTRIBUTE_NORMAL, NULL);
- startup.hStdOutput = out;
-
- if (startup.hStdInput != INVALID_HANDLE_VALUE &&
- CreateProcess(comspec.c_str(), (LPTSTR)cmdarg.c_str(),
- &security, &security, TRUE, CREATE_NO_WINDOW,
- 0, 0, &startup, &process)) {
-
- CloseHandle(process.hThread);
- int fno = _open_osfhandle((intptr_t)in, _O_RDONLY);
- CloseHandle(out);
- inf = _fdopen(fno, "r");
- }
- }
-#elif defined (HAVE_POPEN)
- FILE * inf = ::popen(cmd.c_str(), os::popen_read_mode());
-#elif defined (HAVE__POPEN)
- FILE * inf = ::_popen(cmd.c_str(), os::popen_read_mode());
-#else
-#error No popen() function.
-#endif
-
- // (Claus Hentschel) Check if popen was successful ;-)
- if (!inf) {
- lyxerr << "RunCommand: could not start child process" << endl;
- return { false, string() };
- }
-
- string result;
- int c = fgetc(inf);
- while (c != EOF) {
- result += static_cast<char>(c);
- c = fgetc(inf);
- }
-
-#if defined (_WIN32)
- WaitForSingleObject(process.hProcess, INFINITE);
- DWORD pret;
- BOOL success = GetExitCodeProcess(process.hProcess, &pret);
- bool valid = (pret == 0) && success;
- if (!success)
- pret = -1;
- if (!infile.empty())
- CloseHandle(startup.hStdInput);
- CloseHandle(process.hProcess);
- if (fclose(inf) != 0)
- pret = -1;
-#elif defined (HAVE_PCLOSE)
- int const pret = pclose(inf);
- bool const valid = (WEXITSTATUS(pret) == 0);
-#elif defined (HAVE__PCLOSE)
- int const pret = _pclose(inf);
- bool const valid = (WEXITSTATUS(pret) == 0);
-#else
-#error No pclose() function.
-#endif
-
- if (pret == -1)
- perror("RunCommand: could not terminate child process");
-
- return { valid, result };
-}
-
FileName const findtexfile(string const & fil, string const & /*format*/,
bool const onlykpse)
diff --git a/src/support/filetools.h b/src/support/filetools.h
index dc9e5877e7..f65198c2c6 100644
--- a/src/support/filetools.h
+++ b/src/support/filetools.h
@@ -336,13 +336,6 @@ bool prefs2prefs(FileName const & filename, FileName const & tempfile,
/// Does file \p file need to be updated by configure.py?
bool configFileNeedsUpdate(std::string const & file);
-struct cmd_ret {
- bool valid;
- std::string result;
-};
-
-cmd_ret const runCommand(std::string const & cmd);
-
int fileLock(const char * lock_file);
void fileUnlock(int fd, const char * lock_file);
diff --git a/src/support/os.h b/src/support/os.h
index c4c6e10419..8d05837c76 100644
--- a/src/support/os.h
+++ b/src/support/os.h
@@ -122,11 +122,6 @@ std::string latex_path(std::string const & p);
*/
std::string latex_path_list(std::string const & p);
-/** Returns a string suitable to be passed to popen when
- * reading a file.
- */
-char const * popen_read_mode();
-
enum path_type {
PLATFORM,
TEXENGINE
diff --git a/src/support/os_cygwin.cpp b/src/support/os_cygwin.cpp
index 0261e2c70d..19f923075e 100644
--- a/src/support/os_cygwin.cpp
+++ b/src/support/os_cygwin.cpp
@@ -364,14 +364,6 @@ string latex_path_list(string const & p)
}
-// returns a string suitable to be passed to popen when
-// reading a pipe
-char const * popen_read_mode()
-{
- return "r";
-}
-
-
string const & nulldev()
{
static string const nulldev_ = "/dev/null";
diff --git a/src/support/os_unix.cpp b/src/support/os_unix.cpp
index e24a4708e1..2dc577cd10 100644
--- a/src/support/os_unix.cpp
+++ b/src/support/os_unix.cpp
@@ -193,12 +193,6 @@ string latex_path_list(string const & p)
}
-char const * popen_read_mode()
-{
- return "r";
-}
-
-
string const & nulldev()
{
static string const nulldev_ = "/dev/null";
diff --git a/src/support/os_win32.cpp b/src/support/os_win32.cpp
index 8ca3051268..bcaff9e93d 100644
--- a/src/support/os_win32.cpp
+++ b/src/support/os_win32.cpp
@@ -553,14 +553,6 @@ string latex_path_list(string const & p)
}
-// returns a string suitable to be passed to popen when
-// reading a pipe
-char const * popen_read_mode()
-{
- return "r";
-}
-
-
string const & nulldev()
{
static string const nulldev_ = "nul";
--
2.39.5
--
lyx-devel mailing list
[email protected]
https://lists.lyx.org/mailman/listinfo/lyx-devel
