You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ds...@apache.org on 2022/07/08 20:47:42 UTC
svn commit: r1902582 - /subversion/trunk/tools/dist/release.py
Author: dsahlberg
Date: Fri Jul 8 20:47:42 2022
New Revision: 1902582
URL: http://svn.apache.org/viewvc?rev=1902582&view=rev
Log:
ASF no longer provide a aggregated KEYS file, so we need to construct it
ourselves using the make-keys.sh script.
* tools/dist/release.py
(roll_tarballs): Call make-keys.sh to create the KEYS file
(get_keys): Call make-keys.sh to create the KEYS file
Modified:
subversion/trunk/tools/dist/release.py
Modified: subversion/trunk/tools/dist/release.py
URL: http://svn.apache.org/viewvc/subversion/trunk/tools/dist/release.py?rev=1902582&r1=1902581&r2=1902582&view=diff
==============================================================================
--- subversion/trunk/tools/dist/release.py (original)
+++ subversion/trunk/tools/dist/release.py Fri Jul 8 20:47:42 2022
@@ -98,7 +98,6 @@ dist_release_url = dist_repos + '/releas
dist_archive_url = 'https://archive.apache.org/dist/subversion'
buildbot_repos = os.getenv('SVN_RELEASE_BUILDBOT_REPOS',
'https://svn.apache.org/repos/infra/infrastructure/buildbot/aegis/buildmaster')
-KEYS = 'https://people.apache.org/keys/group/subversion.asc'
extns = ['zip', 'tar.gz', 'tar.bz2']
@@ -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...
+ 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)
def add_to_changes_dict(changes_dict, audience, section, change, revision):
# Normalize arguments
Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Sahlberg wrote on Fri, Jul 08, 2022 at 23:07:08 +0200:
> Den fre 8 juli 2022 kl 22:47 skrev <ds...@apache.org>:
>
> > Author: dsahlberg
> > Date: Fri Jul 8 20:47:42 2022
> > New Revision: 1902582
> >
> > URL: http://svn.apache.org/viewvc?rev=1902582&view=rev
> > Log:
> > ASF no longer provide a aggregated KEYS file, so we need to construct it
> > ourselves using the make-keys.sh script.
> >
> > * tools/dist/release.py
> > (roll_tarballs): Call make-keys.sh to create the KEYS file
> > (get_keys): Call make-keys.sh to create the KEYS file
> >
> > Modified:
> > subversion/trunk/tools/dist/release.py
> >
> > Modified: subversion/trunk/tools/dist/release.py
> > URL:
> > http://svn.apache.org/viewvc/subversion/trunk/tools/dist/release.py?rev=1902582&r1=1902581&r2=1902582&view=diff
> >
> > ==============================================================================
> > --- subversion/trunk/tools/dist/release.py (original)
> > +++ subversion/trunk/tools/dist/release.py Fri Jul 8 20:47:42 2022
> > @@ -98,7 +98,6 @@ dist_release_url = dist_repos + '/releas
> > dist_archive_url = 'https://archive.apache.org/dist/subversion'
> > buildbot_repos = os.getenv('SVN_RELEASE_BUILDBOT_REPOS',
> > '
> > https://svn.apache.org/repos/infra/infrastructure/buildbot/aegis/buildmaster
> > ')
> > -KEYS = 'https://people.apache.org/keys/group/subversion.asc'
> > extns = ['zip', 'tar.gz', 'tar.bz2']
> >
> >
> > @@ -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...
> > + subprocess.check_call([os.path.dirname(__file__) +
> > '/make-keys.sh',
> > + '-c', os.path.dirname(__file__) + '/../..',
> > + '-o', filepath])
> > shutil.move(filepath, get_target(args))
> >
>
> I have tested the above part but NOT within the full roll_tarballs codepath
> since I'm not sure if I might cause changes in the repository. I believe
> the change is correct and I don't think things will be worse than trying to
> download a non-existing URL but I would appreciate the help from someone
> experienced in the release process to review or at least give me the
> confidence to roll a tarball locally.
IIRC, rolling the tarballs in itself just creates the foo.tar.gz files
locally; it doesn't create the tag or do the post-tagging housekeeping
commits.
To be sure it doesn't commit, you can invalidate or delete any caches of
your svn.apache.org password. Or you could create another local user on
your OS and test from that. The test user should have its own UID,
homedir, and environment, so it doesn't have access to your regular
user's cached usernames/passwords.
Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Sahlberg wrote on Fri, Jul 08, 2022 at 23:07:08 +0200:
> Den fre 8 juli 2022 kl 22:47 skrev <ds...@apache.org>:
>
> > Author: dsahlberg
> > Date: Fri Jul 8 20:47:42 2022
> > New Revision: 1902582
> >
> > URL: http://svn.apache.org/viewvc?rev=1902582&view=rev
> > Log:
> > ASF no longer provide a aggregated KEYS file, so we need to construct it
> > ourselves using the make-keys.sh script.
> >
> > * tools/dist/release.py
> > (roll_tarballs): Call make-keys.sh to create the KEYS file
> > (get_keys): Call make-keys.sh to create the KEYS file
> >
> > Modified:
> > subversion/trunk/tools/dist/release.py
> >
> > Modified: subversion/trunk/tools/dist/release.py
> > URL:
> > http://svn.apache.org/viewvc/subversion/trunk/tools/dist/release.py?rev=1902582&r1=1902581&r2=1902582&view=diff
> >
> > ==============================================================================
> > --- subversion/trunk/tools/dist/release.py (original)
> > +++ subversion/trunk/tools/dist/release.py Fri Jul 8 20:47:42 2022
> > @@ -98,7 +98,6 @@ dist_release_url = dist_repos + '/releas
> > dist_archive_url = 'https://archive.apache.org/dist/subversion'
> > buildbot_repos = os.getenv('SVN_RELEASE_BUILDBOT_REPOS',
> > '
> > https://svn.apache.org/repos/infra/infrastructure/buildbot/aegis/buildmaster
> > ')
> > -KEYS = 'https://people.apache.org/keys/group/subversion.asc'
> > extns = ['zip', 'tar.gz', 'tar.bz2']
> >
> >
> > @@ -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...
> > + subprocess.check_call([os.path.dirname(__file__) +
> > '/make-keys.sh',
> > + '-c', os.path.dirname(__file__) + '/../..',
> > + '-o', filepath])
> > shutil.move(filepath, get_target(args))
> >
>
> I have tested the above part but NOT within the full roll_tarballs codepath
> since I'm not sure if I might cause changes in the repository. I believe
> the change is correct and I don't think things will be worse than trying to
> download a non-existing URL but I would appreciate the help from someone
> experienced in the release process to review or at least give me the
> confidence to roll a tarball locally.
IIRC, rolling the tarballs in itself just creates the foo.tar.gz files
locally; it doesn't create the tag or do the post-tagging housekeeping
commits.
To be sure it doesn't commit, you can invalidate or delete any caches of
your svn.apache.org password. Or you could create another local user on
your OS and test from that. The test user should have its own UID,
homedir, and environment, so it doesn't have access to your regular
user's cached usernames/passwords.
Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py
Posted by Daniel Sahlberg <da...@gmail.com>.
Den fre 8 juli 2022 kl 22:47 skrev <ds...@apache.org>:
> Author: dsahlberg
> Date: Fri Jul 8 20:47:42 2022
> New Revision: 1902582
>
> URL: http://svn.apache.org/viewvc?rev=1902582&view=rev
> Log:
> ASF no longer provide a aggregated KEYS file, so we need to construct it
> ourselves using the make-keys.sh script.
>
> * tools/dist/release.py
> (roll_tarballs): Call make-keys.sh to create the KEYS file
> (get_keys): Call make-keys.sh to create the KEYS file
>
> Modified:
> subversion/trunk/tools/dist/release.py
>
> Modified: subversion/trunk/tools/dist/release.py
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/tools/dist/release.py?rev=1902582&r1=1902581&r2=1902582&view=diff
>
> ==============================================================================
> --- subversion/trunk/tools/dist/release.py (original)
> +++ subversion/trunk/tools/dist/release.py Fri Jul 8 20:47:42 2022
> @@ -98,7 +98,6 @@ dist_release_url = dist_repos + '/releas
> dist_archive_url = 'https://archive.apache.org/dist/subversion'
> buildbot_repos = os.getenv('SVN_RELEASE_BUILDBOT_REPOS',
> '
> https://svn.apache.org/repos/infra/infrastructure/buildbot/aegis/buildmaster
> ')
> -KEYS = 'https://people.apache.org/keys/group/subversion.asc'
> extns = ['zip', 'tar.gz', 'tar.bz2']
>
>
> @@ -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...
> + subprocess.check_call([os.path.dirname(__file__) +
> '/make-keys.sh',
> + '-c', os.path.dirname(__file__) + '/../..',
> + '-o', filepath])
> shutil.move(filepath, get_target(args))
>
I have tested the above part but NOT within the full roll_tarballs codepath
since I'm not sure if I might cause changes in the repository. I believe
the change is correct and I don't think things will be worse than trying to
download a non-existing URL but I would appreciate the help from someone
experienced in the release process to review or at least give me the
confidence to roll a tarball locally.
Kind regards,
Daniel
> # 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)
>
> def add_to_changes_dict(changes_dict, audience, section, change,
> revision):
> # Normalize arguments
>
>
>
Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py
Posted by Nathan Hartman <ha...@gmail.com>.
On Thu, Jul 14, 2022 at 3:55 PM Daniel Sahlberg
<da...@gmail.com> 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 <d....@daniel.shahaf.name>:
>>
>> dsahlberg@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).
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
Posted by Nathan Hartman <ha...@gmail.com>.
On Thu, Jul 14, 2022 at 3:55 PM Daniel Sahlberg
<da...@gmail.com> 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 <d....@daniel.shahaf.name>:
>>
>> dsahlberg@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).
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
Posted by Daniel Sahlberg <da...@gmail.com>.
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>:
> dsahlberg@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
Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py
Posted by Daniel Sahlberg <da...@gmail.com>.
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>:
> dsahlberg@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
Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
dsahlberg@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.
> + 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?
> def add_to_changes_dict(changes_dict, audience, section, change, revision):
> # Normalize arguments
>
>
Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
dsahlberg@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.
> + 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?
> def add_to_changes_dict(changes_dict, audience, section, change, revision):
> # Normalize arguments
>
>
Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py
Posted by Nathan Hartman <ha...@gmail.com>.
On Fri, Jul 8, 2022 at 4:47 PM <ds...@apache.org> wrote:
>
> Author: dsahlberg
> Date: Fri Jul 8 20:47:42 2022
> New Revision: 1902582
>
> URL: http://svn.apache.org/viewvc?rev=1902582&view=rev
> Log:
> ASF no longer provide a aggregated KEYS file, so we need to construct it
> ourselves using the make-keys.sh script.
>
> * tools/dist/release.py
> (roll_tarballs): Call make-keys.sh to create the KEYS file
> (get_keys): Call make-keys.sh to create the KEYS file
>
> Modified:
> subversion/trunk/tools/dist/release.py
>
> Modified: subversion/trunk/tools/dist/release.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/tools/dist/release.py?rev=1902582&r1=1902581&r2=1902582&view=diff
> ==============================================================================
> --- subversion/trunk/tools/dist/release.py (original)
> +++ subversion/trunk/tools/dist/release.py Fri Jul 8 20:47:42 2022
> @@ -98,7 +98,6 @@ dist_release_url = dist_repos + '/releas
> dist_archive_url = 'https://archive.apache.org/dist/subversion'
> buildbot_repos = os.getenv('SVN_RELEASE_BUILDBOT_REPOS',
> 'https://svn.apache.org/repos/infra/infrastructure/buildbot/aegis/buildmaster')
> -KEYS = 'https://people.apache.org/keys/group/subversion.asc'
> extns = ['zip', 'tar.gz', 'tar.bz2']
>
>
> @@ -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...
> + 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)
>
> def add_to_changes_dict(changes_dict, audience, section, change, revision):
> # Normalize arguments
>
>
Okay, so this will generate subversion-<version>.KEYS with the current
keys of all full committers. It won't touch the cumulative KEYS file,
which needs to be maintained manually (for now).
This change seems reasonable conceptually. (I haven't tested it as I'm
also a little concerned about making any inadvertent commits!)
Cheers,
Nathan
Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py
Posted by Daniel Sahlberg <da...@gmail.com>.
Den fre 8 juli 2022 kl 22:47 skrev <ds...@apache.org>:
> Author: dsahlberg
> Date: Fri Jul 8 20:47:42 2022
> New Revision: 1902582
>
> URL: http://svn.apache.org/viewvc?rev=1902582&view=rev
> Log:
> ASF no longer provide a aggregated KEYS file, so we need to construct it
> ourselves using the make-keys.sh script.
>
> * tools/dist/release.py
> (roll_tarballs): Call make-keys.sh to create the KEYS file
> (get_keys): Call make-keys.sh to create the KEYS file
>
> Modified:
> subversion/trunk/tools/dist/release.py
>
> Modified: subversion/trunk/tools/dist/release.py
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/tools/dist/release.py?rev=1902582&r1=1902581&r2=1902582&view=diff
>
> ==============================================================================
> --- subversion/trunk/tools/dist/release.py (original)
> +++ subversion/trunk/tools/dist/release.py Fri Jul 8 20:47:42 2022
> @@ -98,7 +98,6 @@ dist_release_url = dist_repos + '/releas
> dist_archive_url = 'https://archive.apache.org/dist/subversion'
> buildbot_repos = os.getenv('SVN_RELEASE_BUILDBOT_REPOS',
> '
> https://svn.apache.org/repos/infra/infrastructure/buildbot/aegis/buildmaster
> ')
> -KEYS = 'https://people.apache.org/keys/group/subversion.asc'
> extns = ['zip', 'tar.gz', 'tar.bz2']
>
>
> @@ -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...
> + subprocess.check_call([os.path.dirname(__file__) +
> '/make-keys.sh',
> + '-c', os.path.dirname(__file__) + '/../..',
> + '-o', filepath])
> shutil.move(filepath, get_target(args))
>
I have tested the above part but NOT within the full roll_tarballs codepath
since I'm not sure if I might cause changes in the repository. I believe
the change is correct and I don't think things will be worse than trying to
download a non-existing URL but I would appreciate the help from someone
experienced in the release process to review or at least give me the
confidence to roll a tarball locally.
Kind regards,
Daniel
> # 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)
>
> def add_to_changes_dict(changes_dict, audience, section, change,
> revision):
> # Normalize arguments
>
>
>