From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout02.t-online.de (mailout02.t-online.de [194.25.134.17]) by sourceware.org (Postfix) with ESMTPS id 204663858D34 for ; Wed, 1 May 2024 14:49:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 204663858D34 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=t-online.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=t-online.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 204663858D34 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=194.25.134.17 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714574961; cv=none; b=gqhJq8I70Ta5tWFBh3RwJT3K0eN05fpYdAPKVyFPyjIgPxaVfgT2/u9a3qYMhIMfPFeVWWSX9ciDc0NYrBczPcuGgqmNxHWO+zN5aay/f1XgIVLLuLyYwcEmT1WqwdaG3gb3i+QvQ8b5nub86qNLLmfRLrV9jDz9aU2XELQhoDg= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714574961; c=relaxed/simple; bh=nwmUcaoncZpPEHT556ZH4frMfPDWt9wcCn/VZXNcs78=; h=Subject:To:From:Message-ID:Date:MIME-Version; b=k8sEE7THZOagbKwI16X4+7dqPvKsPtN5ugb83oZYJwBFy8DsM64S9rvfj8sIF6lmJkODCVikw8vBWUOI9p+vbdyXL9WXh/FBRYgtD/g1cWMycbQlmAlRy8fxDrKuhlnQJPbjgrYb1xPx9qOC0ZOXpqtCRB4UyfC0+jBehaFQ1x4= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from fwd86.aul.t-online.de (fwd86.aul.t-online.de [10.223.144.112]) by mailout02.t-online.de (Postfix) with SMTP id CAA2F238F1 for ; Wed, 1 May 2024 16:49:16 +0200 (CEST) Received: from [192.168.2.101] ([87.187.44.52]) by fwd86.t-online.de with (TLSv1.3:TLS_AES_256_GCM_SHA384 encrypted) esmtp id 1s2BGh-1EP7Wi0; Wed, 1 May 2024 16:49:11 +0200 Subject: Re: [PATCH cygport] Add customization support for announce command To: cygwin-apps@cygwin.com 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> Reply-To: cygwin-apps@cygwin.com From: Christian Franke Message-ID: Date: Wed, 1 May 2024 16:49:10 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 SeaMonkey/2.53.16 MIME-Version: 1.0 In-Reply-To: <20240501125058.bp5763vyv2zo2u3b@lucy.dinwoodie.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-TOI-EXPURGATEID: 150726::1714574951-297F9D65-E6026744/0/0 CLEAN NORMAL X-TOI-MSGID: fa851524-ff56-4b83-92f8-38d9d68fbe4f X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00,BODY_8BITS,FREEMAIL_FROM,KAM_DMARC_STATUS,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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.