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

RESOLVED FIXED in mozilla17

Status

()

Core
Build Config
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: glandium)

Tracking

unspecified
mozilla17
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 636199 [details] [diff] [review]
Proposed patch
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #636199 - Flags: review?(khuey)
(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...
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 on attachment 636199 [details] [diff] [review]
Proposed patch

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

r=me assuming this produces the same output.
Attachment #636199 - Flags: review?(khuey) → review+
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.
Attachment #636199 - Flags: review+ → review-
(Reporter)

Comment 6

5 years ago
Created attachment 638438 [details] [diff] [review]
Just update-settings.ini
Attachment #638438 - Flags: review?(khuey)
Depends on: 770426
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.
Attachment #638438 - Flags: review?(khuey) → review+
(Reporter)

Comment 8

5 years ago
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 :-(
(Reporter)

Comment 9

5 years ago
(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.
(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.
(Reporter)

Comment 11

5 years ago
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.
(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.
Whiteboard: [leave open]
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)
Attachment #638623 - Flags: review?(khuey)
Attachment #638623 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/d8a4ef92f06e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4d94efad3e
Assignee: neil → mh+mozilla
Whiteboard: [leave open]
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/8e4d94efad3e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.