Closed
Bug 1192013
Opened 9 years ago
Closed 8 years ago
Automate parts of NSS and NSPR release engineering
Categories
(NSS :: Test, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
3.27
People
(Reporter: KaiE, Assigned: KaiE)
Details
Attachments
(4 files, 5 obsolete files)
6.83 KB,
patch
|
mt
:
review+
|
Details | Diff | Splinter Review |
5.03 KB,
patch
|
Details | Diff | Splinter Review | |
11.20 KB,
patch
|
mt
:
review+
|
Details | Diff | Splinter Review |
10.79 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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
Assignee: nobody → kaie
Assignee | ||
Comment 2•9 years ago
|
||
Wan-Teh, if you care, I encourage you to experiment with it.
Assignee | ||
Comment 3•9 years ago
|
||
updated version that automates creating the archive files for publishing.
Attachment #8644576 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
Comment 5•9 years ago
|
||
Can we get this in the tree Kai? Bugzilla isn't exactly the best place for code.
Flags: needinfo?(kaie)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(kaie)
Summary: Automate parts of NSS release engineering → Automate parts of NSS and NSPR release engineering
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8690099 -
Flags: review?(martin.thomson)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8645192 -
Attachment is obsolete: true
Attachment #8649929 -
Attachment is obsolete: true
Attachment #8690100 -
Flags: review?(martin.thomson)
Assignee | ||
Comment 8•9 years ago
|
||
I suggest we check in the initial versions that seem to work. We can enhance them in follow up bugs.
Comment 9•9 years ago
|
||
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.
Attachment #8690100 -
Flags: review?(martin.thomson) → review+
Comment 10•9 years ago
|
||
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?
Attachment #8690099 -
Flags: review?(martin.thomson) → review+
Assignee | ||
Comment 11•8 years ago
|
||
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<<
Assignee | ||
Comment 12•8 years ago
|
||
(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•8 years ago
|
||
Yeah, you have to sniff (check for Darwin) to make this portable.
Assignee | ||
Comment 14•8 years ago
|
||
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.
Assignee | ||
Comment 15•8 years ago
|
||
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.
Attachment #8690100 -
Attachment is obsolete: true
Attachment #8773693 -
Flags: review?(martin.thomson)
Assignee | ||
Comment 16•8 years ago
|
||
nspr interdiff for easier reviewing
Assignee | ||
Comment 17•8 years ago
|
||
(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).
Assignee | ||
Comment 18•8 years ago
|
||
(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.
Assignee | ||
Comment 19•8 years ago
|
||
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.
Assignee | ||
Comment 20•8 years ago
|
||
I've only addressed the blocker comments, sed -i portability, whitespace, usage output completeness,
Attachment #8690099 -
Attachment is obsolete: true
Attachment #8773698 -
Flags: review?(martin.thomson)
Assignee | ||
Comment 21•8 years ago
|
||
nss helper interdiff for review convenience
Updated•8 years ago
|
Attachment #8773693 -
Flags: review?(martin.thomson) → review+
Comment 22•8 years ago
|
||
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.
Attachment #8773698 -
Flags: review?(martin.thomson) → review+
Assignee | ||
Comment 23•8 years ago
|
||
(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.
Assignee | ||
Comment 24•8 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.27
You need to log in
before you can comment on or make changes to this bug.
Description
•