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 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 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 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 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 Brian R. Bondy [:bbondy] 2012-02-27 12:41:21 PST
Created attachment 601018 [details] [diff] [review]
Patch v2.
Comment 5 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 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 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 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 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 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 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 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 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 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 Brian R. Bondy [:bbondy] 2012-02-28 11:01:30 PST
http://hg.mozilla.org/mozilla-central/rev/5439f4751116
Comment 16 Brian R. Bondy [:bbondy] 2012-02-29 18:31:30 PST
http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97
Comment 17 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 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 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 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 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 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 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 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 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 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 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 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 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.