Re: [PATCH cygport] Add customization support for announce command

[Christian Franke <Christian.Franke@t-online.de>]

Adam Dinwoodie via Cygwin-apps wrote:
> On Tue, Apr 30, 2024 at 12:27:35PM +0200, Christian Franke via Cygwin-apps wrote:
>> Jon Turney wrote:
>>> On 10/03/2024 16:33, Christian Franke via Cygwin-apps wrote:
>>>> +    /bin/bash -c "cd ${top} || exit 1
>>>> +${HOMEPAGE+HOMEPAGE=${HOMEPAGE@Q}}
>>>> +P=${P@Q}; PF=${PF@Q}; PN=${PN@Q}; PR=${PR@Q}; PV=(${PV[*]@Q})
>>>> +${SMTP_SENDER+SMTP_SENDER=${SMTP_SENDER@Q}}
>>>> +${SMTP_SERVER+SMTP_SERVER=${SMTP_SERVER@Q}}
>>>> +${SMTP_SERVER_PORT+SMTP_SERVER_PORT=${SMTP_SERVER_PORT@Q}}
>>>> +${SMTP_ENCRYPTION+SMTP_ENCRYPTION=${SMTP_ENCRYPTION@Q}}
>>>> +${SMTP_USER+SMTP_USER=${SMTP_USER@Q}}
>>>> +${SMTP_PASS+SMTP_PASS=${SMTP_PASS@Q}}
>>>> +${cmd}
>>>> +"         $0 ${msg} || error "Command '\${${cmdvar}} ${msg}'
>>>> (cwd=${top}) failed"
>>>> +}
>>> Sorry I didn't notice this before, and I am terrible at writing shell,
>>> but perhaps you could share the reasoning behind writing this as above,
>>> and not as, e.g.
>>>
>>> (cd ${top} && env BLAH ${cmd})
>>>
>>> avoiding all the verbiage in the description of ANNOUNCE_EDITOR about it
>>> being fed into 'bash -c' (and hence getting evaluated twice??) rather
>>> than just run?
>>>
>>>
>> None of the mentioned variables are exported to the environment by cygport.
>> I wanted to keep this fact in the subshell. Therefore the assignments are
>> added to the script instead of passing via env(ironment). The latter won't
>> even work with the PV variable because arrays could not be exported.
>>
>> Variables would not be evaluated twice. For example in the rare case that
>> someone uses something like
>>
>> SMTP_SERVER="smtp.$(hostname -d)"
>>
>> in cygport.conf, this would immediately expand to
>> SMTP_SERVER="smtp.some.domain". The above
>>
>> ${SMTP_SERVER+SMTP_SERVER=${SMTP_SERVER@Q}}
>>
>> would expand to
>>
>> SMTP_SERVER=${SMTP_SERVER@Q}
>>
>> and then to
>>
>> SMTP_SERVER='smtp.some.domain'
>>
>> (The @Q bash extension ensures proper quoting).
> Using a subshell created by ( ... ) would achieve the behaviour you're
> after, without requiring nearly so much quote handling.  The code Jon
> has pulled out could be rewritten as below; using ( ... ) would mean
> that everything happens in a subshell and the exports don't affect the
> rest of the environment.
>
> ```
> (
> 	cd "$top"
> 	export HOMEPAGE P PF PN PR PV SMTP_SENDER SMTP_SERVER SMTP_SERVER_PORT SMTP_ENCRYPTION SMTP_USER SMTP_PASS

This unnecessarily exports all variables (except PV, see below) to the 
subcommands run by $cmd.


> 	"$cmd" "$msg" || error "Command $cmd $msg (cwd=${top}) failed"

This would limit $cmd to simple commands instead of multiline scripts. 
This reduces flexibility and some of the examples I provided in my 
original post would no longer work:
https://sourceware.org/pipermail/cygwin-apps/2024-February/043501.html


> )
> ```
>
> I've removed the array handling for $PV, as it's not an array; possibly
> you've confused it with $PVP?
>

No. PV it is initialized as a regular shell variable but is later 
changed to an array by appending the PVP array.

/bin/cygport:
...
declare     PV=${VERSION}
...
             PV=$(echo ${PF} | sed -e "s/${PN}\-\(.*\)\-${PR}$/\1/");
...
declare -r  PV=(${PV} ${PVP[*]});
...

$ git blame bin/cygport.in | grep 'declare -r  PV='
deb528a88 (Yaakov Selkowitz   2009-12-31 08:05:52 +0000 397) declare -r  
PV=(${PV} ${PVP[*]});

Bash silently ignores 'export PV'.

>    In any case, there is no way to pass an
> array to "$cmd" unless "$cmd" is itself a Bash function, as there's no
> standard way to store anything other than strings in environment
> variables.

That's why I use 'PV=(${PV[*]@Q})' as a prefix of the configured $cmd 
string instead of passing any new environment to $cmd.


> I've also removed the `|| return 1` part, since cygport runs with `set
> -e` enabled so you only want to catch non-zero return codes if you want
> specific error handling.

There is no 'return 1' is my patch.


> Finally, I've also added "$msg" to the arguments to "$cmd"; that seems
> to be missing and seems to be critical to this working at all!

$msg is not missing in my patch but passed to the launched /bin/bash as $1.


> Alternatively, if you really wanted to avoid the export statement, the
> below will achieve the same thing; the only use of the subshell at this
> point is to avoid the `cd` changing the working directory for the rest
> of the script.
>
> ```
> (
> 	cd "$top"
> 	HOMEPAGE="$HOMEPAGE" P="$P" PF="$PF" PN="$PN" PR="$PR" PV="$PV" SMTP_SENDER="$SMTP_SENDER" SMTP_SERVER="$SMTP_SERVER" SMTP_SERVER_PORT="$SMTP_SERVER_PORT" SMTP_ENCRYPTION="$SMTP_ENCRYPTION" SMTP_USER="$SMTP_USER" SMTP_PASS="$SMTP_PASS" "$cmd" "$msg" || error "Command $cmd $msg (cwd=${top}) failed"
> )
> ```

Same problem with missing flexibility for $cmd as above.