Last Comment Bug 508942 - Preprocessor.py: Make filters usable in defines
: Preprocessor.py: Make filters usable in defines
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 751865
Blocks: 508940
  Show dependency treegraph
 
Reported: 2009-08-06 18:12 PDT by Markus Stange [:mstange]
Modified: 2012-05-04 05:36 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (2.51 KB, patch)
2009-08-06 18:12 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
Use Preprocessor.py filters in #defines and #includes (7.28 KB, patch)
2012-04-23 06:02 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Use Preprocessor.py filters in #defines and #includes (6.18 KB, patch)
2012-04-23 07:58 PDT, Mike Hommey [:glandium]
ted: review+
l10n: review+
Details | Diff | Splinter Review

Description Markus Stange [:mstange] 2009-08-06 18:12:15 PDT
Created attachment 393091 [details] [diff] [review]
v1

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?
Comment 1 Axel Hecht [:Pike] 2009-08-06 23:33:58 PDT
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 2 Markus Stange [:mstange] 2009-08-17 21:29:54 PDT
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.
Comment 3 Mike Hommey [:glandium] 2012-04-23 06:02:49 PDT
Created attachment 617450 [details] [diff] [review]
Use Preprocessor.py filters in #defines and #includes

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
Comment 4 Mike Hommey [:glandium] 2012-04-23 06:49:39 PDT
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
Comment 5 Mike Hommey [:glandium] 2012-04-23 06:57:36 PDT
This is embarassing. Another try, with js/src/config in sync:
https://hg.mozilla.org/try/rev/4634e1c1f1d4
https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=4634e1c1f1d4
Comment 6 Mike Hommey [:glandium] 2012-04-23 07:44:06 PDT
... and avoid failing the testcase:
https://hg.mozilla.org/try/rev/9c7a9058034b
https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=9c7a9058034b
Comment 7 Mike Hommey [:glandium] 2012-04-23 07:58:13 PDT
Created attachment 617480 [details] [diff] [review]
Use Preprocessor.py filters in #defines and #includes

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
Comment 8 Mike Hommey [:glandium] 2012-04-24 00:39:27 PDT
(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.
Comment 9 Mike Hommey [:glandium] 2012-04-24 00:40:13 PDT
(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 10 Axel Hecht [:Pike] 2012-04-24 06:06:23 PDT
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.

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