528806 - Needs to package htmlpars.dll instead of gkparser.dll after bug 514665, in comm-central (apps)

Status: VERIFIED FIXED

(MailNews Core :: Build Config, defect)

Description

[Serge Gautherie (:sgautherie)]

Bug 518632 http://hg.mozilla.org/mozilla-central/rev/6c4f06ec0300
removed m-c "embedding" packaging part.

It looks like SeaMonkey should be updated:
{
Warning: package error or possible missing or unnecessary file: bin/components/gkparser.dll (packages, 76).
}
and Thunderbird too, fwiw.

Phil, Benjamin, do you agree with that?
Or should something else be done?

NB: This file was packaged in non-static SM 1.0.x, SM 1.1.x and SM 2.0.x.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Just remove it on m-c, Plus a little clean-up on TB side

This should be it, if I guessed right about this bug.

TB clean-up:
*Add a missing '#ifdef XP_WIN', afaict.
*Remove a duplicated (and misplaced) 'components/xmlextras.xpt'.

Comment 2

[Phil Ringnalda (:philor)]

gkparser.dll no longer existing has nothing to do with having removed an unused file that would have tried to package it except nobody was using it because it was obsolete in any number of other ways. gkparser.dll no longer exists, but htmlpars.dll does, and is pretty much essential, and this is just a duplicate of bug 519068, which I can't believe is still unfixed more than a month after I explained that all it needs is one person to build shared and look at make package-compare.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 412478 [details] [diff] [review]
(Av1) Just remove it on m-c, Plus a little clean-up on TB side

Per Phil's comments, this patch only addresses part of the issue.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)

> gkparser.dll no longer existing has nothing to do with having removed an unused
> file that would have tried to package it except nobody was using it because it
> was obsolete in any number of other ways. gkparser.dll no longer exists, but
> htmlpars.dll does, and is pretty much essential,

Thanks for the explanation: I thought I was missing something ;-<

> and this is just a duplicate
> of bug 519068, which I can't believe is still unfixed more than a month after I
> explained that all it needs is one person to build shared and look at make
> package-compare.

As you may have noticed, I have already filed a bunch of bugs and landed some patches +/- part of this issue.
You might have noticed too that comm-central has been branching, there is 3 apps to fix, releases are going on, reviewers are focused on the latters, ...
So, yes, it takes some time to fully fix!

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Fix m-c and m-1.9.2, Plus a little clean-up on TB side

Av1, with comment 3 suggestion(s).

Comment 6

[Serge Gautherie (:sgautherie)]

Mark, ping for review.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 412835 [details] [diff] [review]
(Av2) Fix m-c and m-1.9.2, Plus a little clean-up on TB side

KaiRo, in hope to move forward on the SM part at least.

Comment 8

[Serge Gautherie (:sgautherie)]

Phil told me (irc) that he would handle the TB part in bug 533043.

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Av2a-SM) Fix m-c and m-1.9.2

Comment 10

[Robert Kaiser]

Comment on attachment 416487 [details] [diff] [review]
(Av2a-SM) Fix m-c and m-1.9.2

>diff --git a/suite/installer/removed-files.in b/suite/installer/removed-files.in

>+#ifndef XP_WIN
> components/@DLL_PREFIX@htmlpars@DLL_SUFFIX@
>+#endif

> #ifdef XP_WIN
> jsj3250.dll
> components/appshell.dll
> components/cmdlines.dll
>+#ifdef MOZILLA_1_9_2_BRANCH
> components/gkparser.dll
>+#else
>+components/htmlpars.dll
>+#endif

No need to exclude it above and add it here, just let the first variant handle all platforms, please.


> #ifdef XP_WIN
> #ifdef MOZILLA_1_9_2_BRANCH
>+components/htmlpars.dll
> mozjs.dll
> #else
>+components/gkparser.dll


And why add them here again? That's very unclean. we should very much try to mention every file only once in removed-files.


And don't we need to patch the unix packages file as well?

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 416487 [details] [diff] [review]
(Av2a-SM) Fix m-c and m-1.9.2

(In reply to comment #10)

> we should very much try to mention every file only once in removed-files.

I know that, but I don't know how to do it (better) in this case as m-c and m-1.9.2 need "opposite" behaviors ... and static/non-static doesn't help at that either :-|

I think we have to live with such things until c-1.9.2 is created ... much like we had to add a lot of #ifdef before c-1.9.1 existed.

Re-asking for review ... or suggestion on how to achieve a better result.

> And don't we need to patch the unix packages file as well?

No, bug 514665 is Windows only.

Comment 12

[Robert Kaiser]

Comment on attachment 416487 [details] [diff] [review]
(Av2a-SM) Fix m-c and m-1.9.2

Still minus as I think you shouldn't ifdef the file in the beginning just to add it twice below.

Also, I think we should go for a more inclusive solution, as this is only the tip of the iceberg, as philor already mentioned, and I now analyzed a bit more, see bug 519068 comment#17

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Av3-SM) Fix m-c and m-1.9.2 [Checkin: See comment 15]

Av2a-SM, with comment 12 suggestion(s).

(In reply to comment #12)

> (From update of attachment 416487 [details] [diff] [review])
> Still minus as I think you shouldn't ifdef the file in the beginning just to
> add it twice below.

Done (per our irc discussion).

> Also, I think we should go for a more inclusive solution, as this is only the
> tip of the iceberg, as philor already mentioned, and I now analyzed a bit more,
> see bug 519068 comment#17

I know about that, but I wanted a review of a single case first.
Also, it may be easier (for me) to continue to do it in multiple patches: I'll see when I work on the next one(s).

Comment 14

[Robert Kaiser]

Comment on attachment 417151 [details] [diff] [review]
(Av3-SM) Fix m-c and m-1.9.2
[Checkin: See comment 15]

r=me when you also remove the whitespace changes, which are unrelated and I don't see a need for them.

Also, don't expect my reviewes on any of the other patches to be faster, as I need to look into the surroundings of the patch every time and need to call everything into mind so that I understand it. Taking that into account and looking at the pace we're at, it will take a few months until we have working builds from trunk again.
On the other hand, if you stick all the mentioned changes (see previously cited comment) I only need to look everything up one time and we could have working builds next week...

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 417151 [details] [diff] [review]
(Av3-SM) Fix m-c and m-1.9.2
[Checkin: See comment 15]


http://hg.mozilla.org/comm-central/rev/315c6fa822e0
Av3-SM, with comment 14 suggestion(s).

Comment 16

[Serge Gautherie (:sgautherie)]

V.Fixed, per tinderbox.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-SM-191) Fix m-1.9.1 [Checkin: Comment 18]

Support downgrading.

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 417309 [details] [diff] [review]
(Bv1-SM-191) Fix m-1.9.1
[Checkin: Comment 18]


http://hg.mozilla.org/releases/comm-1.9.1/rev/f5d52ff41bc0