767839 - Don't preprocess application.ini and update-settings.ini twice

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[[e-mail]]

application.ini and update-settings.ini are preprocessed from .in files into the objdir/build and then again into objdir/dist/bin but this only needs to happen once.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Mike Hommey [:glandium]]

(In reply to [e-mail] from comment #1)
> Created attachment 636199 [details] [diff] [review]
> Proposed patch

The patch does much more than what comment 0 suggest should happen. BTW, I don't see application.ini and update-settings.ini preprocessed twice when building...

Comment 3

[Mike Hommey [:glandium]]

I'll add that the reason I added appini_header.py was to avoid application.ini and application.ini.h being out of sync, which would probably happen unnoticed if they were both preprocessed from a different source.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 636199 [details] [diff] [review]
Proposed patch

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

r=me assuming this produces the same output.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 636199 [details] [diff] [review]
Proposed patch

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

Actually, comment 3 is a pretty compelling reason not to do this.

Comment 6

[[e-mail]]

Attachment: Just update-settings.ini

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 638438 [details] [diff] [review]
Just update-settings.ini

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

This will work, although it would be better to wait for bug 770426 and do the same for application.ini.

Comment 8

[[e-mail]]

Comment on attachment 638438 [details] [diff] [review]
Just update-settings.ini

I pushed mozilla-inbound changeset d8a4ef92f06e, but I didn't read my bugmail carefully enough and I credited the wrong reviewer :-(

Comment 9

[[e-mail]]

(In reply to Mike Hommey from comment #7)
> This will work, although it would be better to wait for bug 770426 and do
> the same for application.ini.
application.ini is a different kettle of fish because you want to preprocess it and then subsequently convert it to a .h file.

Comment 10

[Mike Hommey [:glandium]]

(In reply to [e-mail] from comment #9)
> (In reply to Mike Hommey from comment #7)
> > This will work, although it would be better to wait for bug 770426 and do
> > the same for application.ini.
> application.ini is a different kettle of fish because you want to preprocess
> it and then subsequently convert it to a .h file.

That's not a problem with proper dependencies.

Comment 11

[[e-mail]]

So you intend to run appini_header.py on the $(DIST)/application.ini instead?

On an unrelated note, appini_header.ini.h is already effectively preprocessed because it's used by compiled code, so I did wonder why we don't just add the relevant defines so that the C preprocessor would insert all the right data.

Comment 12

[Mike Hommey [:glandium]]

(In reply to [e-mail] from comment #11)
> So you intend to run appini_header.py on the $(DIST)/application.ini instead?
> 
> On an unrelated note, appini_header.ini.h is already effectively
> preprocessed because it's used by compiled code, so I did wonder why we
> don't just add the relevant defines so that the C preprocessor would insert
> all the right data.

Because it's included from several different places, that each would need the right defines, and that AC_DEFINE()ing them would basically make incremental builds almost clobber builds. In the end, it's simpler this way.

Comment 13

[Mike Hommey [:glandium]]

Attachment: Avoid preprocessing application.ini.in twice

With bug 770426, this works as intended.

The $(srcdir) is required because VPATH makes the previously preprocessed file from objdir being used instead on incremental builds, and since the build id needs to changes on every build, that would break this. (it was not important for update-settings.ini)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8a4ef92f06e

Comment 15

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4d94efad3e

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e4d94efad3e