Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py
On Thu, Jul 14, 2022 at 3:55 PM Daniel Sahlberg wrote: > > Thanks for the review. I've committed a few changes as r1902722. More below. > > Den ons 13 juli 2022 kl 15:57 skrev Daniel Shahaf : >> >> dsahlb...@apache.org wrote on Fri, Jul 08, 2022 at 20:47:42 -: >> > +++ subversion/trunk/tools/dist/release.py Fri Jul 8 20:47:42 2022 >> > @@ -980,7 +979,12 @@ def roll_tarballs(args): >> > # from a committer's LDAP profile down the road) >> > basename = 'subversion-%s.KEYS' % (str(args.version),) >> > filepath = os.path.join(get_tempdir(args.base_dir), basename) >> > -download_file(KEYS, filepath, None) >> > +# The following code require release.py to be executed within a >> > +# complete wc, not a shallow wc as indicated in HACKING as one >> > option. >> > +# We /could/ download COMMITTERS from /trunk if it doesn't >> > exist... >> >> Well, could you please either change HACKING or download COMMITTERS? >> The code for the latter is basically the tempfile+urlopen mechanics from >> the next hunk of this very diff. > > > I prefer to have less variations in the process to make it easier for new > RMs. I've removed this from HACKING in r1902723 (only on site/staging so far). Thanks for doing that. I had intended to draft some changes for HACKING but it ended up being a busier week than I had anticipated. Nathan
Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py
Thanks for the review. I've committed a few changes as r1902722. More below. Den ons 13 juli 2022 kl 15:57 skrev Daniel Shahaf : > dsahlb...@apache.org wrote on Fri, Jul 08, 2022 at 20:47:42 -: > > +++ subversion/trunk/tools/dist/release.py Fri Jul 8 20:47:42 2022 > > @@ -980,7 +979,12 @@ def roll_tarballs(args): > > # from a committer's LDAP profile down the road) > > basename = 'subversion-%s.KEYS' % (str(args.version),) > > filepath = os.path.join(get_tempdir(args.base_dir), basename) > > -download_file(KEYS, filepath, None) > > +# The following code require release.py to be executed within a > > +# complete wc, not a shallow wc as indicated in HACKING as one > option. > > +# We /could/ download COMMITTERS from /trunk if it doesn't > exist... > > Well, could you please either change HACKING or download COMMITTERS? > The code for the latter is basically the tempfile+urlopen mechanics from > the next hunk of this very diff. > I prefer to have less variations in the process to make it easier for new RMs. I've removed this from HACKING in r1902723 (only on site/staging so far). But while I was at it, I changed make-keys.sh to accept the full filename for the COMMITTERS file, to prepare for someone updating release.py to download the file to a tempfile. > > +subprocess.check_call([os.path.dirname(__file__) + > '/make-keys.sh', > > + '-c', os.path.dirname(__file__) + > '/../..', > > + '-o', filepath]) > > shutil.move(filepath, get_target(args)) > > > > # And we're done! > > @@ -1465,12 +1469,11 @@ def check_sigs(args): > > > > def get_keys(args): > > 'Import the LDAP-based KEYS file to gpg' > > -# We use a tempfile because urlopen() objects don't have a .fileno() > > -with tempfile.SpooledTemporaryFile() as fd: > > -fd.write(urlopen(KEYS).read()) > > -fd.flush() > > -fd.seek(0) > > -subprocess.check_call(['gpg', '--import'], stdin=fd) > > +with tempfile.NamedTemporaryFile(delete=False) as tmpfile: > > + keyspath = tmpfile.name > > +subprocess.check_call([os.path.dirname(__file__) + '/make-keys.sh', > '-c', os.path.dirname(__file__) + '/../..', '-o', keyspath]) > > +subprocess.check_call(['gpg', '--import', keyspath]) > > +os.remove(keyspath) > > That's not how one uses NamedTemporaryFile(). > > Generally, all uses of the file should be inside the «with» block, and > unlinking the file should be left to block's implicit handling > (tmpfile.__exit__()). > > As written, however, NamedTemporaryFile() is used as though it were > a "generate a safe temporary name" API. That means the file is not > created atomically and won't be cleaned up if subprocess.check_call() > raises an exception. > > Could you rewrite so the file isn't used outside its «with» block? > Tried my best. I was afraid of the notice in the docs that a opened a second time, but since we don't support running the RM process on Windows it should be alright. Kind regards, Daniel
svn commit: r1902723 - /subversion/site/staging/docs/community-guide/releasing.part.html
Author: dsahlberg Date: Thu Jul 14 19:51:28 2022 New Revision: 1902723 URL: http://svn.apache.org/viewvc?rev=1902723&view=rev Log: In site/staging: Remove the option to run release.py in a shallow working copy. release.py get-keys (as of r1902722) requires COMMITTERS to be part of the WC. We could update release.py to download COMMITTERS directly from the repository but having less variations to the release process should decrease the chance of an unnoticed snag. * community-guide/releasing.part.html: (#before-release-pristine-tools): As above Modified: subversion/site/staging/docs/community-guide/releasing.part.html Modified: subversion/site/staging/docs/community-guide/releasing.part.html URL: http://svn.apache.org/viewvc/subversion/site/staging/docs/community-guide/releasing.part.html?rev=1902723&r1=1902722&r2=1902723&view=diff == --- subversion/site/staging/docs/community-guide/releasing.part.html (original) +++ subversion/site/staging/docs/community-guide/releasing.part.html Thu Jul 14 19:51:28 2022 @@ -827,8 +827,7 @@ time pass. the release. The details of the rolling process are automated by the https://svn.apache.org/repos/asf/subversion/trunk/tools/dist/release.py";>release.py helper script. To run this script, you'll need a Subversion trunk working -copy (or a shallow trunk working copy containing the tools/dist and -build/generator directories). Run release.py -h to get a +copy. Run release.py -h to get a list of available subcommands. Before you can actually roll the archives, you need to
svn commit: r1902722 - in /subversion/trunk/tools/dist: make-keys.sh release.py
Author: dsahlberg
Date: Thu Jul 14 19:40:57 2022
New Revision: 1902722
URL: http://svn.apache.org/viewvc?rev=1902722&view=rev
Log:
Follow-up to r1902582: Improvements to the release.py script
Suggested by: danielsh
* tools/dist/make-keys.sh:
Make the -c argument expect the NAME of the COMMITTERS file, to make it
easier in the future to point to a file with another (temporary) name.
* tools/dist/release.py
(get_keys): Better use of NamedTemporaryFile. Doesn't work on Windows
(according to Python docs) but there are other release process
requirements mandating the use of *nix so it should be ok.
Modified:
subversion/trunk/tools/dist/make-keys.sh
subversion/trunk/tools/dist/release.py
Modified: subversion/trunk/tools/dist/make-keys.sh
URL:
http://svn.apache.org/viewvc/subversion/trunk/tools/dist/make-keys.sh?rev=1902722&r1=1902721&r2=1902722&view=diff
==
--- subversion/trunk/tools/dist/make-keys.sh (original)
+++ subversion/trunk/tools/dist/make-keys.sh Thu Jul 14 19:40:57 2022
@@ -26,21 +26,21 @@ while getopts ":c:o:h" ARG; do
o) OUTPUT=$OPTARG ;;
h) echo "USAGE: $0 [-c PATH/TO/COMMITTERS] [-o PATH/TO/OUTPUT/KEYS]
[-h]";
echo "";
- echo "-c Set the path to the COMMITTERS file. Default current
directory.";
+ echo "-c Set the path/name to the COMMITTERS file. Default current
directory/COMMITTERS.";
echo "-o Set the path/name of the resulting KEYS file. Default
current directory/KEYS.";
echo "-h Display this message";
return ;;
esac
done
-if [ ! -f $COMMITTERS/COMMITTERS ]; then
+if [ ! -f $COMMITTERS ]; then
echo "COMMITTERS file not found."
exit 1
fi
cd $TMP
-for availid in $( perl -anE 'say $F[0] if (/^Blanket/../END ACTIVE
FULL.*SCRIPTS LOOK FOR IT/ and /@/)' < $COMMITTERS/COMMITTERS )
+for availid in $( perl -anE 'say $F[0] if (/^Blanket/../END ACTIVE
FULL.*SCRIPTS LOOK FOR IT/ and /@/)' < $COMMITTERS )
do
key_url=https://people.apache.org/keys/committer/${availid}.asc
Modified: subversion/trunk/tools/dist/release.py
URL:
http://svn.apache.org/viewvc/subversion/trunk/tools/dist/release.py?rev=1902722&r1=1902721&r2=1902722&view=diff
==
--- subversion/trunk/tools/dist/release.py (original)
+++ subversion/trunk/tools/dist/release.py Thu Jul 14 19:40:57 2022
@@ -1469,11 +1469,10 @@ def check_sigs(args):
def get_keys(args):
'Import the LDAP-based KEYS file to gpg'
-with tempfile.NamedTemporaryFile(delete=False) as tmpfile:
+with tempfile.NamedTemporaryFile() as tmpfile:
keyspath = tmpfile.name
-subprocess.check_call([os.path.dirname(__file__) + '/make-keys.sh', '-c',
os.path.dirname(__file__) + '/../..', '-o', keyspath])
-subprocess.check_call(['gpg', '--import', keyspath])
-os.remove(keyspath)
+ subprocess.check_call([os.path.dirname(__file__) + '/make-keys.sh',
'-c', os.path.dirname(__file__) + '/../../COMMITTERS', '-o', keyspath])
+ subprocess.check_call(['gpg', '--import', keyspath])
def add_to_changes_dict(changes_dict, audience, section, change, revision):
# Normalize arguments
Re: svn commit: r1902590 - /subversion/trunk/tools/client-side/store-plaintext-password.py
On Thu, Jul 14, 2022 at 10:02 AM Daniel Sahlberg wrote: > > Den tors 14 juli 2022 kl 15:52 skrev Daniel Shahaf : >> >> Nathan Hartman wrote on Wed, 13 Jul 2022 15:29 +00:00: >> > On Wed, Jul 13, 2022 at 10:55 AM Daniel Shahaf >> > wrote: >> >> Should the entry link to the zsh script >> >> (https://mail-archives.apache.org/mod_mbox/subversion-dev/202008.mbox/%3C20200816130713.6abca815%40tarpaulin.shahaf.local2%3E) >> >> as well, as an alternative? It might be useful for someone if their >> >> environment doesn't have Python installed or if they find the zsh script >> >> easier to audit. >> > >> > I think it would be useful, and... >> > >> >> (Well, I suppose it might make more sense to copy the script >> >> somewhere than to link to an immutable archives message with that >> >> subject line.) >> > >> > ...the place to put it is probably tools/client-side/ just like the >> > Python script. >> >> Being in tools/ would imply dev@ accepts responsibility for bug reports >> against the zsh script. Is dev@ happy to do that? I'm concerned about >> the bus factor. > > > I was just about to say the same thing (and with no intention to discredit > the zsh version). If it is desirable to list all available realms and let the > user choose interactively, I could add that to the Python script. > > I was also going to add that I think it is better to provide one tool and > make sure that tool is working well instead of having two tools that differ > only in tiny details, since they might bit-rot in different ways over time > and it might be hard for a newcomer to understand the motivation of having > different tools. These are all good points. I admit that zsh is a bit of a mystery to me, as is the script, so I couldn't provide support for it, at least not with my current knowledge. I am impressed that zsh can do so much with so little. It's in the list archives, but as DanielSh points out, is in a thread with a not-so-nice subject. That could be addressed by re-mailing it to dev@ with a new subject, e.g., "Prototype zsh script to store svn password in plaintext" in case anyone ever asks or searches for a non-Python way to do it. We could even link to it from the same FAQ, e.g., "An example of how to store svn plaintext credentials was implemented as a zsh script. It is unsupported by the SVN maintainers but can be found at [link] for pedagogical purposes." Cheers, Nathan
Re: svn commit: r1902590 - /subversion/trunk/tools/client-side/store-plaintext-password.py
Den tors 14 juli 2022 kl 15:52 skrev Daniel Shahaf : > Nathan Hartman wrote on Wed, 13 Jul 2022 15:29 +00:00: > > On Wed, Jul 13, 2022 at 10:55 AM Daniel Shahaf > wrote: > >> Should the entry link to the zsh script > >> ( > https://mail-archives.apache.org/mod_mbox/subversion-dev/202008.mbox/%3C20200816130713.6abca815%40tarpaulin.shahaf.local2%3E > ) > >> as well, as an alternative? It might be useful for someone if their > >> environment doesn't have Python installed or if they find the zsh script > >> easier to audit. > > > > I think it would be useful, and... > > > >> (Well, I suppose it might make more sense to copy the script > >> somewhere than to link to an immutable archives message with that > >> subject line.) > > > > ...the place to put it is probably tools/client-side/ just like the > > Python script. > > Being in tools/ would imply dev@ accepts responsibility for bug reports > against the zsh script. Is dev@ happy to do that? I'm concerned about > the bus factor. > I was just about to say the same thing (and with no intention to discredit the zsh version). If it is desirable to list all available realms and let the user choose interactively, I could add that to the Python script. I was also going to add that I think it is better to provide one tool and make sure that tool is working well instead of having two tools that differ only in tiny details, since they might bit-rot in different ways over time and it might be hard for a newcomer to understand the motivation of having different tools. Kind regards, Daniel
Re: svn commit: r1902590 - /subversion/trunk/tools/client-side/store-plaintext-password.py
Nathan Hartman wrote on Wed, 13 Jul 2022 15:29 +00:00: > On Wed, Jul 13, 2022 at 10:55 AM Daniel Shahaf > wrote: >> Should the entry link to the zsh script >> (https://mail-archives.apache.org/mod_mbox/subversion-dev/202008.mbox/%3C20200816130713.6abca815%40tarpaulin.shahaf.local2%3E) >> as well, as an alternative? It might be useful for someone if their >> environment doesn't have Python installed or if they find the zsh script >> easier to audit. > > I think it would be useful, and... > >> (Well, I suppose it might make more sense to copy the script >> somewhere than to link to an immutable archives message with that >> subject line.) > > ...the place to put it is probably tools/client-side/ just like the > Python script. Being in tools/ would imply dev@ accepts responsibility for bug reports against the zsh script. Is dev@ happy to do that? I'm concerned about the bus factor. Cheers, Daniel
