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 491BB385841D for ; Wed, 1 May 2024 12:50:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 491BB385841D 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 491BB385841D 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=1714567861; cv=none; b=KPQT7SoLJwRG6gbyPHI8IpIg0vch0u9VM3rjGGqOqeDBmNpjsumoYZSg9BywhdZnl7eFi4Fsl8sOPoy/ffMjMO4122x63M4eGIR7nBmKtSwxcCJkux5Ti6Gnrlho73KStmxuWMJHBP3lu3alz3Q1YemGPJ05WFo2m3CCeH1grMw= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714567861; c=relaxed/simple; bh=a7LRaZzG9lIAXZeXWVlA3UJ4QkiBwWwe0QNi2jhWlWE=; h=Date:From:To:Subject:Message-ID:MIME-Version; b=cc38FOL2aVxaSUnwv/9QdzvHV4kugc6YWpO9JBYnNPECtCdaWIylYRq6T6d+NxmxufXAHzx871R9MaBnZMXsS58mbZ7uTElA+jpenbOtDydELwW6kLbQSsYDLcf2aK3IC9KWSY4OrsthQGp5ibnU9EKtPYapYTbul2fpQc0Hx0E= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from adam by lucy.dinwoodie.org with local (Exim 4.96) (envelope-from ) id 1s29QI-0008OB-0M for cygwin-apps@cygwin.com; Wed, 01 May 2024 13:50:58 +0100 Date: Wed, 1 May 2024 13:50:58 +0100 From: Adam Dinwoodie To: Christian Franke via Cygwin-apps Subject: Re: [PATCH cygport] Add customization support for announce command Message-ID: <20240501125058.bp5763vyv2zo2u3b@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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <83629cb5-9807-f20c-c8e5-0deff7d8236c@t-online.de> 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 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 "$cmd" "$msg" || error "Command $cmd $msg (cwd=${top}) failed" ) ``` I've removed the array handling for $PV, as it's not an array; possibly you've confused it with $PVP? 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. 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. 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! 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" ) ``` Jon suggested using `env`, although I don't think it provides any function here that isn't already provided by built-in Bash function. Personally, I'd rewrite the whole __pkg_announce_run_cmd_on_msg function as below, although some of this is just my own stylistic preferences (in particular, I try to avoid `eval` if at all possible, and here `local -n` works just fine): ``` __pkg_announce_run_cmd_on_msg() { local cmdvar="$1" local -n cmd="$1" local msg="$2" if [[ ! "$cmd" ]]; then inform "$cmdvar is empty" return 0 fi echo inform "Launching '$cmd $msg'" ( cd "$top" export HOMEPAGE P PF PN PR PV SMTP_SENDER SMTP_SERVER \ SMTP_SERVER_PORT SMTP_ENCRYPTION SMTP_USER \ SMTP_PASS "$cmd" "$msg" || error "Command '$cmd $msg' (cwd=${top}) failed" ) } ```