theypsilon / Update_All_MiSTer

All-in-one script for updating your MiSTer
GNU General Public License v3.0
667 stars 28 forks source link

Intent is unclear (to me) #23

Closed frederic-mahe closed 4 years ago

frederic-mahe commented 4 years ago
if ! "${SCRIPT_PATH}" ; then

This way to execute a script and to test its execution is a bit unusual (at least to me). At first, it was unclear to me whether this line tests the variable (set or not), the file (exists or not) or the execution (succeeds or not). To execute the script and print a message if execution fails, I suggest to use a command || echo "fail".

Passing the script to a bash subshell saves a chmod +x. The subshell should inherit the export values, but I didn't tested it.

theypsilon commented 4 years ago

I agree it's a bit unusual. I will probably change it, the only issue with || echo is that I would like to print two line breaks.

What I didn't get is the second paragrapth. Which bash subshell are you referring to?

theypsilon commented 4 years ago

I didn't realize this was a PR, I saw now how you did to have the two line breaks.

frederic-mahe commented 4 years ago

I thought bash script.sh would spawn a subshell and execute the script in that subshell, which could potentially mess up with your exported variables. But It seems I was wrong:

echo "echo $$ $BASHPID" > script.sh 
echo $$ $BASHPID ; bash script.sh 
19441 19441
19441 19441

Everything is executed in the same process (here 19441).

Merging the pull request is up to you. I just wanted to point out that in my opinion bash script.sh makes it clearer that you are actually executing the content of script.sh.

Thanks for your work on MiSTer!

frederic-mahe commented 4 years ago

I made a mistake:

# within a script, inside a subshell, $$ returns the PID of the script, not the subshell.
# $BASHPID: process ID of the current instance of Bash
# (not the same as $$, but often gives the same result).
echo 'echo $$ $BASHPID' > script.sh 
echo $$ $BASHPID ; bash script.sh ; chmod +x script.sh ; ./script.sh 
19441 19441
29274 29274
29275 29275

calling a script spawns a subshell, whatever the calling method. So using bash should not mess up with exported variables.

theypsilon commented 4 years ago

Thanks @frederic-mahe I tested and it seems to be good to merge.