521523 - Port |Bug 511642 - use a single packaging manifest across all three platforms (with preprocessing)| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Serge Gautherie (:sgautherie)]

I guess we may want to do that for our apps too?

Comment 1

[Phil Ringnalda (:philor)]

That would be a per-app decision, not something mailnews could or should decide for both SM and Tb.

Comment 2

[Robert Kaiser]

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

[(not currently active) Ted Mielczarek]

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]

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)]

Would it make sense to start by merging our Unix and Windows files, then add MacOSX as a second step?

Comment 6

[Robert Kaiser]

Taking, I have started to work on a patch.

Comment 7

[Robert Kaiser]

Attachment: 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)]

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]

Attachment: 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)]

Attachment: 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)]

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]

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]

(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

[(not currently active) Ted Mielczarek]

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]

Ah, ted, thanks, good to know.

Comment 16

[Justin Wood (:Callek)]

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]

Attachment: 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]

(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)]

(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)]

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]

Attachment: removed-files update, v2

This patch moves the XPTs to the end of the file into their own section.

Comment 22

[Robert Kaiser]

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)]

(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)]

Attachment: (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]

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)]

(In reply to comment #25)

Ah. Ftr, why is this hack not needed anymore on c-c?

Comment 27

[Robert Kaiser]

(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)]

(In reply to comment #27)

I filed bug 545777.