1192013 - Automate parts of NSS and NSPR release engineering

Status: RESOLVED FIXED

(NSS :: Test, defect)

Description

[Kai Engert (:KaiE:)]

Manual editing of version number files, and removal and addition of beta status, is cumbersome and needs training to avoid mistakes.
Let's automate it.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: 1192013-v1.patch

This script can be used to automate the changes to the following version number header files:

nssutil_h = "lib/util/nssutil.h"
softkver_h = "lib/softoken/softkver.h"
nss_h = "lib/nss/nss.h"
nssckbi_h = "lib/ckfw/builtins/nssckbi.h"

It can perform the following actions:

- remove beta status
- set beta status

The above is done independently of setting version numbers.

- set to a new "x.y" version number (resetting patch and build to zero)

- set to a new "x.y.z" version number (resetting build to zero)

- set to a new "x.y.z.b" version number

- independently set the release candidate number (4th digit),
  without influencing the printed 2 or 3 digit version number

- print the current state of library version numbers


- print the version number of the root ca module

- set the version number of the root ca module

Comment 2

[Kai Engert (:KaiE:)]

Wan-Teh, if you care, I encourage you to experiment with it.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: nss-release-helper-v2.py

updated version that automates creating the archive files for publishing.

Comment 4

[Kai Engert (:KaiE:)]

Attachment: nspr-release-helper-v1.py

Comment 5

[Martin Thomson [:mt:]]

Can we get this in the tree Kai?  Bugzilla isn't exactly the best place for code.

Comment 6

[Kai Engert (:KaiE:)]

Attachment: nss-rel-helper.patch

Comment 7

[Kai Engert (:KaiE:)]

Attachment: nspr-rel-helper.patch

Comment 8

[Kai Engert (:KaiE:)]

I suggest we check in the initial versions that seem to work.

We can enhance them in follow up bugs.

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8690100 [details] [diff] [review]
nspr-rel-helper.patch

Review of attachment 8690100 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, but I would have written a shell script instead.

The following line is your friend:
trap 'exit 1' ERR

::: nspr-release-helper.py
@@ +23,5 @@
> +def check_call_noisy(cmd, *args, **kwargs):
> +    print "Executing command:", cmd
> +    check_call(cmd, *args, **kwargs)
> +
> +o = OptionParser(usage="client.py [options] remove_beta | set_beta")

This needs to include all the options from below.

@@ +44,5 @@
> +def toggle_beta_status(is_beta):
> +    check_files_exist()
> +    if (is_beta):
> +        print "adding Beta status to version numbers"
> +        check_call_noisy(["sed", "-i", 's/^\(#define *PR_VERSION *\"[0-9.]\+\)\" *$/\\1 Beta\"/', prinit_h])

The MAC version of sed doesn't like the -i option on its own.  I don't know if you want to make this portable.

@@ +50,5 @@
> +
> +    else:
> +        print "removing Beta status from version numbers"
> +        check_call_noisy(["sed", "-i", 's/^\(#define *PR_VERSION *\"[0-9.]\+\) *Beta\" *$/\\1\"/', prinit_h])
> +        check_call_noisy(["sed", "-i", 's/^\(#define *PR_BETA *\)PR_TRUE *$/\\1PR_FALSE/', prinit_h])

I was also thinking that this could be made simpler by having PR_VERSION set based on the value of PR_BETA in the code, so that there would be no change of a mismatch between the two.  e.g.,

#define PR_BETA PR_TRUE
#if (PR_BETA == PR_TRUE)
#define PR_BETA_STRING " Beta"
#else
#define PR_BETA_STRING
#endif
#define PR_VERSION "..... " PR_BETA_STRING

@@ +73,5 @@
> +
> +def print_library_versions():
> +    check_files_exist()
> +    check_call_noisy(["egrep", "#define *PR_VERSION|#define PR_VMAJOR|#define *PR_VMINOR|#define *PR_VPATCH|#define *PR_BETA", prinit_h])
> +    

Drop trailing space.

@@ +83,5 @@
> +def set_major_versions(major):
> +    check_call_noisy(["sed", "-i", 's/^\(#define *PR_VMAJOR *\).*$/\\1' + major + '/', prinit_h])
> +    check_call_noisy(["sed", "-i", 's/^MOD_MAJOR_VERSION=.*$/MOD_MAJOR_VERSION=' + major + '/', f_conf])
> +    check_call_noisy(["sed", "-i", 's/^MOD_MAJOR_VERSION=.*$/MOD_MAJOR_VERSION=' + major + '/', f_conf_in])
> +    

Trailing space.

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8690099 [details] [diff] [review]
nss-rel-helper.patch

Review of attachment 8690099 [details] [diff] [review]:
-----------------------------------------------------------------

After looking at this more thoroughly, I think that this is workable, but it could be significantly more automated.  I think that we should check this in, but look at improving the process so that the script does more work.  As it stands, there are lots of manual pieces.

::: nss-release-helper.py
@@ +49,5 @@
> +        check_call_noisy(["sed", "-i", 's/^\(#define *NSSUTIL_VERSION *\"[0-9.]\+\)\" *$/\\1 Beta\"/', nssutil_h])
> +        check_call_noisy(["sed", "-i", 's/^\(#define *NSSUTIL_BETA *\)PR_FALSE *$/\\1PR_TRUE/', nssutil_h])
> +        check_call_noisy(["sed", "-i", 's/^\(#define *SOFTOKEN_VERSION *\"[0-9.]\+\" *SOFTOKEN_ECC_STRING\) *$/\\1 \" Beta"/', softkver_h])
> +        check_call_noisy(["sed", "-i", 's/^\(#define *SOFTOKEN_BETA *\)PR_FALSE *$/\\1PR_TRUE/', softkver_h])
> +        check_call_noisy(["sed", "-i", 's/^\(#define *NSS_VERSION *\"[0-9.]\+\" *_NSS_ECC_STRING *_NSS_CUSTOMIZED\) *$/\\1 \" Beta"/', nss_h])

I'd really like to get to the point where these headers all reference the same variable(s) for versions.  Is that possible?

@@ +84,5 @@
> +def print_library_versions():
> +    check_files_exist()
> +    check_call_noisy(["egrep", "#define *NSSUTIL_VERSION|#define NSSUTIL_VMAJOR|#define *NSSUTIL_VMINOR|#define *NSSUTIL_VPATCH|#define *NSSUTIL_VBUILD|#define *NSSUTIL_BETA", nssutil_h])
> +    check_call_noisy(["egrep", "#define *SOFTOKEN_VERSION|#define SOFTOKEN_VMAJOR|#define *SOFTOKEN_VMINOR|#define *SOFTOKEN_VPATCH|#define *SOFTOKEN_VBUILD|#define *SOFTOKEN_BETA", softkver_h])
> +    check_call_noisy(["egrep", "#define *NSS_VERSION|#define NSS_VMAJOR|#define *NSS_VMINOR|#define *NSS_VPATCH|#define *NSS_VBUILD|#define *NSS_BETA", nss_h])

I would rather see this parse out the version numbers from each of these into a single string and report that.  Also check that these locations all report the same numbers and scream if there is a mismatch.  That's more work for this script, but easier to use.

My ideal usability outcome is:

$ ./nss-release-helper.py show
NSS_3_22_BETA
$ ./nss-release-helper.py release
Error: can't create a beta release
$ ./nss-release-helper.py set NSS_3_22_RTM
Old: NSS_3_22_BETA
New: NSS_3_22_RTM
$ ./nss-release-helper.py release
../stage/vNSS_3_22_RTM/nss-3.22-with-nspr-4.10.tar.gz

@@ +85,5 @@
> +    check_files_exist()
> +    check_call_noisy(["egrep", "#define *NSSUTIL_VERSION|#define NSSUTIL_VMAJOR|#define *NSSUTIL_VMINOR|#define *NSSUTIL_VPATCH|#define *NSSUTIL_VBUILD|#define *NSSUTIL_BETA", nssutil_h])
> +    check_call_noisy(["egrep", "#define *SOFTOKEN_VERSION|#define SOFTOKEN_VMAJOR|#define *SOFTOKEN_VMINOR|#define *SOFTOKEN_VPATCH|#define *SOFTOKEN_VBUILD|#define *SOFTOKEN_BETA", softkver_h])
> +    check_call_noisy(["egrep", "#define *NSS_VERSION|#define NSS_VMAJOR|#define *NSS_VMINOR|#define *NSS_VPATCH|#define *NSS_VBUILD|#define *NSS_BETA", nss_h])
> +    

ws

@@ +99,5 @@
> +def set_major_versions(major):
> +    check_call_noisy(["sed", "-i", 's/^\(#define *NSSUTIL_VMAJOR *\).*$/\\1' + major + '/', nssutil_h])
> +    check_call_noisy(["sed", "-i", 's/^\(#define *SOFTOKEN_VMAJOR *\).*$/\\1' + major + '/', softkver_h])
> +    check_call_noisy(["sed", "-i", 's/^\(#define *NSS_VMAJOR *\).*$/\\1' + major + '/', nss_h])
> +    

ws

@@ +171,5 @@
> +
> +def create_nss_release_archive():
> +    ensure_arguments_after_action(4, "nss_release_version  nss_hg_release_tag  nspr_release_version  path_to_stage_directory")
> +    nssrel = args[1].strip() #e.g. 3.19.3
> +    nssreltag = args[2].strip() #e.g. NSS_3_19_3_RTM

Can we synthesize this tag (or the version from the tag)?  That would cut down on errors.

@@ +178,5 @@
> +
> +    nspr_tar = "nspr-" + nsprrel + ".tar.gz"
> +    nsprtar_with_path= stagedir + "/v" + nsprrel + "/src/" + nspr_tar
> +    if (not os.path.exists(nsprtar_with_path)):
> +        exit_with_failure("cannot find nspr archive at expected location " + nsprtar_with_path)

Rather than fail, is it possible to create an NSPR release from ../nspr (or whereever that is configured to be)?  We have an nss_build_all target that pulls in NSPR, why not do the same here?  I understand that this might be more work, but it would make this less brittle.

@@ +185,5 @@
> +    if (os.path.exists(nss_stagedir)):
> +        exit_with_failure("nss stage directory already exists: " + nss_stagedir)
> +
> +    nss_tar = "nss-" + nssrel + ".tar.gz"
> +    

ws

@@ +193,5 @@
> +    check_call_noisy(["tar", "-xz", "-C", nss_stagedir, "-f", nsprtar_with_path])
> +    print "changing to directory " + nss_stagedir
> +    os.chdir(nss_stagedir)
> +    check_call_noisy(["tar", "-xz", "-f", nss_tar])
> +    check_call_noisy(["mv", "-i", "nspr-" + nsprrel + "/nspr", "nss-" + nssrel + "/"])

I would use ln or cp rather than mv.

@@ +236,5 @@
> +
> +# use the build/release candiate number in the identifying version number
> +# 4 parameters
> +elif action in ('set_4_digit_release_number'):
> +    set_4_digit_release_number()

Do we really need all of these different options?  Can we just have a set_version that takes a tag string and just extracts the necessary pieces then sets the version?

Comment 11

[Kai Engert (:KaiE:)]

8 months since Martin's review when I didn't have time to address his comments :-(

Because the syntax of NSS_VERSION has been changed in bug 1273505, its regular expression needs to be updated.

The following text must be removed from the code, in two locations:

>> *_NSS_ECC_STRING<<

Comment 12

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt:] from comment #9)
> The MAC version of sed doesn't like the -i option on its own.  I don't know
> if you want to make this portable.

According to http://stackoverflow.com/questions/7573368/in-place-edits-with-sed-on-os-x 
 -i ''
works on OSX.

I'll try to change
  "sed", "-i",
to
  "sed", "-i", "\'\'",

which hopefully will work.

Comment 13

[Martin Thomson [:mt:]]

Yeah, you have to sniff (check for Darwin) to make this portable.

Comment 14

[Kai Engert (:KaiE:)]

Ok, using -i in a portable way is complicated, as explained at http://unix.stackexchange.com/questions/92895/how-to-achieve-portability-with-sed-i-in-place-editing#92907

I'll just use a helper function to call set with a backup extension, and remove the backup file afterwards.

Comment 15

[Kai Engert (:KaiE:)]

Attachment: nspr-rel-helper-v2.patch

Addresses Martin's comments.

And instead of dropping the script into the main directory, I suggest we use a subdirectory. Because nss already has an "automation" subdirectory, I suggest we use "automation/release" for both nss and nspr.

Comment 16

[Kai Engert (:KaiE:)]

Attachment: nspr-interdiff.patch

nspr interdiff for easier reviewing

Comment 17

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt:] from comment #10)
> ::: nss-release-helper.py
> @@ +49,5 @@
> > +        check_call_noisy(["sed", "-i", 's/^\(#define *NSSUTIL_VERSION *\"[0-9.]\+\)\" *$/\\1 Beta\"/', nssutil_h])
> > +        check_call_noisy(["sed", "-i", 's/^\(#define *NSSUTIL_BETA *\)PR_FALSE *$/\\1PR_TRUE/', nssutil_h])
> > +        check_call_noisy(["sed", "-i", 's/^\(#define *SOFTOKEN_VERSION *\"[0-9.]\+\" *SOFTOKEN_ECC_STRING\) *$/\\1 \" Beta"/', softkver_h])
> > +        check_call_noisy(["sed", "-i", 's/^\(#define *SOFTOKEN_BETA *\)PR_FALSE *$/\\1PR_TRUE/', softkver_h])
> > +        check_call_noisy(["sed", "-i", 's/^\(#define *NSS_VERSION *\"[0-9.]\+\" *_NSS_ECC_STRING *_NSS_CUSTOMIZED\) *$/\\1 \" Beta"/', nss_h])
> 
> I'd really like to get to the point where these headers all reference the
> same variable(s) for versions.  Is that possible?

You're suggesting, that we should have just a global variable?

That doesn't work for Red Hat / Fedora, because we mix different versions (older softokn version frozen at FIPS validated snapshot).

Comment 18

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt:] from comment #10)
> @@ +193,5 @@
> > +    check_call_noisy(["tar", "-xz", "-C", nss_stagedir, "-f", nsprtar_with_path])
> > +    print "changing to directory " + nss_stagedir
> > +    os.chdir(nss_stagedir)
> > +    check_call_noisy(["tar", "-xz", "-f", nss_tar])
> > +    check_call_noisy(["mv", "-i", "nspr-" + nsprrel + "/nspr", "nss-" + nssrel + "/"])
> 
> I would use ln or cp rather than mv.

Why unnecessarily have extra files? The file is intended to live in the stage area, nobody else needs it, so I don't need another copy. I don't want to use ln, because I have the habit that the stage directory on my system is stable, and I'm collecting the past archives I have created, so I could doublecheck in the future, if there's ever something fishy on the public archives (compare with the version I had made), so I want my stage directory to have real files, not links.

As your for ideas to make all of this smarter, sure, but I don't have the time currently.

Comment 19

[Kai Engert (:KaiE:)]

BTW, I had used python, not a batch script, because Mozilla already uses python scripts, too, and we never know how much more functionality we want in the future, and python seems more capable.

Comment 20

[Kai Engert (:KaiE:)]

Attachment: nss-rel-helper-v4.patch

I've only addressed the blocker comments, sed -i portability, whitespace, usage output completeness,

Comment 21

[Kai Engert (:KaiE:)]

Attachment: nss-interdiff.patch

nss helper interdiff for review convenience

Comment 22

[Martin Thomson [:mt:]]

Comment on attachment 8773698 [details] [diff] [review]
nss-rel-helper-v4.patch

Review of attachment 8773698 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't run these, but it is better that these be checked in than left floating around.  We can shave any edges off later.

::: automation/release/nss-release-helper.py
@@ +15,5 @@
> +softkver_h = "lib/softoken/softkver.h"
> +nss_h = "lib/nss/nss.h"
> +nssckbi_h = "lib/ckfw/builtins/nssckbi.h"
> +
> +topsrcdir = os.path.dirname(__file__)

This is going to be incorrect now that you have moved these.

Comment 23

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt:] from comment #22)
> 
> > +topsrcdir = os.path.dirname(__file__)
> 
> This is going to be incorrect now that you have moved these.

Thanks. I found that topsrcdir actually isn't being used.

I'll just remove the "topsrcdir" code from both scripts.

Comment 24

[Kai Engert (:KaiE:)]

checked in.

https://hg.mozilla.org/projects/nspr/rev/8d33018c8254
https://hg.mozilla.org/projects/nspr/rev/f133cb26dea0

https://hg.mozilla.org/projects/nss/rev/a10597f76ce3
https://hg.mozilla.org/projects/nss/rev/028a66b0a336