686466 - Get rid of application.ini in non-xulrunner applications

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mike Hommey [:glandium]]

This is especially important on android where we need to unpack the file for the XRE initialization code to read it. Instead of using an application.ini file, we can hardcode the application data in the binary itself.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Get rid of application.ini on Android

This only covers the Android part, in an ugly way

Comment 2

[Mike Hommey [:glandium]]

Attachment: part 1 - Centralize application.ini creation

Comment 3

[Mike Hommey [:glandium]]

Attachment: part 2 - Use centralized application.ini creation for mobile

Comment 4

[Mike Hommey [:glandium]]

Attachment: part 3 - Use a pre-generated nsXREAppData struct instead of application.ini

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 561189 [details] [diff] [review]
part 1 - Centralize application.ini creation

Review of attachment 561189 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/Makefile.in
@@ +79,5 @@
> +DEFINES += -DGRE_MILESTONE=$(GRE_MILESTONE) -DAPP_BUILDID=$(APP_BUILDID)
> +
> +DEFINES += -DMOZ_APP_VERSION="$(MOZ_APP_VERSION)"
> +
> +MOZ_SOURCE_STAMP := $(firstword $(shell hg -R $(topsrcdir) parent --template="{node|short}\n" 2>/dev/null))

You lost a ?= here, we run this at the top-level as well and export it.

@@ +91,5 @@
> +DEFINES += -DMOZ_SOURCE_REPO="$(SOURCE_REPO)"
> +endif
> +
> +DEFINES += -DMOZ_APP_BASENAME="$(MOZ_APP_BASENAME)" \
> +           -DMOZ_APP_VENDOR="$(MOZ_APP_VENDOR)"

Reformat this to be two-space indent and everything on its own line while you're here?

::: browser/app/application.ini
@@ +1,1 @@
> +#if 0

Hah!

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 561190 [details] [diff] [review]
part 2 - Use centralized application.ini creation for mobile

Review of attachment 561190 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/app/Makefile.in
@@ +84,3 @@
>  APP_ICON = mobile
>  
> +DEFINES += -DAPP_NAME=$(MOZ_APP_NAME) \

Change formatting to two-space indent + one entry per line while you're here?

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 561191 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini

Review of attachment 561191 [details] [diff] [review]:
-----------------------------------------------------------------

I ran out of time to give this a thorough review, but we can't stop shipping application.ini without breaking a bunch of automation at the moment.

::: browser/app/nsBrowserApp.cpp
@@ +176,5 @@
>    }
>  
>    nsXREAppData *appData;
> +  int result;
> +  if (appini) {

Do we preserve the mtime of application.ini when we package it? If so, could we bake the mtime into the header file and only read the file if it's newer?

::: build/Makefile.in
@@ +71,2 @@
>  DIST_FILES = application.ini
> +endif

I'm pretty sure not shipping application.ini will break some things. I think we need to continue to ship it even if we're not reading it.

It would probably be confusing to ship it and not honor changes to it, though, hence my comment above.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

talos depends on this for buildid and sourcestamp.  Should we not report these values?

Comment 9

[Jeff Hammel]

QA mozmill efforts also depend on application.ini

Comment 10

[Mike Hommey [:glandium]]

Do they all rely on application.ini even on mobile ?

Comment 11

[Jeff Hammel]

(In reply to Mike Hommey [:glandium] from comment #10)
> Do they all rely on application.ini even on mobile ?

For QA automation, I don't think so, but I'm also not in QA so I can't really speak for them

Comment 12

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #7)
> Comment on attachment 561191 [details] [diff] [review] [diff] [details] [review]
> part 3 - Use a pre-generated nsXREAppData struct instead of application.ini
> 
> Review of attachment 561191 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I ran out of time to give this a thorough review, but we can't stop shipping
> application.ini without breaking a bunch of automation at the moment.
> 
> ::: browser/app/nsBrowserApp.cpp
> @@ +176,5 @@
> >    }
> >  
> >    nsXREAppData *appData;
> > +  int result;
> > +  if (appini) {
> 
> Do we preserve the mtime of application.ini when we package it? If so, could
> we bake the mtime into the header file and only read the file if it's newer?

More importantly, what happens to mtime when we install and upgrade it?
On Linux, tar will preserve it. On Mac, what is in the dmg is what is seen. On Windows, it would depend on either zip extraction tools or the installer itself.
For upgrades, it depends on our updater, if it preserves mtime or not.

Comment 13

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #7)
> It would probably be confusing to ship it and not honor changes to it,
> though, hence my comment above.

How about a big warning at the beginning of the file, like:
# This file is not used. If you modify it and want the application to
# use your modifications, start with the "-app /path/to/application.ini"
# argument.

Comment 14

[Mike Hommey [:glandium]]

The following commit on birch clashes with part 1 here.
http://hg.mozilla.org/projects/birch/rev/76e6b7b29f81

Now, my question is, as the serverURL for crash reporter does use an id=uuid argument, without {}, how are email-like appids supposed to send their crash reports, if we ever have some?
Related question: the mobile application.ini doesn't give any arguments to the crash reporter serverURL. Would that work on all platforms?

Comment 15

[(not currently active) Ted Mielczarek]

The ServerURL bits are purely for stats, they're ignored by the server. They're just there so we can do data mining on the logs.

Your suggestion for a warning in the file sounds fine.

Comment 16

[Mike Hommey [:glandium]]

Attachment: part 2 - Use centralized application.ini creation for mobile

Comment 17

[Mike Hommey [:glandium]]

Attachment: part 3 - Use a pre-generated nsXREAppData struct instead of application.ini

Comment 18

[Mike Hommey [:glandium]]

Attachment: part 1 - Centralize application.ini creation

Comment 19

[Mike Hommey [:glandium]]

Attachment: part 3 - Use a pre-generated nsXREAppData struct instead of application.ini

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 569934 [details] [diff] [review]
part 1 - Centralize application.ini creation

Review of attachment 569934 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/Makefile.in
@@ +70,5 @@
> +
> +ifdef LIBXUL_SDK
> +GRE_MILESTONE = $(shell $(PYTHON) $(topsrcdir)/config/printconfigsetting.py $(LIBXUL_DIST)/bin/platform.ini Build Milestone)
> +else
> +GRE_MILESTONE = $(shell tail -n 1 $(topsrcdir)/config/milestone.txt 2>/dev/null || tail -1 $(topsrcdir)/config/milestone.txt)

Should these become := while you're here?

@@ +103,5 @@
> +ifdef MOZILLA_OFFICIAL
> +DEFINES += -DMOZILLA_OFFICIAL
> +endif
> +
> +DEFINES += -DMOZ_APP_ID="$(MOZ_APP_ID)"

Move this up to the other DEFINES block?

Comment 21

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #20)
> Comment on attachment 569934 [details] [diff] [review] [diff] [details] [review]
> part 1 - Centralize application.ini creation
> 
> Review of attachment 569934 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: build/Makefile.in
> @@ +70,5 @@
> > +
> > +ifdef LIBXUL_SDK
> > +GRE_MILESTONE = $(shell $(PYTHON) $(topsrcdir)/config/printconfigsetting.py $(LIBXUL_DIST)/bin/platform.ini Build Milestone)
> > +else
> > +GRE_MILESTONE = $(shell tail -n 1 $(topsrcdir)/config/milestone.txt 2>/dev/null || tail -1 $(topsrcdir)/config/milestone.txt)
> 
> Should these become := while you're here?

I thought it was better to avoid running these when we don't need the value.

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 569968 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini

Review of attachment 569968 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Just one thing that needs fixed, and a couple of suggestions.

::: build/appini_header.py
@@ +43,5 @@
> +
> +def main(file):
> +    config = ConfigParser.RawConfigParser()
> +    config.read(file)
> +    options = []

Feels like maybe you want to use a set here? Should work exactly the same, except you use options.add('FOO').

@@ +54,5 @@
> +            options += ['NS_XRE_ENABLE_PROFILE_MIGRATOR']
> +    except: pass
> +    try:
> +        if config.getint('Crash Reporter', 'Enabled') == 1:
> +            options += ['NS_XRE_ENABLE_PROFILE_MIGRATOR']

Copy/paste error here, you used NS_XRE_ENABLE_PROFILE_MIGRATOR again.

@@ +59,5 @@
> +    except: pass
> +    if not options:
> +        options = [ '0' ]
> +    print '#include "nsXULAppAPI.h"'
> +    print 'static inline nsXREAppData *getAppData(nsILocalFile *dir = NULL) {'

nsXULAppApi says the directory field can be NULL if the xre and app are in the same dir. Can you make use of that fact and just declare a static singleton struct in this header instead of having this getter method?

@@ +61,5 @@
> +        options = [ '0' ]
> +    print '#include "nsXULAppAPI.h"'
> +    print 'static inline nsXREAppData *getAppData(nsILocalFile *dir = NULL) {'
> +    print '    static nsXREAppData appData = {'
> +    print '        sizeof(nsXREAppData),'

This whole thing feels like it might look nicer in a triple-quoted multiline string.

@@ +77,5 @@
> +    print '        "%s",' % config.get('Crash Reporter', 'ServerURL')
> +    try:
> +        print '        "%s"' % config.get('App', 'Profile')
> +    except:
> +        print '        NULL'

You could write this as:
"%s" % config.get('App', 'Profile') if config.has_option('App', 'Profile') else 'NULL'

@@ +83,5 @@
> +    print '    return &appData;'
> +    print '}'
> +
> +if __name__ == '__main__':
> +    main(sys.argv[1:])

You might want to explicitly error here if len(sys.argv) < 2.

Comment 23

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #22)
> nsXULAppApi says the directory field can be NULL if the xre and app are in
> the same dir. Can you make use of that fact and just declare a static
> singleton struct in this header instead of having this getter method?

In practice, that is not true. Maybe we could fix that, though.

Comment 24

[Mike Hommey [:glandium]]

Attachment: part 3 - Use a pre-generated nsXREAppData struct instead of application.ini

How about this?

Note that for appdata construction, i was originally using:
"appdata = { "%s:%s" % (s, o) : config.get(s, o) for s in config.sections() for o in config.options(s) }"

but python 2.6 chokes on it with a syntax error (while it works with 2.7).

Comment 25

[Mike Hommey [:glandium]]

Attachment: part 3 - Use a pre-generated nsXREAppData struct instead of application.ini

Forgot to hg qref the final iteration.

Comment 26

[(not currently active) Ted Mielczarek]

Right, that's a dict comprehension, which got added in Python 3.0 and 2.7. You can achieve the same effect by writing:
appdata = dict(("%s:%s" % (s, o), config.get(s, o) for s in config.sections() for o in config.options(s))
in earlier Python.

Comment 27

[Mike Hommey [:glandium]]

Attachment: part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

Comment 28

[(not currently active) Ted Mielczarek]

Comment on attachment 573485 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

Review of attachment 573485 [details] [diff] [review]:
-----------------------------------------------------------------

I'm starting to wonder if we can't pull more of those ns*App source files into a common file somewhere. We've got a lot of repeated code there.

::: build/Makefile.in
@@ +153,5 @@
>  	chmod +x $@
>  GARBAGE += leaktest.py
>  
> +ifdef MOZ_APP_BASENAME
> +application.ini: application.ini.in $(APP_INI_DEPS)

We should probably define something in rules.mk that Makefiles can use to run files through the preprocessor, like PREPROCESSOR_FILES = foo.in, and it'd generate "foo" from that. (You don't have to implement that here, maybe we should just file a followup.)

@@ +160,5 @@
> +
> +ifdef MOZ_APP_STATIC_INI
> +application.ini.h: appini_header.py application.ini 
> +	$(PYTHON) $^ > $@
> +libs:: application.ini.h

Can we generate this in export? Feels more correct, since that's when we generate headers. (Probably doesn't matter much in practice here.)

::: build/appini_header.py
@@ +63,5 @@
> +
> +    print '''#include "nsXULAppAPI.h"
> +             static const nsXREAppData sAppData = {
> +                 sizeof(nsXREAppData),
> +                 NULL,

It might be worth adding C++ comments here for what the NULL fields are. (The ones with format strings are self-evident.)

@@ +82,5 @@
> +if __name__ == '__main__':
> +    if len(sys.argv) != 1:
> +        main(sys.argv[1])
> +    else:
> +        sys.stderr.write("Usage: %s /path/to/application.ini\n" % sys.argv[0]);

You can also use print >>sys.stderr, "foo", FYI. Doesn't matter that much to me.

Comment 29

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #28)
> I'm starting to wonder if we can't pull more of those ns*App source files
> into a common file somewhere. We've got a lot of repeated code there.

That's bug 658847.

Comment 30

[Mike Hommey [:glandium]]

Attachment: part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

Refreshed accordingly. I also renamed "options" to "flags", so as to fit the nsXREAppData field name.

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 573516 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

Carrying over r+

Comment 32

[Mike Hommey [:glandium]]

Attachment: part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

The change to make the appdata static made android fail because what XRE_main
guesses on Android was wrong. It worked in the previous iteration because we
were giving the right XRE path. This iteration makes it so that XRE_main itself
gets the right path on Android.

Sending this review to Benjamin instead of Ted because it touches more
toolkit/xre|xpcom stuff.

Comment 33

[Mike Hommey [:glandium]]

Comment on attachment 573762 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

Still fails on try, though it works locally :(

Comment 34

[Mike Hommey [:glandium]]

Attachment: part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

This time, it works properly

https://tbpl.mozilla.org/?tree=Try&rev=6c56c8d9fa19

Comment 35

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/346488b20c22
https://hg.mozilla.org/integration/mozilla-inbound/rev/234baeb36628
https://hg.mozilla.org/integration/mozilla-inbound/rev/696df30f6a51

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/346488b20c22
https://hg.mozilla.org/mozilla-central/rev/234baeb36628
https://hg.mozilla.org/mozilla-central/rev/696df30f6a51

Comment 38

[Mike Kaply [:mkaply]]

Apologize for jumping in here, but I don't see any mention.

Some of the values in application.ini need to be set by a client on Firefox, particularly in enterprise environments.

In particular, being able to turn off the profile migrator:

EnableProfileMigrator=1

And being able to turn off crash reporting (and maybe change the reporting URL).

[Crash Reporter]
Enabled=1
ServerURL=https://crash-reports.mozilla.com/submit?id=ec8030f7-c20a-464f-9b0e-13a3a9e97384&version=10.0.1&buildid=20120208060813

Are these now hardcoded into the executable?

Comment 39

[Mike Hommey [:glandium]]

(In reply to Michael Kaply (mkaply) from comment #38)
> Are these now hardcoded into the executable?

Yes, they are. Modifying application.ini was a broken way to disable these, anyways. Crash reporter can be disabled with an environment variable. I don't think there is something easy to disable the profile migrator (and i don't see why one would want that), but if you need one, you can file a new bug.

Comment 40

[Mike Kaply [:mkaply]]

> I don't think there is something easy to disable the profile migrator (and i don't see why one would want that)

See: https://bugzilla.mozilla.org/show_bug.cgi?id=286557

I'm the one that put that EnableProfileMigrator code there in the first place

Comment 41

[Ben Bucksch (:BenB)]

Mike Hommey, you didn't hard code the crash reporter URL in C++, did you? Please tell me that you didn't hardcode a URL in C++.

I concur that the settings Mike mentioned must be configurable by an end-user, preferably in prefs.

Comment 42

[(not currently active) Ted Mielczarek]

Yes, we currently compile all the information from application.ini into the Firefox binary. I don't think we're going to change this decision.

Comment 43

[Ben Bucksch (:BenB)]

Sorry, but mkaply's right. This has to be configurable. It doesn't matter what your opinion is, this is important for many usecases. Both migration and crash stuff must be optional.

And NEVER EVER EVER hardcode URLs in C++.

Comment 44

[Ben Bucksch (:BenB)]

Followup: bug 727122

Comment 45

[(not currently active) Ted Mielczarek]

If you want it to be optional, you can provide your own builds. These are Mozilla builds, and we determine the feature set that ships with them.

Hardcoding the server URL is the only thing that makes sense. Nobody else has the debug symbols for our builds, so it doesn't make any sense for them to receive crash reports from our builds.

Comment 46

[Ben Bucksch (:BenB)]

> These are Mozilla builds, and we determine the feature set that ships with them.

No, sir, your ruling does not stand for 400 million people, without any possibility to be challenged.

The question wasn't just about the URL, but the ability to disable the crash reporter and the profile migration.

Comment 47

[(not currently active) Ted Mielczarek]

If you have a problem with the concept of module ownership, I suggest you take that up elsewhere.

Comment 48

[Ben Bucksch (:BenB)]

I have a problem with module owners who are not open to reasonable arguments.

Comment 49

[Don Kleppinger]

Please look at bug 723493.  This broke our enterprise web app and is preventing us from upgrading to firefox 11.  I need to be able to configure a custom vendor & name in Application.ini without having to add a command line parameter to the firefox startup.