Bug 140756 - makeprop.pl script is called too many times during build
Summary: makeprop.pl script is called too many times during build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-01-21 20:41 PST by Chris Dumez
Modified: 2015-01-22 11:38 PST (History)
3 users (show)

See Also:


Attachments
Patch (2.93 KB, patch)
2015-01-21 21:04 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.69 KB, patch)
2015-01-21 22:58 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-01-21 20:41:12 PST
makeprop.pl script is called 4 times (instead of once) during build due to the following Makefile rule:

CSSPropertyNames.h StyleBuilder.cpp StylePropertyShorthandFunctions.h StylePropertyShorthandFunctions.cpp : $(WEBCORE_CSS_PROPERTY_NAMES) css/makeprop.pl bindings/scripts/preprocessor.pm $(PLATFORM_FEATURE_DEFINES)
        $(PERL) -pe '' $(WEBCORE_CSS_PROPERTY_NAMES) > CSSPropertyNames.in
        $(PERL) -I$(WebCore)/bindings/scripts "$(WebCore)/css/makeprop.pl" --defines "$(FEATURE_DEFINES)"

Having 4 targets here is wrong as makeprop.pl only needs to be called once to generate these 4 files. Currently, we are calling makeprop.pl once per target and therefore generating all these files 4 times.

May be related to <rdar://problem/19467942>.
Comment 1 Chris Dumez 2015-01-21 20:49:50 PST
I am thinking this may be the source of the following build error we get sometimes:
CSSPropertyNames.gperf: No keywords in input file!
calling gperf failed: 256 at WebCore/css/makeprop.pl line 1036.

In particular, since WebCore/css/makeprop.pl is executed 4 times, I am worried that the script may be executed several times in parallel, which would explain why gperf is complaining (because another instance of makeprop.pl may be writing to gperf's its input file). makeprop.pl first generates gperf's input file then calls gperf on it at the end.
Comment 2 Chris Dumez 2015-01-21 21:04:47 PST
Created attachment 245117 [details]
Patch
Comment 3 David Kilzer (:ddkilzer) 2015-01-21 21:59:27 PST
Comment on attachment 245117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245117&action=review

> Source/WebCore/DerivedSources.make:820
> +CSSPropertyNames.h : $(WEBCORE_CSS_PROPERTY_NAMES) css/makeprop.pl bindings/scripts/preprocessor.pm $(PLATFORM_FEATURE_DEFINES)

This removes the dependencies on the other three files.  I think you need some dummy rules to make sure any one missing file causes them all to be regenerated per:
http://stackoverflow.com/questions/2973445/gnu-makefile-rule-generating-a-few-targets-from-a-single-source-file
Comment 4 Chris Dumez 2015-01-21 22:10:36 PST
Comment on attachment 245117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245117&action=review

>> Source/WebCore/DerivedSources.make:820
>> +CSSPropertyNames.h : $(WEBCORE_CSS_PROPERTY_NAMES) css/makeprop.pl bindings/scripts/preprocessor.pm $(PLATFORM_FEATURE_DEFINES)
> 
> This removes the dependencies on the other three files.  I think you need some dummy rules to make sure any one missing file causes them all to be regenerated per:
> http://stackoverflow.com/questions/2973445/gnu-makefile-rule-generating-a-few-targets-from-a-single-source-file

If this is an issue, this is a pre-existing one. For example, notice that CSSPropertyNames.cpp is not mentioned anywhere in this file even though it is also generated by makeprop.pl script as well. We never had any issue with CSSPropertyNames.cpp AFAIK. However, right now, makeprop.pl is called 4 times for no reason.

Similarly, CSSValueKeywords.cpp is generated by makevalues.pl a few lines below but only CSSValueKeywords.h is mentioned in this file.
Comment 5 Chris Dumez 2015-01-21 22:24:12 PST
Comment on attachment 245117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245117&action=review

>>> Source/WebCore/DerivedSources.make:820
>>> +CSSPropertyNames.h : $(WEBCORE_CSS_PROPERTY_NAMES) css/makeprop.pl bindings/scripts/preprocessor.pm $(PLATFORM_FEATURE_DEFINES)
>> 
>> This removes the dependencies on the other three files.  I think you need some dummy rules to make sure any one missing file causes them all to be regenerated per:
>> http://stackoverflow.com/questions/2973445/gnu-makefile-rule-generating-a-few-targets-from-a-single-source-file
> 
> If this is an issue, this is a pre-existing one. For example, notice that CSSPropertyNames.cpp is not mentioned anywhere in this file even though it is also generated by makeprop.pl script as well. We never had any issue with CSSPropertyNames.cpp AFAIK. However, right now, makeprop.pl is called 4 times for no reason.
> 
> Similarly, CSSValueKeywords.cpp is generated by makevalues.pl a few lines below but only CSSValueKeywords.h is mentioned in this file.

The stack overflow solution is not perfect either:
CSSPropertyNames.h : $(WEBCORE_CSS_PROPERTY_NAMES) css/makeprop.pl bindings/scripts/preprocessor.pm $(PLATFORM_FEATURE_DEFINES)
  makeprop.pl ...

StyleBuilder.cpp : CSSPropertyNames.h
  noop

If you delete StyleBuilder.cpp but CSSPropertyNames.h is still there, then it still won't regenerate StyleBuilder.cpp, will it? Or am I missing something.
Comment 6 Chris Dumez 2015-01-21 22:58:13 PST
Created attachment 245122 [details]
Patch
Comment 7 Chris Dumez 2015-01-21 23:01:04 PST
Comment on attachment 245122 [details]
Patch

Hopefully this addresses your comments. I have verified manually that if I delete CSSPropertyNames.cpp, makeprop.pl correctly gets called to regenerate it. makeprop.pl is also only called once.
Comment 8 Chris Dumez 2015-01-21 23:04:14 PST
Comment on attachment 245122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245122&action=review

> Source/WebCore/DerivedSources.make:826
> +.INTERMEDIATE : makeprop.intermediate

Doc: https://www.gnu.org/software/make/manual/html_node/Special-Targets.html

This was also one of the solutions proposed on the stack overflow thread you provided:
http://stackoverflow.com/a/10609434
Comment 9 David Kilzer (:ddkilzer) 2015-01-22 11:37:09 PST
Comment on attachment 245122 [details]
Patch

r=me!
Comment 10 Chris Dumez 2015-01-22 11:38:37 PST
Comment on attachment 245122 [details]
Patch

Clearing flags on attachment: 245122

Committed r178929: <http://trac.webkit.org/changeset/178929>
Comment 11 Chris Dumez 2015-01-22 11:38:41 PST
All reviewed patches have been landed.  Closing bug.