Closed
Bug 508942
Opened 15 years ago
Closed 12 years ago
Preprocessor.py: Make filters usable in defines
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla15
People
(Reporter: mstange, Assigned: glandium)
References
Details
Attachments
(2 files, 1 obsolete file)
2.51 KB,
patch
|
Details | Diff | Splinter Review | |
6.18 KB,
patch
|
ted
:
review+
Pike
:
review+
|
Details | Diff | Splinter 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)
Comment 1•15 years ago
|
||
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.
Reporter | ||
Comment 2•15 years ago
|
||
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)
Reporter | ||
Updated•15 years ago
|
Assignee: mstange → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 3•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #617450 -
Flags: review?(community) → review?(l10n)
Assignee | ||
Comment 4•12 years ago
|
||
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
Assignee | ||
Comment 5•12 years ago
|
||
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
Assignee | ||
Comment 6•12 years ago
|
||
... and avoid failing the testcase: https://hg.mozilla.org/try/rev/9c7a9058034b https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=9c7a9058034b
Assignee | ||
Comment 7•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #617450 -
Attachment is obsolete: true
Attachment #617450 -
Flags: review?(ted.mielczarek)
Attachment #617450 -
Flags: review?(l10n)
Assignee | ||
Comment 8•12 years ago
|
||
(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.
Assignee | ||
Comment 9•12 years ago
|
||
(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•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #617480 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d893b87af327
Assignee: nobody → mh+mozilla
Target Milestone: --- → mozilla15
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d893b87af327
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•