Closed Bug 686466 Opened 8 years ago Closed 8 years ago

Get rid of application.ini in non-xulrunner applications

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mobilestartupshrink][inbound])

Attachments

(3 files, 11 obsolete files)

5.57 KB, patch
ted
: review+
Details | Diff | Splinter Review
10.60 KB, patch
ted
: review+
Details | Diff | Splinter Review
23.49 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
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.
Blocks: 686480
This only covers the Android part, in an ugly way
Whiteboard: [mobilestartupshrink]
Depends on: 687805
Attachment #561189 - Flags: review?(ted.mielczarek)
Attachment #559982 - Attachment is obsolete: true
Attachment #561191 - Flags: review?(benjamin)
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!
Attachment #561189 - Flags: review?(ted.mielczarek) → review+
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?
Attachment #561190 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #561191 - Flags: review?(ted.mielczarek) → review-
talos depends on this for buildid and sourcestamp.  Should we not report these values?
QA mozmill efforts also depend on application.ini
Do they all rely on application.ini even on mobile ?
(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
Attachment #561191 - Flags: review?(benjamin)
Blocks: 695699
(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.
(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.
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?
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.
Blocks: 695145
Attachment #561190 - Attachment is obsolete: true
Attachment #561191 - Attachment is obsolete: true
Attachment #561189 - Attachment is obsolete: true
Attachment #569933 - Attachment is obsolete: true
Attachment #569933 - Flags: review?(ted.mielczarek)
Depends on: 698493
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?
Attachment #569934 - Flags: review?(ted.mielczarek) → review+
Attachment #569932 - Flags: review?(ted.mielczarek) → review+
(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.
Target Milestone: --- → mozilla9
Version: Trunk → Other Branch
Target Milestone: mozilla9 → ---
Version: Other Branch → Trunk
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.
Attachment #569968 - Flags: review?(ted.mielczarek) → review+
(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.
Target Milestone: --- → mozilla9
Version: Trunk → 11 Branch
Target Milestone: mozilla9 → ---
Version: 11 Branch → Trunk
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).
Attachment #573464 - Flags: review?(ted.mielczarek)
Attachment #569968 - Attachment is obsolete: true
Forgot to hg qref the final iteration.
Attachment #573473 - Flags: review?(ted.mielczarek)
Attachment #573464 - Attachment is obsolete: true
Attachment #573464 - Flags: review?(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.
Attachment #573473 - Attachment is obsolete: true
Attachment #573473 - Flags: review?(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.
Attachment #573485 - Flags: review?(ted.mielczarek) → review+
(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.
Refreshed accordingly. I also renamed "options" to "flags", so as to fit the nsXREAppData field name.
Attachment #573485 - Attachment is obsolete: true
Comment on attachment 573516 [details] [diff] [review]
part 3 - Use a pre-generated nsXREAppData struct instead of application.ini.

Carrying over r+
Attachment #573516 - Flags: review+
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.
Attachment #573762 - Flags: review?(benjamin)
Attachment #573516 - Attachment is obsolete: true
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 :(
Attachment #573762 - Flags: review?(benjamin)
Attachment #573762 - Attachment is obsolete: true
Attachment #574890 - Flags: review?(benjamin) → review+
Duplicate of this bug: 704230
Blocks: 726978
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?
(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.
> 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
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.
Depends on: 727122
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.
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++.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Followup: bug 727122
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
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.
> 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.
If you have a problem with the concept of module ownership, I suggest you take that up elsewhere.
I have a problem with module owners who are not open to reasonable arguments.
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.
Depends on: 822646
Depends on: 933803
No longer depends on: 933803
You need to log in before you can comment on or make changes to this bug.