Closed
Bug 415966
Opened 16 years ago
Closed 16 years ago
sign-release.pl should support already-signed en-US build
Categories
(Release Engineering :: General, defect, P3)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: rhelmer, Assigned: rhelmer)
Details
(Whiteboard: looking at test results)
Attachments
(2 files, 4 obsolete files)
4.91 KB,
patch
|
nthomas
:
review+
|
Details | Diff | Splinter Review |
4.93 KB,
patch
|
Details | Diff | Splinter Review |
Once we have done an en-US build for a release and repacked the l10n builds, we run sign-release.pl which: * unpacks and signs en-US DLL/EXE files and caches the result * unpacks each l10n build and copies the internals (if applicable) This ensures that all builds for a release have the same binaries where possible (A/V vendors need this for example). However, it's not currently possible to come back and later add a new locale, the script will want to re-sign en-US (authenticode signatures are different depending on the time they are applied). Therefore, sign-release.pl should support a mode that lets it know that en-US is already signed. The signed and unsigned versions would both need to be provided, so the "before" file checksums can be compared to the unsigned l10n builds.
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → rhelmer
Priority: -- → P1
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Flags: blocking1.9?
Assignee | ||
Comment 1•16 years ago
|
||
I'm going to try to get this going tomorrow, I'll be out next week though so hopefully Nick will have time to take a look if I don't complete it in time :)
Assignee | ||
Comment 2•16 years ago
|
||
Ok, the trick here is that you need to supply signed en-US files in the output dir, and an unsigned en-US plus any locales you want to re-sign in the input dir. This code will use the unsigned en-US to build the unsigned hash, and will use the signed en-US build to copy files out of instead of unpacking and signing the unsigned en-US as usual. I've tested this against one locale with --previously-signed off and on, seems to work as expected, although I'd like to do some more exhaustive testing before checking this in. Also, I'll need to amend the docs to make it clear what the procedure for adding locales to an RC is (more clear than the paragraph above, anyway).
Attachment #303570 -
Flags: review?(nrthomas)
Assignee | ||
Comment 3•16 years ago
|
||
Sorry, previous patch had a typo in the arg handling.. here's a fix for that, the same besides that.
Attachment #303570 -
Attachment is obsolete: true
Attachment #303573 -
Flags: review?(nrthomas)
Attachment #303570 -
Flags: review?(nrthomas)
Assignee | ||
Updated•16 years ago
|
Whiteboard: waiting for review
Assignee | ||
Comment 4•16 years ago
|
||
Nick, feel free to take this bug if you like, since I will be out. Otherwise, I'll pick it up when I get back.
Assignee | ||
Comment 5•16 years ago
|
||
Nick, should I go ahead and add a new section on how this would be used to the signing docs? comment #2 is all I've got right now.
Comment 6•16 years ago
|
||
Comment on attachment 303573 [details] [diff] [review] handle arg correctly >Index: sign-release.pl >=================================================================== >@@ -686,7 +697,41 @@ ProcessWin32Exe() > closedir(ARCHIVE_ENTRIES); > > find(\&HashUnsignedWin32ExecutablesCallback, ($tmp)); >- find(\&SignWin32ExecutablesCallback, ($tmp)); >+ >+ if ($previouslySigned) { >+ # unpack the previously signed en-US binary >+ print("Assuming en-US EXE previously signed\n"); >+ rmtree($tmp, 0, 0); >+ mkdir($tmp); >+ >+ $exe = catfile($outDir, $exeBasename); >+ >+ print STDERR "Processing $exe...\n"; Something like "Processing previously signed $exe...\n" would be good here. Would also be useful to have a message near the top of the log in the previously signed case. >+ copy($exe, $tmp) or die "copy($exe, $tmp) failed: $!\n"; >+ >+ chdir($tmp) or die "Couldn't chdir($tmp): $!\n"; >+ >+ my $rv = RunShellCommand(command => $SEVENZIP_BIN, >+ args => ['x', $exeBasename], >+ output => 1); >+ >+ if ($rv->{'exitValue'} != 0) { >+ die "7z extraction of $exe failed\n"; >+ } >+ >+ $exeCopy = File::Spec->rel2abs($exeBasename); $exeCopy should be used to unlink the installer, similar to the unsigned case. >+ opendir(ARCHIVE_ENTRIES, $tmp) or >+ die "opendir($tmp) failed; $!\n"; >+ my @archiveEntries = (); >+ >+ while ((my $dirEntry = readdir(ARCHIVE_ENTRIES))) { >+ push(@archiveEntries, $dirEntry) if ($dirEntry !~ /^\.\.?$/); >+ } This block seems to be unused. >+ closedir(ARCHIVE_ENTRIES); >+ } else { >+ find(\&SignWin32ExecutablesCallback, ($tmp)); >+ } > > $rv = RunShellCommand(command => $SEVENZIP_BIN, > args => [ 'a', This code goes on to recreate the signed en-US installer. Is there some reason to do that ? If not, I'd prefer not bump the timestamp, eg either return early or move the code that follows into the else block. >@@ -809,6 +854,12 @@ ProcessWin32Mar() > my $marBasename = basename($mar); > my $signedTmpDir = $args{'tmpDir'}; > my $out = $args{'output'}; >+ my $previouslySigned = $args{'previouslySigned'}; >+ >+ if ($previouslySigned) { >+ print("Assuming en-US MAR previously signed\n"); >+ $mar = catfile($out, $marBasename); >+ } Apart from testing that $mar is present, can't we return early here ? Goes with the question above about needing to recreate the en-US files. Maybe you need not call ProcessWin32Mar in the previously signed case.
Attachment #303573 -
Flags: review?(nrthomas) → review-
Assignee | ||
Comment 7•16 years ago
|
||
Hmm.. re-reading this two weeks later, I think that you are right on all counts. This patch: * adds logging about being in previously signed mode at the top * unlink $exeCopy after use in previously signed case * remove unneeded ARCHIVE_ENTRIES processing * do not bother re-creating signed en-US (return early) * do not bother unpacking previously signed MAR (return early) I'll start testing this out today, I'd like to do a full run of e.g. 3.0b3 and compare the results to the actual release. Also, I'll try to make sure this doesn't break the normal mode :)
Attachment #306057 -
Flags: review?(nrthomas)
Updated•16 years ago
|
Flags: tracking1.9? → blocking1.9?
Comment 8•16 years ago
|
||
Comment on attachment 306057 [details] [diff] [review] address nick's comments This is closer to the sort of thing I had in mind, but doesn't look like it'll run as-is. >Index: sign-release.pl >=================================================================== ... >+ $exeCopy = File::Spec->rel2abs($exeBasename); >+ >+ unlink($exeCopy) or die "Couldn't delete $exeCopy: $!\n"; >+ >+ return; I think you need to "return 1" here, as ProcessWin32Directory() is checking the return value and the bare return gives undef. >@@ -809,6 +849,7 @@ ProcessWin32Mar() > my $marBasename = basename($mar); > my $signedTmpDir = $args{'tmpDir'}; > my $out = $args{'output'}; >+ my $previouslySigned = $args{'previouslySigned'}; > > if (not -f $mar) { > die "Couldn't find mar: $mar\n"; >@@ -842,7 +883,9 @@ > print STDERR "Deleting $marCopy...\n"; > unlink($marCopy) or die "Couldn't unlink($marCopy): $!\n"; > >- find(\&SignMarExecutablesCallback , ($marScratchDir)); >+ if (! $previouslySigned) { >+ find(\&SignMarExecutablesCallback , ($marScratchDir)); >+ } > > $rv = RunShellCommand(command => $UPDATE_TOOLS_REWRAP, > args => [ $marBasename, '.' ], Are these changes to ProcessWin32Mar() needed, given it's only called when previouslySigned is false ? Actually, previouslySigned isn't passed in below, so this must be leftovers from the last patch. >@@ -965,13 +1010,18 @@ > } > > my $enUsBuildMar = GetMarNameFromExeName(exe => $enUsBuildExe); >- $rv = ProcessWin32Mar(tmpDir => $scratchDir, >- mar => catfile($dirToProcess, $enUsBuildMar), >- output => $outputDir); >- >- if (not $rv) { >- die 'ProcessWin32Directory(): ProcessWin32Mar() failed at some point. ' . >- ":-(\n"; >+ >+ if ($previouslySigned) { >+ print("Assuming previously signed en-US\n"); Since this duplicates the message when validating args, you could make it say ... previously signed en-US mar. >+ } else { >+ $rv = ProcessWin32Mar(tmpDir => $scratchDir, >+ mar => catfile($dirToProcess, $enUsBuildMar), >+ output => $outputDir); >+ >+ if (not $rv) { >+ die 'ProcessWin32Directory(): ProcessWin32Mar()' . >+ ' failed at some point. ' . ":-(\n"; >+ }
Attachment #306057 -
Flags: review?(nrthomas) → review-
Assignee | ||
Comment 9•16 years ago
|
||
Can you tell I didn't get a chance to test yet? :) Thanks Nick. * remove unused code * remove redundant logging * return 1 for success, not undef
Attachment #303573 -
Attachment is obsolete: true
Attachment #306057 -
Attachment is obsolete: true
Attachment #306283 -
Flags: review?(nrthomas)
Comment 10•16 years ago
|
||
Had time to do some testing ? Should I review now ?
Assignee | ||
Updated•16 years ago
|
Whiteboard: waiting for review → testing
Assignee | ||
Comment 11•16 years ago
|
||
(In reply to comment #10) > Had time to do some testing ? Should I review now ? Sorry, not yet. Changed the whiteboard msg, I'll try to get it done this week.
Comment 12•16 years ago
|
||
While definitely important, I can't see this blocking the release unless it simply means we can't do a release. Please renom if I don't understand the issue (which seems like we'd have a work around for a script error, i.e., manual intervention). -.
Flags: blocking1.9? → blocking1.9-
Assignee | ||
Comment 13•16 years ago
|
||
Fortunately didn't need this for b4, lowering priority. All that's left is final testing, I don't have time at the moment but if someone else does feel free to take this.
Priority: P1 → P3
Assignee | ||
Comment 14•16 years ago
|
||
(In reply to comment #10) > Had time to do some testing ? Should I review now ? Ok, tested this out today/yesterday. I used the 3.0b4 files, and it looks like the old case ("dir mode") works just fine, as far as I can tell. To test the --previously-signed mode: 1) create resigned-rc2 dir, move signed en-US into it 2) backup unsigned-rc2, remove everything except AR and en-US 3) sign as usual with above dirs and --previously-signed The only difference from the AR built with the usual case and with the --previously-signed case is that helper.exe and uninstall.exe are different (because they have localized strings, these are always re-signed). However, for the purposes of this bug I think this is fine, because this is exactly what you'd want to happen if for example you are inserting a new locale into the RC. Nick, I am planning on adding a section to the signing doc detailing those three steps (with examples), does this (and the patch) all look ok?
Whiteboard: testing → waiting for review
Comment 15•16 years ago
|
||
Sorry I ran out of time to review this but the steps sound sane. Seems like you you'd have to change the command line for the new output dir, so I'd probably make a new input dir rather than backing up [maybe unsigned-rc2, signed-rc2 for the first run; unsigned-rc2-2 signed-rc2-2 for the second].
Assignee | ||
Comment 16•16 years ago
|
||
(In reply to comment #15) > Sorry I ran out of time to review this but the steps sound sane. Seems like you > you'd have to change the command line for the new output dir, so I'd probably > make a new input dir rather than backing up [maybe unsigned-rc2, signed-rc2 for > the first run; unsigned-rc2-2 signed-rc2-2 for the second]. Ok, let me know when you have time :) Do you want me to run the doc changes by you before r+? I was thinking of doing them after landing, as this adds a new optional feature and does not change normal operation.
Comment 17•16 years ago
|
||
Will attempt to review this on the plane out, and doc'ing later is fine by me.
Comment 18•16 years ago
|
||
Comment on attachment 306283 [details] [diff] [review] take 4 >Index: sign-release.pl >=================================================================== >@@ -686,7 +699,35 @@ > closedir(ARCHIVE_ENTRIES); > > find(\&HashUnsignedWin32ExecutablesCallback, ($tmp)); >- find(\&SignWin32ExecutablesCallback, ($tmp)); >+ >+ if ($previouslySigned) { >+ # unpack the previously signed en-US binary >+ rmtree($tmp, 0, 0); >+ mkdir($tmp); Nit: I wonder what Perl does when you remove the cwd (earlier there is a chdir to $tmp to extract the unsigned en-US exe). >+ $exe = catfile($outDir, $exeBasename); >+ >+ print STDERR "Processing previously signed $exe...\n"; >+ copy($exe, $tmp) or die "copy($exe, $tmp) failed: $!\n"; >+ >+ chdir($tmp) or die "Couldn't chdir($tmp): $!\n"; Nit: already in this directory, required because of rmtree above ? >+ my $rv = RunShellCommand(command => $SEVENZIP_BIN, >+ args => ['x', $exeBasename], >+ output => 1); >+ >+ if ($rv->{'exitValue'} != 0) { >+ die "7z extraction of $exe failed\n"; >+ } >+ >+ $exeCopy = File::Spec->rel2abs($exeBasename); >+ >+ unlink($exeCopy) or die "Couldn't delete $exeCopy: $!\n"; >+ >+ return 1; >+ } else { >+ find(\&SignWin32ExecutablesCallback, ($tmp)); >+ } > > $rv = RunShellCommand(command => $SEVENZIP_BIN, > args => [ 'a', Nit: Immediately following the above is rebuilding the installer, which logically belongs in the else block. ie stuff that only happens in the normal situation (aka not previously signed). Would be a bit easier to read if it moved into there. >@@ -842,8 +883,6 @@ > print STDERR "Deleting $marCopy...\n"; > unlink($marCopy) or die "Couldn't unlink($marCopy): $!\n"; > >- find(\&SignMarExecutablesCallback , ($marScratchDir)); >- > $rv = RunShellCommand(command => $UPDATE_TOOLS_REWRAP, > args => [ $marBasename, '.' ], > output => 1); I didn't understand this modification to ProcessWin32Mar(). Doesn't it prevent the en-US mar getting signed in the "normal" situation ? Looks like ProcessWin32Mar isn't called in the previously signed case. Perhaps a leftover from an earlier patch or testing ?
Assignee | ||
Comment 19•16 years ago
|
||
Thanks Nick, addressed the nits and added back the MAR signing callback; that really should not have been in this patch, sorry :(
Attachment #306283 -
Attachment is obsolete: true
Attachment #313472 -
Flags: review?
Attachment #306283 -
Flags: review?(nrthomas)
Assignee | ||
Updated•16 years ago
|
Attachment #313472 -
Flags: review? → review?(nrthomas)
Comment 20•16 years ago
|
||
Comment on attachment 313472 [details] [diff] [review] support previously signed en-US take 5 >Index: sign-release.pl >=================================================================== >+ if ($previouslySigned) { >+ # unpack the previously signed en-US binary >+ chdir($cwd); Please put the ".. or die ..." on here too. >+ rmtree($tmp, 0, 0); >+ mkdir($tmp); >+ chdir($tmp) or die "Couldn't chdir($tmp): $!\n"; >+ ... >+ unlink($exeCopy) or die "Couldn't delete $exeCopy: $!\n"; >+ >+ return 1; >+ } else { need to zap the "else {" r+ with that and some testing.
Attachment #313472 -
Flags: review?(nrthomas) → review+
Assignee | ||
Comment 21•16 years ago
|
||
Thanks Nick, I actually did "perl -c" this time :P Checking in sign-release.pl; /mofo/release/signing/tools/sign-release.pl,v <-- sign-release.pl new revision: 1.23; previous revision: 1.22 done
Assignee | ||
Comment 22•16 years ago
|
||
Going to post some test results before closing this out.
Whiteboard: waiting for review → testing
Assignee | ||
Comment 23•16 years ago
|
||
I put up some samples here: <url> RC1 is signed the usual way. Here's the procedure for RC2 (which is just the AR locale): * create signed-rc2-partial and unsigned-rc2-partial * copy signed-rc1/*.en-US.* to both above dirs * put any files that need signing into unsigned-rc2-partial * run sign-release.pl as usual on -partial dirs, plus new "-p" option Not sure that it's really necessary to change the dir at all, but appending "-partial" seems to make it clear what happened here.
Assignee | ||
Updated•16 years ago
|
Whiteboard: testing → looking at test results
Assignee | ||
Comment 24•16 years ago
|
||
(In reply to comment #23) > I put up some samples here: > > <url> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/experimental/bug415966/ Nick points out that I needed to copy unsigned to unsigned-rc2-partial/ above so the hashing would work correctly, so I am re-doing the RC2 sample.
Assignee | ||
Comment 25•16 years ago
|
||
Alright, RC2 is now up. Here are the instructions: * create signed-rc2-partial/ and unsigned-rc2-partial/ * copy signed-rc1/*.en-US.* to signed-rc2-partial/ * copy unsigned-rc1/*.en-US.* to unsigned-rc2-partial/ * put any files that need signing into unsigned-rc2-partial/ * run sign-release.pl as usual on "-partial" dirs, using new "-p" option Builds and logs are at: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/experimental/bug415966/
Comment 26•16 years ago
|
||
Thanks for putting these files up for easy access. I took a look at .../bug415966/rc2/ and found something odd - the en-US installer looks to be the rc1 build, while the ar installer is rc2: diff -r en-US-install/combined/application.ini ar-install/combined/application.ini 42c42 < BuildID=2008030317 --- > BuildID=2008030714 (I unpacked the installer and rsynced all the files into combined for each locale) Files like toolkit.jar also differ, as do freebl.{chk,dll} & softokn3.{chk,dll}; and the expected differences between setup.exe and helper.exe are there. Some sort of rsync glitch, or rc1/rc2 confusion ? Actually, now I look at comment #25 again, all the references to rc2-partial should be rc1-partial, right ?
Comment 27•16 years ago
|
||
I've done some testing on keymaster too, and added docs on the intranet. I think we can close this out. What do you think Rob ? (grab me online for a copy of the docs).
Comment 28•16 years ago
|
||
Closing out.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•