Last Comment Bug 767839 - Don't preprocess application.ini and update-settings.ini twice
: Don't preprocess application.ini and update-settings.ini twice
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: x86_64 Linux
: -- enhancement (vote)
: mozilla17
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 770426
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-24 16:03 PDT by neil@parkwaycc.co.uk
Modified: 2012-08-08 09:35 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (4.34 KB, patch)
2012-06-24 16:04 PDT, neil@parkwaycc.co.uk
khuey: review-
Details | Diff | Splinter Review
Just update-settings.ini (1006 bytes, patch)
2012-07-02 11:52 PDT, neil@parkwaycc.co.uk
mh+mozilla: review+
Details | Diff | Splinter Review
Avoid preprocessing application.ini.in twice (1.58 KB, patch)
2012-07-03 01:49 PDT, Mike Hommey [:glandium]
khuey: review+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2012-06-24 16:03:05 PDT
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 neil@parkwaycc.co.uk 2012-06-24 16:04:47 PDT
Created attachment 636199 [details] [diff] [review]
Proposed patch
Comment 2 Mike Hommey [:glandium] 2012-06-24 23:11:27 PDT
(In reply to neil@parkwaycc.co.uk 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] 2012-06-24 23:22:38 PDT
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 [:khuey] (khuey@mozilla.com) 2012-07-02 09:59:58 PDT
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 [:khuey] (khuey@mozilla.com) 2012-07-02 10:10:13 PDT
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 neil@parkwaycc.co.uk 2012-07-02 11:52:34 PDT
Created attachment 638438 [details] [diff] [review]
Just update-settings.ini
Comment 7 Mike Hommey [:glandium] 2012-07-03 00:00:01 PDT
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 neil@parkwaycc.co.uk 2012-07-03 00:35:59 PDT
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 neil@parkwaycc.co.uk 2012-07-03 00:37:06 PDT
(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] 2012-07-03 01:16:11 PDT
(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 2012-07-03 01:39:09 PDT
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] 2012-07-03 01:44:15 PDT
(In reply to neil@parkwaycc.co.uk 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] 2012-07-03 01:49:12 PDT
Created attachment 638623 [details] [diff] [review]
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] 2012-07-03 16:08:50 PDT
https://hg.mozilla.org/mozilla-central/rev/d8a4ef92f06e
Comment 16 Ed Morley [:emorley] 2012-08-08 09:35:49 PDT
https://hg.mozilla.org/mozilla-central/rev/8e4d94efad3e

Note You need to log in before you can comment on or make changes to this bug.