342897 - Copy required suite files from xpfe/bootstrap (and elsewhere) to suite/app

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Mark Banner (:standard8)]

There are a number of files in xpfe/bootstrap that currently suite is referencing from suite/app. We need to be copying these to suite/app soon so that we can get our structure more complete and in preparation for the MOZ_XUL_APP transfer.

Here's the list of files I know about, with suggestions as to where to put them:

/xpfe/bootstrap/browser-prefs.js -> /suite/app or /suite/app/profile or /suite/browser?
/xpfe/bootstrap/apprunner-beos.rsrc -> /suite/app
/xpfe/bootstrap/icons/ -> /suite/app/icons/
/xpfe/bootstrap/module.ver -> /suite/app
/xpfe/bootstrap/mozilla.man.in -> /suite/app/suite.man.in (needs makefile update)
/xpfe/bootstrap/version.txt -> /suite/app/version.txt - needs configure.in update and also adding to client.mk
/xpfe/bootstrap/mozos2.ico -> /suite/branding/icons/os2/seamonkey.ico
/profile/defaults/bookmarks.html -> /suite/app/profile
/profile/defaults/mimeTypes.rdf -> /suite/app/profile
/profile/defaults/localstore.rdf -> /suite/app/profile
/profile/defaults/panels.rdf -> /suite/app/profile
/profile/defaults/search.rdf -> /suite/app/profile
/profile/defaults/chrome/userChrome.example -> /suite/app/profile
/profile/defaults/chrome/userContent.example -> /suite/app/profile

The user*.example files are actually missing from current suiterunner builds. They normally reside in defaults/profile/chrome within the installed directory structure.

what about:
/profile/defaults/profiledef.pkg ?
/xpfe/bootstrap/appicons.pkg - appears unused
/xpfe/bootstrap/bootstrap-mac.pkg - ditto
/xpfe/bootstrap/splash.bmp - needs splash implementing before its worth doing this
/xpfe/bootstrap/splash.xpm - ditto?

Not required:
/xpfe/bootstrap/mozilla.ico - already in branding as seamonkey.ico
/xpfe/bootstrap/mozilla.in - version already exists
/xpfe/bootstrap/mozilla.manifest - already as seamonkey.manifest
/xpfe/bootstrap/splash.rc - already exists

The /profile/defaults directory will need to be explicitly removed from suiterunner builds in case we get MOZ_SINGLE_PROFILE set to undefined again (are we likely to?).

Comments?

Are we just going to do a straight file copy and forget about cvs history for these, or do we want to do a cvs copy but leave the originals in place?

Comment 1

[Robert Kaiser]

(In reply to comment #0)
> /xpfe/bootstrap/browser-prefs.js -> /suite/app or /suite/app/profile or
> /suite/browser?

/suite/app/profile would fit with FF/TB (though I don't really like this location yet)

> /xpfe/bootstrap/icons/ -> /suite/app/icons/

nope. /suite/branding/icons/ that is.

> /xpfe/bootstrap/mozilla.man.in -> /suite/app/suite.man.in (needs makefile
> update)

seamonkey.man.in if we want to follow what we've done for other files

> /xpfe/bootstrap/version.txt -> /suite/app/version.txt - needs configure.in
> update and also adding to client.mk

when we want to fit with what FF/TB have, this needs to be /suite/config/version.txt

> /profile/defaults/bookmarks.html -> /suite/app/profile

not quite. all localizeable profile content should go into /suite/locales/en-US/profile actually - bookmarks are among those (see FF)

> /profile/defaults/profiledef.pkg ?
> /xpfe/bootstrap/appicons.pkg - appears unused
> /xpfe/bootstrap/bootstrap-mac.pkg - ditto

*.pkg can be ignored, they were intended for a installer rework that never fully landed.

> /xpfe/bootstrap/splash.bmp - needs splash implementing before its worth doing
> this
> /xpfe/bootstrap/splash.xpm - ditto?

True, we should ignore those for now.

> The /profile/defaults directory will need to be explicitly removed from
> suiterunner builds in case we get MOZ_SINGLE_PROFILE set to undefined again
> (are we likely to?).

I don't think we will do so.

> Are we just going to do a straight file copy and forget about cvs history for
> these, or do we want to do a cvs copy but leave the originals in place?

For all binary files, going without cvs copy is fine in any case, for text files, it might be a good idea to preserve cvs history.

Comment 2

[Benjamin Smedberg]

> /suite/app/profile would fit with FF/TB (though I don't really like this
> location yet)

profile/ is a bad subdirectory, please don't perpetuate it.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #1)
> (In reply to comment #0)
> > /xpfe/bootstrap/browser-prefs.js -> /suite/app or /suite/app/profile or
> > /suite/browser?
> 
> /suite/app/profile would fit with FF/TB (though I don't really like this
> location yet)

Ok, let's put it in /suite/browser or /suite/common - as Benjamin doesn't like the profile directory (quite sensible IMHO), it would make more sense to put it into one of those two. For instance suite specific mailnews prefs would possibly be kept under suite/mailnews eventually.

Comment 4

[Mark Banner (:standard8)]

Attachment: Moves for binary files.

These are the moves for the binary files (mainly icons) and one readme.txt. As discussed, we'll do these without transferring cvs history. I'm also proposing a straight move, as I can't see anyone else using these, and they should really reside in the new locations anyway.

I'll attach a patch in a short while that will do the required code changes to pull in the files in their new locations.

Comment 5

[Mark Banner (:standard8)]

Attachment: Update build process for new binary file locations.

This does the required changes to the build process for the "Moves for binary files". Note that both non MOZ_XUL_APP=1 and MOZ_XUL_APP=1 builds will now include all the icons from within /suite/branding/icons with a centralised build process.

Comment 6

[Robert Kaiser]

Comment on attachment 227396 [details] [diff] [review]
Moves for binary files.

Please copy those icons instead of moving them, you should only cvs remove them in the old location once we are using the new ones and can be sure the old ones are unused.

>mv xpfe/bootstrap/icons/os2/mozilla.ico			suite/branding/icons/os2/mozilla.ico
>mv xpfe/bootstrap/icons/windows/mozilla.ico		suite/branding/icons/windows/mozilla.ico

Are we really using this file? I don't think we are or should be using the Mozilla logo anywhere nowadays. If this is the SeaMonkey icon instead, it should be named seamonkey.ico instead of mozilla.ico, and this is already there (either already now or from the top of this patch).

Comment 7

[[e-mail]]

Comment on attachment 227396 [details] [diff] [review]
Moves for binary files.

sr=me with KaiRo's nits fixed.

Comment 8

[[e-mail]]

Comment on attachment 227397 [details] [diff] [review]
Update build process for new binary file locations.

>+libs:: $(addprefix $(srcdir)/icons/$(ICON_DIR)/,$(DESKTOP_ICON_FILES))
>+	$(INSTALL) $^ $(DIST)/bin/chrome/icons/default
You dont need $(srcdir), see the equivalent xpfe lines you removed.

> ifneq (,$(filter gtk gtk2,$(MOZ_WIDGET_TOOLKIT)))
> 	$(INSTALL) $(srcdir)/icons/gtk/default.xpm $(DIST)/bin/chrome/icons/default
> 	$(INSTALL) $(srcdir)/icons/gtk/default16.xpm $(DIST)/bin/chrome/icons/default
> 	$(INSTALL) $(srcdir)/icons/gtk/seamonkey.png $(DIST)/bin/chrome/icons/default
> endif
Is it possible to add default to the list of gtk icons instead? You might want to break seamonkey.png out into its own rule so that you can use $^ too.

>+install:: $(addprefix $(topsrcdir)/icons/$(ICON_DIR)/,$(DESKTOP_ICON_FILES))
>+	$(SYSINSTALL) $(IFLAGS1) $^ $(DESTDIR)$(mozappdir)/chrome/icons/default
Out of interest, does seamonkey.png not need to be installed here?

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #6)
> (From update of attachment 227396 [details] [diff] [review] [edit])
> Please copy those icons instead of moving them, you should only cvs remove them
> in the old location once we are using the new ones and can be sure the old ones
> are unused.

Ok.

(In reply to comment #8)
> >+install:: $(addprefix $(topsrcdir)/icons/$(ICON_DIR)/,$(DESKTOP_ICON_FILES))
> >+	$(SYSINSTALL) $(IFLAGS1) $^ $(DESTDIR)$(mozappdir)/chrome/icons/default
> Out of interest, does seamonkey.png not need to be installed here?

Probably. I hadn't included it as it wasn't there before - I'll add it in.

Comment 10

[Mark Banner (:standard8)]

Attachment: Copies for binary files (checked in)

Made moves into copies, and removed the copying of the mozilla.ico.

Comment 11

[Mark Banner (:standard8)]

Attachment: Update build process for new binary file locations v2

Updated per Neil's comments. As we are copying the files and not moving them, then I am ifdefing out the relevant items in the xpfe Makefile for suite.

Comment 12

[[e-mail]]

Comment on attachment 227401 [details] [diff] [review]
Update build process for new binary file locations v2

> ifneq (,$(filter gtk gtk2,$(MOZ_WIDGET_TOOLKIT)))
>+DESKTOP_ICONS += default
Isn't this the same as ifeq ($(ICON_DIR),gtk)? In which case, you can write a literal gtk in place of $(ICON_DIR) in the two make rules. I'd also move the DESKTOP_ICONS line to just before the DESKTOP_ICONS_SMALL line.

Comment 13

[Robert Kaiser]

Comment on attachment 227400 [details] [diff] [review]
Copies for binary files (checked in)

Looks good to me now.

Comment 14

[Robert Kaiser]

If you'll do the text files as well, you probably can take care of bug 337920 at the same time...

Comment 15

[Mark Banner (:standard8)]

Attachment: Update build process for new binary file locations v3 (checked in)

Fixed Neil's comment. Carrying his r forward. Requesting sr.

Comment 16

[Robert Kaiser]

Comment on attachment 227424 [details] [diff] [review]
Update build process for new binary file locations v3 (checked in)

>Index: xpfe/bootstrap/splashos2.rc
>===================================================================

You could leave this file unchanged as well, just like for Windows.


>Index: suite/branding/Makefile.in
>===================================================================
>+ifneq (,$(filter windows os2 gtk gtk2,$(MOZ_WIDGET_TOOLKIT)))

Maybe add a comment here that points to the fact that mac icons are handled in suite/app/ (as they're located in branding as well, just in case someone's looking for how all those icons are installed).

Comment 17

[Peter Weilbacher]

(In reply to comment #16)
> (From update of attachment 227424 [details] [diff] [review] [edit])
> >Index: xpfe/bootstrap/splashos2.rc
> >===================================================================
> 
> You could leave this file unchanged as well, just like for Windows.

Windows splash.rc already has the respective change but I can take care of that in bug 342693 afterwards. If you want to include that change here, "..\\branding\\icons\\os2\\seamonkey.ico" should be enough two dirs up is not necessary.

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #16)
> (From update of attachment 227424 [details] [diff] [review] [edit])
> >Index: xpfe/bootstrap/splashos2.rc
> >===================================================================
> 
> You could leave this file unchanged as well, just like for Windows.

(In reply to comment #17)
> Windows splash.rc already has the respective change but I can take care of that
> in bug 342693 afterwards.

KaiRo meant to leave the xpfe/bootstrap version not the suite/app version. I'm inclinde to agree as the existing icon is staying there for the time being as well. So suite/app/splashos2.rc will need changing once its been moved.

Comment 19

[Mark Banner (:standard8)]

Attachment: Required cvs copies for text files

Note, I'll strip off the cp and mv before attaching this to a cvs copy bug once I've got approvals. Most of these are copies, its just the last one is a move which is why I left the extra bit on for now. version.txt will be handled in its own patch as I want to get benjamin's approval for that.

Comment 20

[Robert Kaiser]

Comment on attachment 227530 [details]
Required cvs copies for text files

Sounds good to me

Comment 21

[Mark Banner (:standard8)]

Attachment: Required changes after copy of text files.

These are the changes required after the cvs copies have been performed. I'm still testing them at the moment but its here for reference.

Comment 22

[Robert Kaiser]

Comment on attachment 227537 [details] [diff] [review]
Required changes after copy of text files.

>+# Now copy the current locale files to the correct location
>+libs:: $(addprefix $(AB_CD)/profile/,$(FILES))
>+	$(INSTALL) $^ $(DIST)/bin/defaults/profile/$(AB_CD)

I think this (and all similar blocks) does not work like you'd expect with files from the L10n repository. See browser/locales/Makefile.in for how FF does it to work correctly with that model.

Comment 23

[[e-mail]]

Comment on attachment 227537 [details] [diff] [review]
Required changes after copy of text files.

Presumably there are changes to xpfe/bootstrap ?

Comment 24

[[e-mail]]

Comment on attachment 227537 [details] [diff] [review]
Required changes after copy of text files.

Oh, I get half of it now - suite/app wants to pick up local copies of the files.

The profile files were simply not getting created?

Comment 25

[Mark Banner (:standard8)]

(In reply to comment #23)
> (From update of attachment 227537 [details] [diff] [review] [edit])
> Presumably there are changes to xpfe/bootstrap ?
> 
Nope. We're copying not moving these ones.

(In reply to comment #24)
> Oh, I get half of it now - suite/app wants to pick up local copies of the
> files.
Yep.

> The profile files were simply not getting created?
Wrong - but that's because my patch is missing a bit ;-) The profile files were being created via suite/app/profile/Makefile.in but now that's moving for the locales. I forgot to include the removal of that makefile (and hence folder) in the patch.

Comment 26

[Mark Banner (:standard8)]

Attachment: Moves xpfe/bootstrap/version.txt to suite/config/version.txt (checked in)

This patch does what it says in the description ;-) As version.txt is "ours" we need to move it into the suite structure. So this follows the ff/tb/sb convention and updates the references to it.

Comment 27

[Robert Kaiser]

Comment on attachment 227590 [details] [diff] [review]
Moves xpfe/bootstrap/version.txt to suite/config/version.txt (checked in)

>Index: xpinstall/packager/StageUtils.pm
>===================================================================
>-  $versionMilestone = `cat $aDirMozTopSrc/xpfe/bootstrap/version.txt`;
>+  $versionMilestone = `cat $aDirMozTopSrc/suite/config/version.txt`;

What is this for? It may be better to pull in MOZ_APP_VERSION here than to reference the files directly...

Comment 28

[Mark Banner (:standard8)]

(In reply to comment #27)
> (From update of attachment 227590 [details] [diff] [review] [edit])
> >Index: xpinstall/packager/StageUtils.pm
> >===================================================================
> >-  $versionMilestone = `cat $aDirMozTopSrc/xpfe/bootstrap/version.txt`;
> >+  $versionMilestone = `cat $aDirMozTopSrc/suite/config/version.txt`;
> 
> What is this for? It may be better to pull in MOZ_APP_VERSION here than to
> reference the files directly...
> 
This appears to be used at various places throughout the installers, e.g. for Gre versions and other app versions - though I don't think its actually used on Linux. So I guess it may be used on Windows or Mac. 

I don't think we can use MOZ_APP_VERSION because this is a helper file which doesn't get generated and we'd have to feed it in through quite a few places from what I can tell.

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 227400 [details] [diff] [review]
Copies for binary files (checked in)

I've just done a subsequent change (thanks for pointing it out CTho) which sets the cvs handling mode for the os2 and windows icons to binary - this should fix any problems with those icons displaying. The linux one's aren't necessary as they are text based.

Comment 30

[Mark Banner (:standard8)]

Attachment: Required changes after copy of text files v2 (checked in)

Just realised I hadn't provided an updated version of this ;-)

This addresses Kairo's comments about the profile files and making them being generated in a similar way to ff.

Comment 31

[Robert Kaiser]

Comment on attachment 229261 [details] [diff] [review]
Required changes after copy of text files v2 (checked in)

Looks better now, still have to test it before giving an OK though :)

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 227590 [details] [diff] [review]
Moves xpfe/bootstrap/version.txt to suite/config/version.txt (checked in)

Benjamin, can you review this from a build config perspective please?

Comment 33

[Robert Kaiser]

Comment on attachment 229261 [details] [diff] [review]
Required changes after copy of text files v2 (checked in)

looks good, builds and works :)

Comment 34

[Mark Banner (:standard8)]

Everything is now checked in for this bug -> fixed.