After all the patches to get v2 signing working on OSX, package-manifest deserves a bit of a cleanup.

Try is green with this patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4a1a75b65041

Comment on attachment 8520084 [details] [diff] [review] Patch Please add a comment to the top of package-manifest.in that explains @RESPATH@ and @BINPATH@ similar to the one at http://mxr.mozilla.org/mozilla-central/source/b2g/installer/removed-files.in#1 It would be a good thing to diff before and after bundles just to be safe. r=me with those two things being done

Sorry, I could have sworn that I diffed before and after bundles, but I must have grabbed two identical bundles. Turns out that the previous patch accidentally re-enabled the creation of .chk files under Contents/MacOS and also placed the removed-files and precomplete files back in Contents/MacOS. This patch addresses this in packager.py and also adds the comment requested in the previous review.

Comment on attachment 8521552 [details] [diff] [review] Patch >diff --git a/toolkit/mozapps/installer/packager.py b/toolkit/mozapps/installer/packager.py >--- a/toolkit/mozapps/installer/packager.py >+++ b/toolkit/mozapps/installer/packager.py >@@ -344,28 +345,29 @@ def main(): >... > > # shlibsign libraries > if launcher.can_launch(): >- for lib in SIGN_LIBS: >- libbase = mozpack.path.join(binpath, '%s%s') \ >- % (buildconfig.substs['DLL_PREFIX'], lib) >- libname = '%s%s' % (libbase, buildconfig.substs['DLL_SUFFIX']) >- if copier.contains(libname): >- copier.add(libbase + '.chk', >- LibSignFile(os.path.join(args.destination, >- libname))) >+ if not mozinfo.isMac: >+ for lib in SIGN_LIBS: >+ libbase = mozpack.path.join(respath, '%s%s') \ >+ % (buildconfig.substs['DLL_PREFIX'], lib) >+ libname = '%s%s' % (libbase, buildconfig.substs['DLL_SUFFIX']) >+ if copier.contains(libname): >+ copier.add(libbase + '.chk', >+ LibSignFile(os.path.join(args.destination, >+ libname))) How are the .chk files created for Mac with this exclusion? Clearing r? for an answer to the above

As of the landing of all the v2 patches, these files are no longer created. The reason is that they could no longer be under Contents/MacOS and moving them to Contents/Resources would have broken the signature verification anyway. The creation of these files was accidentally re-enabled by the previous patch because BINPATH was changed to point to Contents/MacOS again. This allowed packager.py to find the dylibs again, hence it created the .chk files again and placed them next to the dylibs in Contents/MacOS.

Comment on attachment 8521552 [details] [diff] [review] Patch (In reply to Stephen Pohl [:spohl] from comment #5) > As of the landing of all the v2 patches, these files are no longer created. > The reason is that they could no longer be under Contents/MacOS and moving > them to Contents/Resources would have broken the signature verification > anyway. > > The creation of these files was accidentally re-enabled by the previous > patch because BINPATH was changed to point to Contents/MacOS again. This > allowed packager.py to find the dylibs again, hence it created the .chk > files again and placed them next to the dylibs in Contents/MacOS. Did someone comment in a bug that these files are no longer needed?

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #6) > Comment on attachment 8521552 [details] [diff] [review] > Patch > > (In reply to Stephen Pohl [:spohl] from comment #5) > > As of the landing of all the v2 patches, these files are no longer created. > > The reason is that they could no longer be under Contents/MacOS and moving > > them to Contents/Resources would have broken the signature verification > > anyway. > > > > The creation of these files was accidentally re-enabled by the previous > > patch because BINPATH was changed to point to Contents/MacOS again. This > > allowed packager.py to find the dylibs again, hence it created the .chk > > files again and placed them next to the dylibs in Contents/MacOS. > Did someone comment in a bug that these files are no longer needed? We've talked about this on IRC, but I think the only thing that made it into a bug comment was [1] and [2]. I had the impression that we basically didn't care about FIPS mode at the moment, but reading the bug comments again, this may just have been true to get v2 signing working and that we wanted FIPS mode to work again in the future. Ted, can you clarify your comment [2]? If we need FIPS support in FF34, we'll have to scramble to get this in. Right now, we don't bundle any .chk files... [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1047584#c5 [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1047584#c6

I just heard that the last beta build (before rc) is today, so if we do need FIPS mode in 34, we'll have to get into a mad scramble. Benjamin, would you know if we need FIPS support on OSX in FF34?

FIPS mode itself doesn't matter; it's a legal fiction and we don't bother with FIPS certification for Firefox . However, I'm not convinced whether NSS will work properly at all without those .chk files, and it may completely refuse to load HTTPS sites.

Richard, do you have any concerns here? To quickly summarize: To support v2 signatures on OSX, we had to move all the .chk files out of Contents/MacOS. Once we heard that we didn't necessarily support FIPS mode, the patches simply removed all .chk files from our .app bundles. This code is in our Firefox 34 beta builds and later (aurora, nightly) and we haven't had any reports that the missing .chk files would cause any problems. Do you have any concerns that NSS might not work properly without the .chk files (see comment 9)?

I am quite confident that NSS will work fine without the .chk files, it will simply refuse to enter FIPS mode.

Comment on attachment 8521552 [details] [diff] [review] Patch We've had cases in the past where the .chk files were invalid and everything worked except FIPS mode and this has been the case on m-c, m-a, and m-b for quite awhile now so I tend towards agreeing with Ted that it works without those files. Please file a followup to make NSS look under resources on Mac for the .chk files. Thanks!

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #12) > Please file a followup to make NSS look under resources on Mac > for the .chk files. Thanks! Filed bug 1100424.

Updated for current trunk, carrying over r+. Landed on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/132909245ca8

sorry had to back this out since this push is causing a merge conflict when merging mozilla-inbound to mozilla-central merging browser/installer/package-manifest.in 3 files to edit merging browser/installer/package-manifest.in failed! could you take a look at this, seems this conflicts with bug 1044736

Trying this again. Updated for current trunk, carrying over r+. https://hg.mozilla.org/integration/mozilla-inbound/rev/ff71fa57c6c9

Minor followup. Noticed that this should point to @BINPATH@ even though it doesn't currently break anything since @RESPATH@ and @BINPATH@ are equivalent on Windows.