Skip to content

Commit 6e26b3c

Browse files
author
andy
committed
RCommand: fix csh-specific \$status in execout()/file transfer; default remote shell to bash
Second round of live testing surfaced that the login-sequence fix alone wasn't enough: execout() -- the function every single remote command execution goes through -- appended "; echo CMDSTAT=\$status" to check exit status. \$status is csh/tcsh-specific; bash has no such variable (it's simply unset there), so this silently produced an empty "CMDSTAT=" with no digit, matching none of execout()'s success/failure patterns and failing every remote command with "No status returned" -- confirmed via a live standalone repro (RCommand + execout("echo test") against a real bash connection: failed before this fix, works correctly after). Same pattern existed in the two file-transfer sites (shellput's dd-based upload, shellget's cat-based download). Fixed all three using the now-correctly-tracked p_remoteBash: "\$?" for bash, "\$status" for csh/tcsh/anything else. The downstream CMDSTAT=0/1/2 pattern matching needed no changes -- it only checks the *expanded* numeric result, identical either way. Also flipped RefMachine::shell()'s default from "csh" to "bash", per explicit direction: fewer hard dependencies going forward (neither this build machine nor niobium have tcsh installed, confirmed live), while keeping full csh/tcsh support for any machine that sets "shell: csh" or "shell: tcsh" explicitly in its CONFIG file. Only affects registered remote machines (RefMachine::shell() reads a per-machine CONFIG file) -- RCommand's own constructor default and the local ("system") connection path both still default to "csh" unchanged, since Point 1 (the local shell) remains explicitly out of scope for now. Known remaining gap, not fixed here: execbg() (used by Launch.C for background job launches) also has a csh-specific pattern ("Exit*") for detecting a background command's completion status. Lower urgency than execout() was -- execbg()'s own documented design already treats background-job status as best-effort/not fully reliable even for csh, so a bash mismatch there degrades gracefully (falls through to "assume success" on seeing the prompt) rather than hard-failing the way execout() did. Worth a follow-up, not blocking. Verified end-to-end against a real bash SSH connection (temporary local key-based auth, removed after testing): login, command execution, and the original csh path and local "system" path all confirmed still working with no regressions.
1 parent 1471eb3 commit 6e26b3c

2 files changed

Lines changed: 35 additions & 8 deletions

File tree

‎src/comm/rcommand/RCommand.C‎

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2737,7 +2737,13 @@ bool RCommand::execout(const string& command, string& output,
27372737
bool status = false;
27382738
string cmdstat = command;
27392739

2740-
cmdstat.append("; echo CMDSTAT=$status");
2740+
// $status is csh/tcsh's exit-status variable; bash/sh use $? instead --
2741+
// $status is simply unset in bash, so this would silently always
2742+
// produce an empty "CMDSTAT=" with no digit, matching none of the
2743+
// patterns below and failing every single remote command with "No
2744+
// status returned". Confirmed via a live standalone repro against a
2745+
// real bash remote connection.
2746+
cmdstat.append(p_remoteBash ? "; echo CMDSTAT=$?" : "; echo CMDSTAT=$status");
27412747

27422748
if (timeout != 0)
27432749
exp_timeout = timeout;
@@ -2806,13 +2812,20 @@ bool RCommand::execout(const string& command, string& output,
28062812
// mpp2 so maybe this fix can be conditionalized based on the machine.
28072813
RCommand::expMungedOutputFix();
28082814

2809-
char* line = strstr(exp_buffer, "$status\r\n");
2815+
// Matches whichever status-variable text was actually echoed back as
2816+
// part of the command's own terminal echo (see cmdstat construction
2817+
// above) -- "$status" (7 chars) for csh/tcsh, "$?" (2 chars) for bash.
2818+
const char* statusVarEcho = p_remoteBash ? "$?" : "$status";
2819+
int statusVarLen = p_remoteBash ? 2 : 7;
2820+
string nlPattern = string(statusVarEcho) + "\r\n";
2821+
string crPattern = string(statusVarEcho) + "\r";
2822+
char* line = strstr(exp_buffer, nlPattern.c_str());
28102823

28112824
if (line != NULL)
2812-
output = line+9;
2813-
else if ((line = strstr(exp_buffer, "$status\r")) != NULL)
2825+
output = line + statusVarLen + 2;
2826+
else if ((line = strstr(exp_buffer, crPattern.c_str())) != NULL)
28142827
// this fixes some weird problem that occured on a Dell Linux workstation
2815-
output = line+10;
2828+
output = line + statusVarLen + 3;
28162829
else if (status)
28172830
output = "";
28182831
else
@@ -4094,7 +4107,8 @@ bool RCommand::shellput(const char** fromFiles, const string& toFile)
40944107
sprintf(buf, "%d", nread);
40954108
string nreadstr = buf;
40964109

4097-
cmdstat = "dd ibs=1 of=" + fullToFile + " count=" + nreadstr + "; echo CMDSTAT=$status";
4110+
cmdstat = "dd ibs=1 of=" + fullToFile + " count=" + nreadstr +
4111+
(p_remoteBash ? "; echo CMDSTAT=$?" : "; echo CMDSTAT=$status");
40984112

40994113
if (!expwrite(cmdstat)) return false;
41004114
// scan until start of file to be transferred
@@ -4270,7 +4284,8 @@ bool RCommand::shellget(const char** fromFiles, const string& toFile)
42704284
ofstream os(fullToFile.c_str());
42714285

42724286
if (os) {
4273-
cmd = "cat " + globbedFile + "; echo CMDSTAT=$status";
4287+
cmd = "cat " + globbedFile +
4288+
(p_remoteBash ? "; echo CMDSTAT=$?" : "; echo CMDSTAT=$status");
42744289
if (!expwrite(cmd)) return false;
42754290

42764291
// the commented out logic was failing because I found that

‎src/tdat/resources/RefMachine.C‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,19 @@ string RefMachine::sourceFile(void) const
292292

293293
string RefMachine::shell(void) const
294294
{
295-
string shell = "csh";
295+
// "bash" by default -- csh/tcsh is a real, still-supported option (set
296+
// "shell: csh" or "shell: tcsh" in this machine's CONFIG file for a
297+
// machine/account that genuinely needs it), just no longer assumed.
298+
// Historically this defaulted to "csh" unconditionally, which meant
299+
// ECCE required tcsh to be installed -- on the *local* machine running
300+
// ECCE (RCommand::shellCommand()'s locShell-based -fc/-f/-i local-
301+
// wrapper invocation, not yet updated to be dialect-aware -- see
302+
// CLAUDE.md) and, via this exact field, on every remote machine too --
303+
// even though nothing about ECCE's own job-launch mechanics actually
304+
// needs csh specifically. Fewer hard dependencies, fewer places this
305+
// breaks on a machine that simply doesn't have csh/tcsh installed
306+
// (confirmed live: neither this build machine nor niobium do).
307+
string shell = "bash";
296308

297309
string configName = RefMachine::configFile(refname());
298310

0 commit comments

Comments
 (0)