Last Comment Bug 521523 - Port |Bug 511642 - use a single packaging manifest across all three platforms (with preprocessing)| to SeaMonkey
: Port |Bug 511642 - use a single packaging manifest across all three platforms...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Robert Kaiser
:
Mentors:
: 511851 (view as bug list)
Depends on: 485935 511642 545320 545534 545628 545631 545777 547499 564606 564657
Blocks: 545471 487283 509147 524030 524033 535342 536451 539355 543289 550657
  Show dependency treegraph
 
Reported: 2009-10-09 17:52 PDT by Serge Gautherie (:sgautherie)
Modified: 2010-05-10 16:56 PDT (History)
4 users (show)
kairo: wanted‑seamonkey2.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1: create new unified manifest, replace old package files (74.40 KB, patch)
2010-02-02 07:53 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
patch v2: new unified manifest, updated for try server results (74.93 KB, patch)
2010-02-04 15:33 PST, Robert Kaiser
bugspam.Callek: review+
Details | Diff | Splinter Review
Diff of Mac package-compare [modulo some manual edits: see comment] (8.69 KB, text/plain)
2010-02-05 10:46 PST, Justin Wood (:Callek) (Away until Aug 29)
no flags Details
removed-files update (6.23 KB, patch)
2010-02-06 05:45 PST, Robert Kaiser
bugspam.Callek: review-
Details | Diff | Splinter Review
removed-files update, v2 (5.00 KB, patch)
2010-02-06 09:12 PST, Robert Kaiser
bugspam.Callek: review+
Details | Diff | Splinter Review
(Cv1-191) Fix Windows README.txt packaging on c-1.9.1 too (948 bytes, patch)
2010-02-11 04:06 PST, Serge Gautherie (:sgautherie)
kairo: review-
kairo: approval‑seamonkey2.0.4-
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2009-10-09 17:52:42 PDT
I guess we may want to do that for our apps too?
Comment 1 Phil Ringnalda (:philor) 2009-10-09 19:24:18 PDT
That would be a per-app decision, not something mailnews could or should decide for both SM and Tb.
Comment 2 Robert Kaiser 2009-10-10 05:28:09 PDT
Yes, I think we should do per-app bugs, the patches will get large enough and the decisions are really app-specific.

That said, I want it for SeaMonkey but only on 2.1, not on 2.0, and so I didn't even file it as long as we are not branched.
Comment 3 Ted Mielczarek [:ted.mielczarek] 2009-10-10 05:49:48 PDT
The Firefox patch wasn't that complicated to write. Most of the difficulty was just verifying all the differences between the packaging manifests for different platforms. I probably could have done that a little smarter by writing a script to just print out files that were packaged in only one manifest or something. It was just a matter of removing ifdefs from installer/Makefile.in, adding a bunch of #ifdefs to the package manifest, and verifying that my package manifest name change didn't break anything else.
Comment 4 Robert Kaiser 2010-01-26 18:17:51 PST
The biggest obstacle there is probably Mac, as we don't have a package manifest for that right now - we probably need one if we want to run tests on debug and opt builds and not clutter packages with test files, though.
Comment 5 Serge Gautherie (:sgautherie) 2010-01-26 20:00:54 PST
Would it make sense to start by merging our Unix and Windows files, then add MacOSX as a second step?
Comment 6 Robert Kaiser 2010-01-28 18:28:09 PST
Taking, I have started to work on a patch.
Comment 7 Robert Kaiser 2010-02-02 07:53:28 PST
Created attachment 424781 [details] [diff] [review]
patch v1: create new unified manifest, replace old package files

This patch should do it. I have tested that package-compare looks good on my Linux machines (with a shared build), but I guess the real proof that it works in the situations that are really important for us is just when it actually runs on our build boxes, i.e. after checkin.
Comment 8 Serge Gautherie (:sgautherie) 2010-02-02 08:15:52 PST
Comment on attachment 424781 [details] [diff] [review]
patch v1: create new unified manifest, replace old package files

>+++ b/suite/installer/package-manifest.in
>@@ -0,0 +1,917 @@
>+; Package file for the Firefox build. 

Nit: not 'Firefox' ;-]
Comment 9 Robert Kaiser 2010-02-04 15:33:19 PST
Created attachment 425330 [details] [diff] [review]
patch v2: new unified manifest, updated for try server results

OK, I did run the patch through the try server, updated it a bit and did run this second version through it again. Everything should look good with this version, it has only small nits corrected to the last try server run.
Comment 10 Justin Wood (:Callek) (Away until Aug 29) 2010-02-05 10:46:36 PST
Created attachment 425496 [details]
Diff of Mac package-compare [modulo some manual edits: see comment]

Robert, there are still some issues here with actual packaging...

(I stripped any - foo/* lines followed by +foo/bar  etc.)
Comment 11 Justin Wood (:Callek) (Away until Aug 29) 2010-02-05 11:00:29 PST
Comment on attachment 425330 [details] [diff] [review]
patch v2: new unified manifest, updated for try server results

Just some brief comments so far based on seeing the try-server mac build log

>diff --git a/suite/installer/package-manifest.in b/suite/installer/package-manifest.in
>new file mode 100644
>+#ifdef XP_MACOSX
>+@BINPATH@/plugins/DefaultPlugin.plugin/

Mean to use a * ?

>+
>+; Modules
>+@BINPATH@/modules/*

[see note later]

>+#ifdef XP_MACOSX
>+@BINPATH@/res/MainMenu.nib/
>+#endif

Meant to use a *?

>+#ifdef XP_MACOSX
>+@BINPATH@/updater.app/
>+#else

Meant to use a *?

>+#ifdef XP_MACOSX
>+@BINPATH@/crashreporter.app/

Don't you mean @BINPATH@/crashreporter.app/*

>+@BINPATH@/modules/gloda/*

Redundant with an earlier @BINPATH@/modules/*
(Though this is under [mail] and the earlier one is under [browser]. does that matter?)
Comment 12 Robert Kaiser 2010-02-05 11:40:49 PST
Comment on attachment 425496 [details]
Diff of Mac package-compare [modulo some manual edits: see comment]

>+SeaMonkey.app/Contents/MacOS/components/widget_cocoa.xpt

That's corrected in the patch, btw.

>-SeaMonkey.app/Contents/MacOS/crashreporter-override.ini

And so is this, we don't have that file, wrongly had copied it from FF.

>+SeaMonkey.app/Contents/MacOS/isp/dotmac.rdf

If anyone wonders, Mnyromyr has made us intentionally not ship this one.

>-SeaMonkey.app/Contents/MacOS/res/MainMenu.nib/

>-SeaMonkey.app/Contents/MacOS/updater.app/

I copied those and similar entries from FF/TB and apparently they work fine even without the * at the end. Actually, I wonder if that scheme would help us with modules/ to not get warnings about the gloda stuff...
Comment 13 Robert Kaiser 2010-02-05 11:47:09 PST
(In reply to comment #11)
> (From update of attachment 425330 [details] [diff] [review])
> >+@BINPATH@/modules/gloda/*
> 
> Redundant with an earlier @BINPATH@/modules/*
> (Though this is under [mail] and the earlier one is under [browser]. does that
> matter?)

Actually, it's not redundant, interestingly. the modules/* does only successfully add files directly under modules/ and not those in the subdir, which is why we need that additional line. This is what we had in the old manifests and what TB has as well.

It might be worth a followup to try if using just "@BINPATH@/modules/" without the * might actually make us add the whole subtree. I would like to only go for that in a followup, though, and potentially point TB folks at that as well if it works.
Comment 14 Ted Mielczarek [:ted.mielczarek] 2010-02-05 12:08:23 PST
philor noted in some other bug that just listing a directory doesn't work on Windows. Those ones you copied from the Firefox manifest are ok because they're mac-only.
Comment 15 Robert Kaiser 2010-02-05 12:19:09 PST
Ah, ted, thanks, good to know.
Comment 16 Justin Wood (:Callek) (Away until Aug 29) 2010-02-06 00:11:46 PST
Comment on attachment 425330 [details] [diff] [review]
patch v2: new unified manifest, updated for try server results

without doing a manual scan or even checking the package-compare on try win/linux. r+

If you want my closer eye on that stuff, I'll be glad to give it.

Just please do the |*| additions that I pointed out earlier based on ted's comment.

This also needs a removed-files update for the mac now. Can be a followup bug, or a new patch here, your choice.
Comment 17 Robert Kaiser 2010-02-06 05:45:36 PST
Created attachment 425635 [details] [diff] [review]
removed-files update

Here's the removed-files update needed for not breaking Mac now that we are linking XPTs. We could go and wrap all those additions in ifdef XP_MACOSX but I'm not sure this is really need or the way to go, actually...
Callek, if you wish me to do that, please let me know.
Comment 18 Robert Kaiser 2010-02-06 05:46:48 PST
(In reply to comment #16)
> Just please do the |*| additions that I pointed out earlier based on ted's
> comment.

Erm, his comment said that those where I did it are OK - and from all we know, * doesn't go deeper than one directory, while we know just / does on Mac, and all those I have in there are Mac-only.
Comment 19 Justin Wood (:Callek) (Away until Aug 29) 2010-02-06 08:41:19 PST
(In reply to comment #18)
> (In reply to comment #16)
> > Just please do the |*| additions that I pointed out earlier based on ted's
> > comment.
> 
> Erm, his comment said that those where I did it are OK - and from all we know,
> * doesn't go deeper than one directory, while we know just / does on Mac, and
> all those I have in there are Mac-only.

erm right; sorry-carry on.
Comment 20 Justin Wood (:Callek) (Away until Aug 29) 2010-02-06 08:44:37 PST
Comment on attachment 425635 [details] [diff] [review]
removed-files update

I'm more partial to a section similar to TB's:

http://mxr.mozilla.org/comm-central/source/mail/installer/removed-files.in#681

Part of the draw of that for me is "You don't want to add anything to this section" comment

But I'm ok if you would rather ifdef this as-is.
Comment 21 Robert Kaiser 2010-02-06 09:12:20 PST
Created attachment 425652 [details] [diff] [review]
removed-files update, v2

This patch moves the XPTs to the end of the file into their own section.
Comment 22 Robert Kaiser 2010-02-06 09:29:07 PST
Pushed both patches to comm-central:

http://hg.mozilla.org/comm-central/rev/a26c51539abe
http://hg.mozilla.org/comm-central/rev/7a8ee180d883
Comment 23 Serge Gautherie (:sgautherie) 2010-02-10 11:28:17 PST
(In reply to comment #22)

Ftr, this happened on waterfall page
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey&maxdate=1265527556&hours=24&legend=0&norules=1
between changesets 789dd6cd45ef and 7a8ee180d883.
Comment 24 Serge Gautherie (:sgautherie) 2010-02-11 04:06:35 PST
Created attachment 426477 [details] [diff] [review]
(Cv1-191) Fix Windows README.txt packaging on c-1.9.1 too

This line had landed as is in bug 351917.
{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.0/1265874980.1265876732.20126.gz&fulltext=1
WINNT 5.2 comm-1.9.1 build on 2010/02/10 23:56:20

make package-compare

-bin/README.txt,bin/readme.txt
+bin/README.txt
}

You fixed it on c-c here.
Comment 25 Robert Kaiser 2010-02-11 04:29:42 PST
Comment on attachment 426477 [details] [diff] [review]
(Cv1-191) Fix Windows README.txt packaging on c-1.9.1 too

This is there intentionally, as FAT32 seems to sometimes just catch the file only with lower-case spelling.
Comment 26 Serge Gautherie (:sgautherie) 2010-02-11 05:07:52 PST
(In reply to comment #25)

Ah. Ftr, why is this hack not needed anymore on c-c?
Comment 27 Robert Kaiser 2010-02-11 10:35:42 PST
(In reply to comment #26)
> (In reply to comment #25)
> 
> Ah. Ftr, why is this hack not needed anymore on c-c?

Because I just tried to go without it - or forgot to port it over. Choose one.
Comment 28 Serge Gautherie (:sgautherie) 2010-02-11 15:46:05 PST
(In reply to comment #27)

I filed bug 545777.
Comment 29 Robert Kaiser 2010-04-01 14:21:39 PDT
*** Bug 511851 has been marked as a duplicate of this bug. ***

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