From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lucy.dinwoodie.org (6.a.6.e.d.e.a.9.5.d.e.6.6.e.c.9.d.a.0.2.5.1.e.d.0.b.8.0.1.0.0.2.ip6.arpa [IPv6:2001:8b0:de15:20ad:9ce6:6ed5:9aed:e6a6]) by sourceware.org (Postfix) with ESMTPS id 11C873858D34 for ; Wed, 1 May 2024 21:00:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 11C873858D34 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=dinwoodie.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dinwoodie.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 11C873858D34 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2001:8b0:de15:20ad:9ce6:6ed5:9aed:e6a6 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714597208; cv=none; b=dZweVjyBvVhoIxdGVYZBbbh8+1bU75Jte7zX+JEetr4+Eu5NtmGLtNfC2Gih1zZS2wWb5wrIeIKwE9Wx8ettxduGNwOHOa56bUj/96J1hAkMywRn2IR1lSj0JPHVvwTd/1y3DxNSz5lTuQpeV5NQHyimjvSQ9ynNhLLi+QiGk7s= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714597208; c=relaxed/simple; bh=XLmHTZYXHcJv7mjDNinyti4nBfD2gsXSb+lUUMTm3Xc=; h=Date:From:To:Subject:Message-ID:MIME-Version; b=rVskly69GQVokfRJsELlpzfdf9BPLEBWsXZaT7qC73jgmg/YCjoihA1V2l9GF6v2nsnqMiRi348W7GdZkRofXibF/2o4suSGxhrSk4mfnQTEyAIuH1RrykgOFoCaoW7cXXAX8a2PZJFicGkAkAzH1ODI7j0yEGVDhqJdJxbFQy8= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from adam by lucy.dinwoodie.org with local (Exim 4.96) (envelope-from ) id 1s2H3c-000ArT-2u for cygwin-apps@cygwin.com; Wed, 01 May 2024 22:00:04 +0100 Date: Wed, 1 May 2024 22:00:04 +0100 From: Adam Dinwoodie To: Christian Franke via Cygwin-apps Subject: Re: [PATCH cygport] Add customization support for announce command Message-ID: <20240501210004.cfruzkczzixcwqaq@lucy.dinwoodie.org> References: <2c21353b-f249-d03f-a9fa-68f0e56b9dcb@t-online.de> <20d64930-9c17-4fb0-861e-3145b5d67601@dronecode.org.uk> <83629cb5-9807-f20c-c8e5-0deff7d8236c@t-online.de> <20240501125058.bp5763vyv2zo2u3b@lucy.dinwoodie.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Spam-Status: No, score=0.4 required=5.0 tests=BAYES_00,BODY_8BITS,KAM_DMARC_STATUS,RDNS_DYNAMIC,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Wed, May 01, 2024 at 04:49:10PM +0200, Christian Franke via Cygwin-apps wrote: > 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 Ah, right! Apologies, I'd missed/forgotten that context. I still think this is the wrong approach. Cygport has a very well established design paradigm for running non-trivial custom code: there's a default Bash function in the Cygport library files, and an individual cygport file can override that function with its own version. If running something more than a simple command is required here, I'd have thought following that established paradigm would be a much more sensible approach. This would also completely avoid the need for any special variable handling, because the function will run in an environment where the variables are already set. I'd suggest a pair of functions that _aren't_ read-only, say `announce_edit` and `announce_send`, that default to the current code for editing and sending the announcement, but which a maintainer can override in their cygport file should they so desire. I really don't like passing strings around to be eval'd, particularly when they're expected to contain non-trivial code. Given how difficult Bash quoting can be to get right, I worry that even if your patch is correct now, it'll be a ripe site for bugs when other people try to amend the code. > > ) > > ``` > > > > 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[*]}); Oh. Oh I don't like that. Clearly your code is correct in this context, but that existing code is deeply unintuitive when other arrays are declared explicitly. > 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. Which wouldn't work if -- as I'd thought -- $cmd was only going to be a simple command, since there was no reference to $msg, but it makes sense if $cmd is actually a complete script in its own right, including being able to reference arguments. > > 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. >