Closed Bug 508942 Opened 15 years ago Closed 12 years ago

Preprocessor.py: Make filters usable in defines

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla15

People

(Reporter: mstange, Assigned: glandium)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch v1Splinter Review
In bug 508940 I'd like to do things like this:

shared.inc:

%filter substitution

%define SCOPE_BAR_TOP_COLOR #E8E8E8
%define SCOPE_BAR_BOTTOM_COLOR #D0D0D0
%define SCOPE_BAR_BACKGROUND -moz-linear-gradient(top, bottom, from(@SCOPE_BAR_TOP_COLOR@), to(@SCOPE_BAR_BOTTOM_COLOR@)) repeat-x

findBar.css:

%include shared.inc

findbar {
  background: @SCOPE_BAR_BACKGROUND@;
}

in order to minimize code repetition. The existing cascading mechanisms of CSS can't satisfy our needs, it seems :(

However, if I read the code in Preprocessor.py correctly, there's currently no way to reuse a #defined value when #defining a new value.

I'm attaching a patch that fixes that by applying filters to the #defined values at the time they're defined. Does that sound like a sane solution to the problem?
Attachment #393091 - Flags: review?(l10n)
I'm a bit concerned about regular values that have @ signs in them, like emails, contract id, @media rules. varsubst only matches non-whitespace, so this is probably OK in general, though.

Can you add some tests that make sure that we're not regressing some of those?

And this should get some testing, I guess we need to at least some fx builds with and without this patch and confirm that they're binary equal right now, same for probably fennec and thunderbird. Just running the build after the patch doesn't seem paranoid enough to me.

From a patch point-of-view, this looks OK, maybe more tests. I'd consider this a patch to need sr, though I don't have a suggestion on a good candidate.
Comment on attachment 393091 [details] [diff] [review]
v1

Testing builds for binary equality will take me a while, so I'm canceling the review request until I've done that. Do you know what would be the fastest way to do it? I suppose I can't just do two try server pushes, since I suspect that tryserver somehow encodes the build time into the builds... is that correct?

I'm not so very concerned about this breaking anything. The "attemptSubstitution" filter doesn't seem to be used anywhere: http://mxr.mozilla.org/comm-central/search?string=attemptSubstitution&tree=comm-central
And the "substitution" filter will cause the build to fail if it encounters an undefined variable.
Attachment #393091 - Flags: review?(l10n)
Assignee: mstange → nobody
Status: ASSIGNED → NEW
I'm testing this patch on Try, with the following additional change:
https://hg.mozilla.org/try/rev/9029c0c2ca22

This effectively compares the output with the patch applied to the
output without the patch applied, and will fail the build when they
don't match.

TBPL link for that try: https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=9029c0c2ca22
Attachment #617450 - Flags: review?(ted.mielczarek)
Attachment #617450 - Flags: review?(community)
Attachment #617450 - Flags: review?(community) → review?(l10n)
New attempt with more check glue (reading sys.stdin twice doesn't work)
https://hg.mozilla.org/try/rev/d5b76b08a1d4
https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=d5b76b08a1d4
And yet another try (sorry for the noise). There was a problem with includesubst in jar.mn, because JarMaker.py doesn't enable the substitution filter.

https://hg.mozilla.org/try/rev/4bc0ce7f082b
https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=4bc0ce7f082b
Attachment #617480 - Flags: review?(ted.mielczarek)
Attachment #617480 - Flags: review?(l10n)
Attachment #617450 - Attachment is obsolete: true
Attachment #617450 - Flags: review?(ted.mielczarek)
Attachment #617450 - Flags: review?(l10n)
(In reply to Mike Hommey [:glandium] from comment #6)
> ... and avoid failing the testcase:
> https://hg.mozilla.org/try/rev/9c7a9058034b
> https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=9c7a9058034b

Finally all green.
(In reply to Mike Hommey [:glandium] from comment #8)
> (In reply to Mike Hommey [:glandium] from comment #6)
> > ... and avoid failing the testcase:
> > https://hg.mozilla.org/try/rev/9c7a9058034b
> > https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=9c7a9058034b
> 
> Finally all green.

Err wrong quote, was meant for comment #7.
Comment on attachment 617480 [details] [diff] [review]
Use Preprocessor.py filters in #defines and #includes

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

Looks OK to me.
Attachment #617480 - Flags: review?(l10n) → review+
Attachment #617480 - Flags: review?(ted.mielczarek) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d893b87af327
Assignee: nobody → mh+mozilla
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/d893b87af327
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 751865
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: