The default bug view has changed. See this FAQ.

Disable signmar by default and provide an option to enable it

RESOLVED FIXED in Firefox 12

Status

()

Toolkit
Application Update
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

unspecified
mozilla12
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox11 unaffected, firefox12 fixed, firefox13 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Attachment #600947 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

5 years ago
Attachment #600947 - Flags: feedback?(bhearsum)
Comment on attachment 600947 [details] [diff] [review]
Patch v1.

I'm testing out this patch by hand on a slave.
(Assignee)

Comment 2

5 years ago
You probably already see this, but you also have to have NO_SIGN_VERIFY defined.
(Assignee)

Comment 3

5 years ago
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
Attachment #600947 - Flags: review?(robert.bugzilla)
Attachment #600947 - Flags: feedback?(bhearsum)
(Assignee)

Comment 4

5 years ago
Created attachment 601018 [details] [diff] [review]
Patch v2.
Assignee: nobody → netzen
Attachment #600947 - Attachment is obsolete: true
Attachment #601018 - Flags: review?(robert.bugzilla)
Attachment #601018 - Flags: feedback?(bhearsum)
(Assignee)

Comment 5

5 years ago
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.
Attachment #601018 - Attachment is obsolete: true
Attachment #601035 - Flags: review?(robert.bugzilla)
Attachment #601035 - Flags: feedback?(bhearsum)
Attachment #601018 - Flags: review?(robert.bugzilla)
Attachment #601018 - Flags: feedback?(bhearsum)
Any particular reason to build to set --enable-signmar on debug builds ? We don't produce mar files or set --enable-update-packaging there.
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?
Attachment #601035 - Flags: feedback?(bhearsum) → feedback+
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
It sounds a bit confusing but --enable-signmar doesn't enable signing of MAR files, it enables building of the signmar program.
Comment on attachment 601035 [details] [diff] [review]
Patch v3.

Might be a good thing to run the new --enable-signmar by khuey
Attachment #601035 - Flags: review?(robert.bugzilla) → review+
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.
(Assignee)

Comment 12

5 years ago
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+.
(Assignee)

Comment 13

5 years ago
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
Attachment #601035 - Flags: superreview?(khuey)
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla13
(Assignee)

Updated

5 years ago
Priority: -- → P1
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.
Attachment #601035 - Flags: superreview?(khuey) → superreview+
(Assignee)

Updated

5 years ago
Summary: Provide the ability to disable signmar when building modules/libmar → Disable signmar by default and provide an option to enable it
(Assignee)

Comment 15

5 years ago
http://hg.mozilla.org/mozilla-central/rev/5439f4751116
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

5 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97
status-firefox11: --- → unaffected
status-firefox12: --- → fixed
status-firefox13: --- → fixed
Target Milestone: mozilla13 → mozilla12
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

5 years ago
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.
(Assignee)

Comment 19

5 years ago
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)
(Assignee)

Comment 20

5 years ago
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.
Attachment #610788 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

5 years ago
Attachment #608944 - Flags: review?(robert.bugzilla)
Attachment #608944 - Flags: review?(robert.bugzilla) → review+
Attachment #610788 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 21

5 years ago
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.
Attachment #611707 - Flags: review?(robert.bugzilla)
I need an OS X signmar...let's throw this patch at try.
Whiteboard: [autoland-try:611707:-b do -p macosx64 -u none -t none]
Apparently autosign is broken right now. I pushed this by hand: https://tbpl.mozilla.org/?tree=Try&rev=0ed2f9764b3e
Whiteboard: [autoland-try:611707:-b do -p macosx64 -u none -t none]
(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.
(Assignee)

Comment 25

5 years ago
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
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
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.
(Assignee)

Comment 28

5 years ago
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.
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 =\.
Attachment #611707 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 30

5 years ago
https://hg.mozilla.org/mozilla-central/rev/f75b7814f698
https://hg.mozilla.org/mozilla-central/rev/ea7a6ca0a55c
https://hg.mozilla.org/mozilla-central/rev/576a14e57ea6
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.