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)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rhelmer, Assigned: rhelmer)

Details

(Whiteboard: looking at test results)

Attachments

(2 files, 4 obsolete files)

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: nobody → rhelmer
Priority: -- → P1
Status: NEW → ASSIGNED
Flags: blocking1.9?
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 :)
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)
Attached patch handle arg correctly (obsolete) — Splinter Review
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)
Whiteboard: waiting for review
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.
Blocks: 418926
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 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-
Attached patch address nick's comments (obsolete) — Splinter Review
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)
Flags: tracking1.9? → blocking1.9?
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-
Attached patch take 4 (obsolete) — Splinter Review
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)
Had time to do some testing ? Should I review now ?
Whiteboard: waiting for review → testing
(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.
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-
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
(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
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].
(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. 
Will attempt to review this on the plane out, and doc'ing later is fine by me.
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 ?
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)
Attachment #313472 - Flags: review? → review?(nrthomas)
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+
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
Going to post some test results before closing this out.
Whiteboard: waiting for review → testing
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. 
Whiteboard: testing → looking at test results
(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.
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/
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 ?
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).
Closing out.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: