startami: strip trailing '})' from amicmd to avoid eval syntax error#332
startami: strip trailing '})' from amicmd to avoid eval syntax error#332laurentroque wants to merge 2 commits intopcdshub:masterfrom
Conversation
|
|
||
| amicmd=$(grep ami_client /reg/g/pcds/dist/pds/"$HUTCH"/scripts/"$CONFIG" | grep -v '#' | awk 'BEGIN { FS = ":" }; { print $4}' | sed s/ami_GUI_path//g | sed s/\'+proxy_cds/"$proxy_cds"/g | sed s:\'+expname:"$EXPNAME"/:g | sed s/+\'//g | sed s/\'\}\)//g) | ||
| # Strip trailing cnf/python artifacts (e.g. "})") so eval doesn't choke | ||
| amicmd=$(printf "%s" "$amicmd" | sed 's/[[:space:]]*[})][})]*$//') |
There was a problem hiding this comment.
Some nitpicky questions, if this works I think it's correct to merge without addressing these.
I'm mostly trying to update my own sed/regex knowledge.
- Why is this a new command using
printfinstead of joining onto the| sedtrain on line 74? - (Conversely, if new commands are preferred for readability, maybe we should unwrap more of them)
- Why
printfwith no formatting overecho? - What's
[[:space:]]*doing in the sed here? Are you expecting e.g.})? Your PR description only mentions}) - It looks like you're using a literal
[})]followed by a[})]*, but in regex we have a + operator for matching 1 or more instances of a symbol. This could have been[})]\+
There was a problem hiding this comment.
OK, one sed call on line 74 is probably more efficient. I will change this.
There was a problem hiding this comment.
To Zach's second point, Line 74 is not too daunting once read carefully, but during a mid-experiment quick-fix it was just easier to add another line rather than risk messing up the previous line.
There was a problem hiding this comment.
On why use printf over echo, this was just was found via StackOverflow first, I have no real opinion on it. If I fold this command into the previous sed line then we don't need to think about this too hard.
There was a problem hiding this comment.
On point 4, it looks like the :space: part is not needed. I will remove and re-test.
On point 5, I was unaware of the + operator in regex.
There was a problem hiding this comment.
Pull request overview
Sanitizes the amicmd command assembled by scripts/startami to prevent bash eval syntax errors caused by trailing config-artifact characters on CXI hosts.
Changes:
- Strip trailing
)/}characters from the constructedamicmdstring before executing it.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ZLLentz your comments have now been addressed and incorporated in the latest push to my fork. The Script has been re-tested as done in this Pull Request's "How Has This Been Tested?" section. |
Description
Strip trailing config-artifact characters (e.g.,
})) from theamicmdstring constructed inscripts/startamibefore it is executed.Motivation and Context
startami -c cxi_0.cnfcould fail with:eval: syntax error near unexpected token ')'On CXI hosts,
amicmdwas being parsed from theami_clientline incxi_0.cnfand could end up with a trailing}), causing bashevalto error. This change sanitizes the constructed command to avoid the syntax error.How Has This Been Tested?
cxi-monitor:/cds/home/i/iroque/programs/engineering_tools/scripts/startami -c cxi_0.cnf(reaches expected restart prompt; no eval syntax error)cxi-control:/cds/home/i/iroque/programs/engineering_tools/scripts/startami -c cxi_0.cnf(successfully launchesonline_ami; no eval syntax error)xcs-control:/cds/home/i/iroque/programs/engineering_tools/scripts/startami(successfully launches online_ami; no eval syntax error)Where Has This Been Documented?