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 <d...@daniel.shahaf.name>: > dsahlb...@apache.org wrote on Fri, Jul 08, 2022 at 20:47:42 -0000: > > +++ 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
