545534 - update SM cc unified package manifest for OS/2

Status: VERIFIED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Walter Meinl]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.3a2pre) Gecko/20100210 SeaMonkey/2.1a1pre
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.3a2pre) Gecko/20100210 SeaMonkey/2.1a1pre

Add OS/2 bits to the new package manifest for all platforms.

Reproducible: Always

Comment 1

[Walter Meinl]

Attachment: patch

It's a bit noisy since we must use 8+3 names for dll's

Comment 2

[Robert Kaiser]

Comment on attachment 426377 [details] [diff] [review]
patch

>-#elifdef XP_WIN32
>+#elifndef XP_UNIX

Hrm, those "#elifndef XP_UNIX" are _really_ hard to read, is there really no better way?

>+#elifndef XP_OS2
> @BINPATH@/chrome/icons/default/gif-file.ico

That one is wrong. We never come here as it's an "_else_ ifndef" there. r- for that.

Comment 3

[Walter Meinl]

Attachment: v2 shuffling to get rid of elifndef xp_unix

(In reply to comment #2)
> (From update of attachment 426377 [details] [diff] [review])
> >-#elifdef XP_WIN32
> >+#elifndef XP_UNIX
> 
> Hrm, those "#elifndef XP_UNIX" are _really_ hard to read, is there really no
> better way?
> 
If I would change the order of the blocks I could at least reduce it to ifndef XP_UNIX which may be at least a little bit better to read.
Done in the new attachment.
Another possibility would be to introduce a new define that combines XP_WIN and XP_OS2 (there was an XP_PC define long ago for that) in the Makefile. Unfortunately, I don't see a way to do it directly in the package manifest. The problem is that the python (and perl) pre-processor is not able to interpret "OR"s and "AND"s.
Likewise, a combination of WIN_MOZ_1_9_2 and XP_OS2 would be nice for a handful of cases to reduce redundancy
> >+#elifndef XP_OS2
> > @BINPATH@/chrome/icons/default/gif-file.ico
> 
> That one is wrong. We never come here as it's an "_else_ ifndef" there. r- for
> that.
Sure? that comes directly after an elifndef XP_UNIX, thus either XP_WIN or XP_OS2 was defined before and both package the files which exist for win and os2. When it's down at the elifndef XP_OS2 it should go on in case XP_WIN is defined and not XP_OS2, if it was XP_OS2 it should stop skip *-file.ico files we don't ship on OS/2.
For the OS/2 case it works I don't see any "possible missing or unnecessary file" in my log. Anyway the new attachment does it the other way round.

Comment 4

[Peter Weilbacher]

If you need extra macros in pre-processed files, you can do something like this:

#ifdef XP_WIN
#define XP_WIN_OR_OS2
#endif
#ifdef XP_OS2
#define XP_WIN_OR_OS2
#endif

near the top of the file. Then further use #ifdef IS_WIN_OR_OS2 for the cases where OS/2 and Windows need the same files. It's not pretty, but it's been used it in the past (I just don't find any more, where it was used).

Comment 5

[Robert Kaiser]

Comment on attachment 426584 [details] [diff] [review]
v2 shuffling to get rid of elifndef xp_unix

> ; shell icons
>-#ifdef MOZ_GTK2
>+#ifndef XP_UNIX
>+@BINPATH@/chrome/icons/default/bmPropsWindow.ico
>+@BINPATH@/chrome/icons/default/bookmark-window.ico
>+@BINPATH@/chrome/icons/default/downloadManager.ico
>+@BINPATH@/chrome/icons/default/editorWindow.ico
>+@BINPATH@/chrome/icons/default/findBookmarkWindow.ico
>+@BINPATH@/chrome/icons/default/findHistoryWindow.ico
>+@BINPATH@/chrome/icons/default/history-window.ico
>+@BINPATH@/chrome/icons/default/JSConsoleWindow.ico
>+@BINPATH@/chrome/icons/default/main-window.ico
>+#elifdef XP_WIN32
>+@BINPATH@/chrome/icons/default/gif-file.ico
>+@BINPATH@/chrome/icons/default/html-file.ico
>+@BINPATH@/chrome/icons/default/misc-file.ico
>+@BINPATH@/chrome/icons/default/image-file.ico
>+@BINPATH@/chrome/icons/default/jpeg-file.ico
>+@BINPATH@/chrome/icons/default/script-file.ico
>+@BINPATH@/chrome/icons/default/xml-file.ico
>+@BINPATH@/chrome/icons/default/xul-file.ico
>+#elifdef MOZ_GTK2

That logic still probably doesn't work correctly, follow those thoughts:
- if not unix-based, use the window .ico files
- ELSE if Windows, use the file .ico files
- else if GTK2...

Do you note that we never get to the file icons?
Please put an |#ifdef XP_WIN32 ... #endif| block _inside_ the |#ifndef XP_UNIX| block (or WIN_OR_OS2 block, see below) instead.

I'd prefer if you define WIN_OR_OS2 as well as WIN_1_9_2_OR_OS2 flags in the Makefile.in that is parallel to this file (just like I did for WIN_MOZ_1_9_2), that would make both the short filename things as well as the blocks like those .ico additions easier.

We're getting there, though, thanks for working on this.

Comment 6

[Walter Meinl]

Attachment: v3 including new Makfile defines

>Do you note that we never get to the file icons?
Yes, I think I got it now :">
>Please put an |#ifdef XP_WIN32 ... #endif| block _inside_ the |#ifndef XP_UNIX|
>block (or WIN_OR_OS2 block, see below) instead.

In addition, the manifest fixes the case for platforms that are not ready yet for necko-wifi (means us, or if --disable-necko-wifi was used for the build) and a few days ago http://hg.mozilla.org/mozilla-central/rev/cbb7edcc6d77 removed contentprefs.xpt from mozilla-central. I tried to simplify the README/README.txt cases as well.

Comment 7

[Walter Meinl]

Comment on attachment 426892 [details] [diff] [review]
v3 including new Makfile defines

>-ifeq (1WINNT,$(MOZILLA_1_9_2_BRANCH)$(OS_ARCH))
>-DEFINES += -DWIN_MOZ_1_9_2=1
>+ifeq (WINNT, $(OS_ARCH))
>+ifdef MOZILLA_1_9_2_BRANCH
>+DEFINES += -DWIN_MOZ_1_9_2=1 -DWIN_1_9_2_OR_OS2=1
>+else
>+DEFINES += -DWIN32_OR_OS2=1
>+endif
>+endif
Looking at this again together with the patch in the manifest, I think, this is wrong for the icons, where in the manifest is ifdef WIN32_OR_OS2 would probably not package the windows files for the branch.

This was the first version I tried, thought I could combine it better
 ifeq (1WINNT,$(MOZILLA_1_9_2_BRANCH)$(OS_ARCH))
-DEFINES += -DWIN_MOZ_1_9_2=1
+DEFINES += -DWIN_MOZ_1_9_2=1 -DWIN_1_9_2_OR_OS2=1
+
+ifeq (WINNT, $(OS_ARCH))
+DEFINES += -DWIN32_OR_OS2=1
>+
>+ifeq (OS2, $(OS_ARCH))
>+DEFINES += -DWIN32_OR_OS2=1 -DWIN_1_9_2_OR_OS2=1
> endif
>

Comment 8

[Robert Kaiser]

(In reply to comment #7)
> This was the first version I tried, thought I could combine it better
>  ifeq (1WINNT,$(MOZILLA_1_9_2_BRANCH)$(OS_ARCH))
> -DEFINES += -DWIN_MOZ_1_9_2=1
> +DEFINES += -DWIN_MOZ_1_9_2=1 -DWIN_1_9_2_OR_OS2=1
> +
> +ifeq (WINNT, $(OS_ARCH))
> +DEFINES += -DWIN32_OR_OS2=1
> >+
> >+ifeq (OS2, $(OS_ARCH))
> >+DEFINES += -DWIN32_OR_OS2=1 -DWIN_1_9_2_OR_OS2=1
> > endif
> >

This adds -DWIN32_OR_OS2=1 twice for Windows builds, which is not what we want. Remove that definition from the first instance, please. The other two look good to me.

Comment 9

[Robert Kaiser]

Oh, and please use WIN_OR_OS2, actually, the "32" is mostly useless, as win64 uses the same names.

And thanks for trying to care about things like wifi and crashreporter, but please put all those things into their own bugs, some of those actually don't work the way you did put them there as we e.g. don't define MOZ_CRASHREPORTER in comm-central.

Comment 10

[Walter Meinl]

(In reply to comment #8)
> (In reply to comment #7)
> > This was the first version I tried, thought I could combine it better
> >  ifeq (1WINNT,$(MOZILLA_1_9_2_BRANCH)$(OS_ARCH))
> > -DEFINES += -DWIN_MOZ_1_9_2=1
> > +DEFINES += -DWIN_MOZ_1_9_2=1 -DWIN_1_9_2_OR_OS2=1
> > +
> > +ifeq (WINNT, $(OS_ARCH))
> > +DEFINES += -DWIN32_OR_OS2=1
> > >+
> > >+ifeq (OS2, $(OS_ARCH))
> > >+DEFINES += -DWIN32_OR_OS2=1 -DWIN_1_9_2_OR_OS2=1
> > > endif
> > >
> 
> This adds -DWIN32_OR_OS2=1 twice for Windows builds, which is not what we want.
> Remove that definition from the first instance, please. The other two look good
> to me.

Sorry, don't understand what you mean exactly here. In the first instance it's WIN_1_9_2_OR_OS2, in the second it's WIN_OR_OS2.

Comment 11

[Robert Kaiser]

(In reply to comment #10)
> Sorry, don't understand what you mean exactly here. In the first instance it's
> WIN_1_9_2_OR_OS2, in the second it's WIN_OR_OS2.

Oh, right, confused the _OR_OS2 flags with each other. I'll cheer when we can just remove the 1.9.2 flags. ;-)

Comment 12

[Walter Meinl]

Attachment: v4 including new Makfile defines

I tried again to combine in the Makefile the different Defines. Now it should be right. This combination shouldn't exclude Win on the branch from installing the icon files (as the last did). I removed the over-eager "corrections" from the packager file as well. Here we go... ;-)

Comment 13

[Robert Kaiser]

Comment on attachment 427008 [details] [diff] [review]
v4 including new Makfile defines

Sorry, I hate to do this, but the targets just changed under our feet today. We are dropping 1.9.2 support on suite/ completely, so we can just remove complexity here and make the WIN_1_9_2_OR_OS2 stuff be just OS/2 instead. Feel free to put the complete elimination of WIN_MOZ_1_9_2 into that patch as well.

Your work was not useless, it was one of the pain points I had in mind to eliminate when bringing the topic into discussion and driving a decision to drop 1.9.2 support.

Comment 14

[Walter Meinl]

Attachment: drop 1_9_2-related defines

(In reply to comment #13)
> (From update of attachment 427008 [details] [diff] [review])
> Sorry, I hate to do this, but the targets just changed under our feet today. We
> are dropping 1.9.2 support on suite/ completely, so we can just remove
> complexity here and make the WIN_1_9_2_OR_OS2 stuff be just OS/2 instead. Feel
> free to put the complete elimination of WIN_MOZ_1_9_2 into that patch as well.
I dropped WIN_MOZ_1_9_2, but felt that the ifdef MOZILLA_1_9_2_BRANCH wouldn't make sense either. I will put it back again if you want me to.
 
> Your work was not useless, it was one of the pain points I had in mind to
> eliminate when bringing the topic into discussion and driving a decision to
> drop 1.9.2 support.
We few OS/2 guys trying to care about all mozilla apps don't mind if we don't have to support an additional branch.

Comment 15

[Robert Kaiser]

Comment on attachment 427212 [details] [diff] [review]
drop 1_9_2-related defines

>-#elifdef XP_WIN32
>+#elifdef WIN_OR_OS2
> @BINPATH@/chrome/icons/default/bmPropsWindow.ico
> @BINPATH@/chrome/icons/default/bookmark-window.ico
> @BINPATH@/chrome/icons/default/downloadManager.ico
> @BINPATH@/chrome/icons/default/editorWindow.ico
> @BINPATH@/chrome/icons/default/findBookmarkWindow.ico
> @BINPATH@/chrome/icons/default/findHistoryWindow.ico
> @BINPATH@/chrome/icons/default/history-window.ico
> @BINPATH@/chrome/icons/default/JSConsoleWindow.ico
> @BINPATH@/chrome/icons/default/main-window.ico
>+#endif

I think I prefer putting this #endif after the following |#ifdef XP_WIN32| block with the file icons, but it shouldn't matter much, actually.

>-#elifdef XP_WIN32
>+#elifdef WIN_OR_OS2
> @BINPATH@/extensions/{59c81df5-4b7a-477b-912d-4e0fdf64e5f2}/chrome/icons/default/chatzilla-window.ico
> #endif

Are you sure that this is an icon that can be used on OS/2?


Beside those nits, thanks a lot for doing this and for bearing with all the r- I gave you!
Oh, and extra thanks for the 1.9.2 ifdef cleanup :)

Comment 16

[Walter Meinl]

Attachment: final fixes for checkin [Checkin: Comment 18]

> I think I prefer putting this #endif after the following |#ifdef XP_WIN32|
> block with the file icons, but it shouldn't matter much, actually.
I've moved the #endif downwards in the final version
 
> >-#elifdef XP_WIN32
> >+#elifdef WIN_OR_OS2
> > @BINPATH@/extensions/{59c81df5-4b7a-477b-912d-4e0fdf64e5f2}/chrome/icons/default/chatzilla-window.ico
> > #endif
> 
> Are you sure that this is an icon that can be used on OS/2?
> 
Don't ask me why it's working but it does, I've digged down deep in mozilla-cvs it was from the first import the windows icon that was used also on OS/2. Probably since it was never replaced by a more recent ico format we can read it too.
My first wild guess was we were using the chatzilla-window icon in suite/branding/icons/os2 but that one wasn't ever used.

Comment 17

[Walter Meinl]

Serge, I added the checkin-needed keyword as a reminder. I think, this checkin will have to be coordinated with any checkin for bug546487.

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 427413 [details] [diff] [review]
final fixes for checkin
[Checkin: Comment 18]


http://hg.mozilla.org/comm-central/rev/4ffa300fc348

Comment 19

[Serge Gautherie (:sgautherie)]

Walter, have you considered asking for a committer access?