The default bug view has changed. See this FAQ.

Preprocessor.py: Make filters usable in defines

RESOLVED FIXED in mozilla15

Status

()

Core
Build Config
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: mstange, Assigned: glandium)

Tracking

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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?
Attachment #393091 - Flags: review?(l10n)

Comment 1

8 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

8 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

8 years ago
Assignee: mstange → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 3

5 years ago
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
Attachment #617450 - Flags: review?(ted.mielczarek)
Attachment #617450 - Flags: review?(community)
(Assignee)

Updated

5 years ago
Attachment #617450 - Flags: review?(community) → review?(l10n)
(Assignee)

Comment 4

5 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

5 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

5 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

5 years ago
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
Attachment #617480 - Flags: review?(ted.mielczarek)
Attachment #617480 - Flags: review?(l10n)
(Assignee)

Updated

5 years ago
Attachment #617450 - Attachment is obsolete: true
Attachment #617450 - Flags: review?(ted.mielczarek)
Attachment #617450 - Flags: review?(l10n)
(Assignee)

Comment 8

5 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

5 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

5 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+
Attachment #617480 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 11

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Depends on: 751865
You need to log in before you can comment on or make changes to this bug.