Last Comment Bug 730862 - Disable signmar by default and provide an option to enable it
: Disable signmar by default and provide an option to enable it
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: unspecified
: x86_64 Windows 7
P1 normal (vote)
: mozilla12
Assigned To: Brian R. Bondy [:bbondy]
:
: Robert Strong [:rstrong] (use needinfo to contact me)
Mentors:
Depends on:
Blocks: 730618
  Show dependency treegraph
 
Reported: 2012-02-27 10:01 PST by Brian R. Bondy [:bbondy]
Modified: 2012-04-12 16:16 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed


Attachments
Patch v1. (2.58 KB, patch)
2012-02-27 10:01 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v2. (6.46 KB, patch)
2012-02-27 12:41 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v3. (13.71 KB, patch)
2012-02-27 13:20 PST, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
khuey: superreview+
bhearsum: feedback+
Details | Diff | Splinter Review
Fix for signmar option not working (887 bytes, patch)
2012-03-23 18:37 PDT, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
Details | Diff | Splinter Review
Signmar test (1.93 KB, patch)
2012-03-29 18:44 PDT, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
Details | Diff | Splinter Review
fix for other platforms (14.68 KB, patch)
2012-04-02 20:33 PDT, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
Details | Diff | Splinter Review
signmar (16.23 KB, application/zip)
2012-04-09 16:12 PDT, Brian R. Bondy [:bbondy]
no flags Details

Description User image Brian R. Bondy [:bbondy] 2012-02-27 10:01:04 PST
Created attachment 600947 [details] [diff] [review]
Patch v1.

When repackaging builds we do a selective build of specific modules.  There is no need for signmar when we do this but we do need to build mar.  This task is to provide a define that can be used to selectively not build signmar when building modules/libmar.
Comment 1 User image Ben Hearsum (:bhearsum) 2012-02-27 10:37:01 PST
Comment on attachment 600947 [details] [diff] [review]
Patch v1.

I'm testing out this patch by hand on a slave.
Comment 2 User image Brian R. Bondy [:bbondy] 2012-02-27 10:38:10 PST
You probably already see this, but you also have to have NO_SIGN_VERIFY defined.
Comment 3 User image Brian R. Bondy [:bbondy] 2012-02-27 11:31:16 PST
Comment on attachment 600947 [details] [diff] [review]
Patch v1.

Going to disable by default and add the option to enable it in Nightly and Debug config/mozconfigs

That way no buildconfig for repackaging MARs will be needed
Comment 4 User image Brian R. Bondy [:bbondy] 2012-02-27 12:41:21 PST
Created attachment 601018 [details] [diff] [review]
Patch v2.
Comment 5 User image Brian R. Bondy [:bbondy] 2012-02-27 13:20:31 PST
Created attachment 601035 [details] [diff] [review]
Patch v3.

Added in the default mozconfigs for all platforms not just Windows to build signmar so that the libmar sign/verify tests can run by default on all platforms.
Comment 6 User image Nick Thomas [:nthomas] 2012-02-27 13:29:18 PST
Any particular reason to build to set --enable-signmar on debug builds ? We don't produce mar files or set --enable-update-packaging there.
Comment 7 User image Ben Hearsum (:bhearsum) 2012-02-27 13:31:02 PST
We talked a bit on IRC about that and figured it would be good to test signmar compilation with any code changes, and there might be some tests too?
Comment 8 User image Brian R. Bondy [:bbondy] 2012-02-27 13:33:14 PST
We don't use signmar for producing MAR files at all, we use the dist/host/bin/mar program instead.  signmar is just for the signing server.

The reason to build signmar for both Nightly and Debug is the same, so that modules/libmar's unit tests can always be run.
Comment 9 User image Brian R. Bondy [:bbondy] 2012-02-27 13:36:24 PST
It sounds a bit confusing but --enable-signmar doesn't enable signing of MAR files, it enables building of the signmar program.
Comment 10 User image Robert Strong [:rstrong] (use needinfo to contact me) 2012-02-28 00:27:53 PST
Comment on attachment 601035 [details] [diff] [review]
Patch v3.

Might be a good thing to run the new --enable-signmar by khuey
Comment 11 User image Ben Hearsum (:bhearsum) 2012-02-28 06:38:43 PST
Comment on attachment 601035 [details] [diff] [review]
Patch v3.

This patch worked for me in a minimal test. I didn't do a full l10n build, but I did go through the autoconf, configure, and module building it does. modules/libmar built without issue.
Comment 12 User image Brian R. Bondy [:bbondy] 2012-02-28 06:40:58 PST
I tried a full build locally without the mozconfig option and it didn't build signmar so it looks good to me as well.  Try and Elm tests also passed.  I'm doing a second nightly now and will make sure nothing is broken.  I'll push soon as long as khuey gives an sr+.
Comment 13 User image Brian R. Bondy [:bbondy] 2012-02-28 06:42:10 PST
Comment on attachment 601035 [details] [diff] [review]
Patch v3.

This is very important because repackaged builds are currently broken, a fast super-review would be greatly appreciated :D
Comment 14 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-28 07:14:53 PST
Comment on attachment 601035 [details] [diff] [review]
Patch v3.

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

I'm fine with adding --enable-signmar.  Deferring to rs on everything else.
Comment 15 User image Brian R. Bondy [:bbondy] 2012-02-28 11:01:30 PST
http://hg.mozilla.org/mozilla-central/rev/5439f4751116
Comment 16 User image Brian R. Bondy [:bbondy] 2012-02-29 18:31:30 PST
http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97
Comment 17 User image Ben Hearsum (:bhearsum) 2012-03-20 05:35:37 PDT
I don't think this worked...I was just looking for a signmar binary in an OS X objdir on mozilla-inbound, and couldn't find it:
moz2-darwin10-slave07:obj-firefox cltbld$ ls -l i386/modules/libmar/
total 8
-rw-rw-r--   1 cltbld  admin  2312 Mar 19 01:27 Makefile
drwxrwxr-x  12 cltbld  admin   408 Mar 19 20:44 src
drwxrwxr-x   6 cltbld  admin   204 Mar 19 20:44 tool
moz2-darwin10-slave07:obj-firefox cltbld$ ls -l x86_64/modules/libmar/
total 8
-rw-rw-r--   1 cltbld  admin  2312 Mar 19 02:47 Makefile
drwxrwxr-x  12 cltbld  admin   408 Mar 19 22:03 src
drwxrwxr-x   6 cltbld  admin   204 Mar 19 22:03 tool
moz2-darwin10-slave07:obj-firefox cltbld$ pwd
/builds/slave/m-in-osx64/build/obj-firefox
moz2-darwin10-slave07:obj-firefox cltbld$ cat ../.mozconfig
. $topsrcdir/build/macosx/universal/mozconfig

# Universal builds override the default of browser (bug 575283 comment 29)
ac_add_options --enable-application=browser

ac_add_options --enable-update-channel=${MOZ_UPDATE_CHANNEL}
ac_add_options --enable-update-packaging
ac_add_options --enable-codesighs
ac_add_options --disable-install-strip
ac_add_options --enable-signmar

# Nightlies only since this has a cost in performance
ac_add_options --enable-js-diagnostics

# Needed to enable breakpad in application.ini
export MOZILLA_OFFICIAL=1

export MOZ_TELEMETRY_REPORTING=1
mk_add_options MOZ_MAKE_FLAGS="-j4"

ac_add_options --with-macbundlename-prefix=Firefox

# Treat warnings as errors in directories with FAIL_ON_WARNINGS.
ac_add_options --enable-warnings-as-errors

# Package js shell.
export MOZ_PACKAGE_JSSHELL=1
Comment 18 User image Brian R. Bondy [:bbondy] 2012-03-20 17:49:39 PDT
I spotted the problem here in configure.in:

> MOZ_ARG_ENABLE_BOOL(sign-mar,

It should instead be:

> MOZ_ARG_ENABLE_BOOL(signmar,

So basically we are never building it right now.

Please change this locally for now if you need to build it.  I don't want to land this until I am around and can test it. It will enable some tests as well and I suspect some of the tests with fail with the major version increase, but I'm not sure about that yet.
Comment 19 User image Brian R. Bondy [:bbondy] 2012-03-23 18:37:39 PDT
Created attachment 608944 [details] [diff] [review]
Fix for signmar option not working

Will ask for review after I try the associated tests with 14.0a1 (instead of 13.0a1, I think there are some binary MAR comparisons that might have broken)
Comment 20 User image Brian R. Bondy [:bbondy] 2012-03-29 18:44:53 PDT
Created attachment 610788 [details] [diff] [review]
Signmar test

The create mar test in particular had to have the channel and version set to what the reference MARs have.

Also I found a bug in the binary comparison check for the create mar test. 
The reference MARs actually have the following data encoded: 
Version: 13.0a1
Channel: @MAR_CHANNEL_ID@
So I match that via the optional signmar command line parameters that can be passed when creating a MAR.
Comment 21 User image Brian R. Bondy [:bbondy] 2012-04-02 20:33:32 PDT
Created attachment 611707 [details] [diff] [review]
fix for other platforms

I had to create different MARs for windows vs other platforms because of the permissions inside the MAR file.  This patch fixes that since the tests compare the binary MAR file data.  The try tests were failing on non windows platforms before this patch.
Comment 22 User image Ben Hearsum (:bhearsum) 2012-04-09 10:36:37 PDT
I need an OS X signmar...let's throw this patch at try.
Comment 23 User image Ben Hearsum (:bhearsum) 2012-04-09 11:10:37 PDT
Apparently autosign is broken right now. I pushed this by hand: https://tbpl.mozilla.org/?tree=Try&rev=0ed2f9764b3e
Comment 24 User image Ben Hearsum (:bhearsum) 2012-04-09 12:35:48 PDT
(In reply to Ben Hearsum [:bhearsum] from comment #23)
> Apparently autosign is broken right now. I pushed this by hand:
> https://tbpl.mozilla.org/?tree=Try&rev=0ed2f9764b3e

This didn't build signmar, either, unless it's only built for the x86_64 target.
Comment 25 User image Brian R. Bondy [:bbondy] 2012-04-09 12:38:54 PDT
Looked like you pushed the fix to the tests but not the fix to the configure.in which is the important one:
https://bugzilla.mozilla.org/attachment.cgi?id=608944
Comment 26 User image Ben Hearsum (:bhearsum) 2012-04-09 13:48:49 PDT
I applied that patch locally, re-ran autoconf & configure, and built modules/libmar by hand (for purposes of speed). I ended up with a 'signmar' binary that was actually a 'mar' binary:
try-mac64-slave14:bin cltbld$ ./signmar
usage:
  mar [-H MARChannelID] [-V ProductVersion] [-C workingDir] {-c|-x|-t|-T} archive.mar [files...]
  mar [-H MARChannelID] [-V ProductVersion] [-C workingDir] -i unsigned_archive_to_refresh.mar
Comment 27 User image Ben Hearsum (:bhearsum) 2012-04-09 13:49:16 PDT
the mozconfig looked like this:
try-mac64-slave14:build cltbld$ cat .mozconfig
. $topsrcdir/build/macosx/universal/mozconfig

# Universal builds override the default of browser (bug 575283 comment 29)
ac_add_options --enable-application=browser

ac_add_options --enable-update-channel=${MOZ_UPDATE_CHANNEL}
ac_add_options --enable-update-packaging
ac_add_options --enable-codesighs
ac_add_options --disable-install-strip
ac_add_options --enable-signmar

# Nightlies only since this has a cost in performance
ac_add_options --enable-js-diagnostics

# Needed to enable breakpad in application.ini
export MOZILLA_OFFICIAL=1

export MOZ_TELEMETRY_REPORTING=1
mk_add_options MOZ_MAKE_FLAGS="-j4"

ac_add_options --with-macbundlename-prefix=Firefox

# Treat warnings as errors in directories with FAIL_ON_WARNINGS.
ac_add_options --enable-warnings-as-errors

# Package js shell.
Comment 28 User image Brian R. Bondy [:bbondy] 2012-04-09 16:12:06 PDT
Created attachment 613425 [details]
signmar

Here's a bin I just build using the configure.in patch (change sign-mar to signmar).  I also had to add: ac_add_options --enable-signmar in my .mozconfig.

It was built with:
Mac OS X Lion 10.7.3

I think you probably have it setup locally correctly but you have to do a clean build.  I think somehow your signmar option is on but some object files are compiled wrongly.
Comment 29 User image Ben Hearsum (:bhearsum) 2012-04-10 07:17:30 PDT
Weird. I just started from scratch on a new machine, and it worked fine. Thanks bbondy, I have no clue what I was doing wrong =\.

Note You need to log in before you can comment on or make changes to this bug.