Bug 166814 - REGRESSION(r178955): Touching Settings.in doesn't cause JSInternalSettingsGenerated.cpp to be updated on first build
Summary: REGRESSION(r178955): Touching Settings.in doesn't cause JSInternalSettingsGen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: http://stackoverflow.com/questions/29...
Keywords:
: 165220 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-01-07 19:46 PST by Myles C. Maxfield
Modified: 2017-02-08 17:59 PST (History)
6 users (show)

See Also:


Attachments
Patch (12.36 KB, patch)
2017-01-08 18:35 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 Myles C. Maxfield 2017-01-07 19:46:43 PST
To reproduce on an OpenSource build:

1. Perform a full, clean build
2. Add or remove an item from Settings.in
3. In a terminal:
$ cd ~/Build/Products/Debug/DerivedSources/WebCore
$ export WebCore="WebCore"
$ export JavaScriptCore_SCRIPTS_DIR="JavaScriptCorePrivateHeaders"
$ export CC="`xcrun -find clang`"
$ export GPERF="`xcrun -find gperf`"
$ export WEBKITADDITIONS_HEADER_SEARCH_PATHS="${HOME}/Build/Products/Debug/usr/local/include/WebKitAdditions /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/local/include/WebKitAdditions"
$ export MAKEFILE_INCLUDE_FLAGS=$(echo "${WEBKITADDITIONS_HEADER_SEARCH_PATHS}" | perl -e 'print "-I" . join(" -I", split(" ", <>));')
$ make -n --no-builtin-rules ${MAKEFILE_INCLUDE_FLAGS} -f "WebCore/DerivedSources.make" SDKROOT="${SDKROOT}"

The "-n" will cause make to only print the commands it would have run. Notice that there is no generate-bindings.pl invocation in the result for InternalSettingsGenerated.idl

However, if you run the make command again (with the -n flag), there is a generate-bindings.pl invocation in the result for InternalSettingsGenerated.idl

If you run these same steps again, but you revert the last part of https://trac.webkit.org/changeset/178955/trunk/Source/WebCore/DerivedSources.make where make_settings.pl is linked to an .INTERMEDIATE rule, both make invocations list a generate-bindings.pl invocation for InternalSettingsGenerated.idl
Comment 1 Myles C. Maxfield 2017-01-07 19:48:40 PST
This caused the build failures in https://bugs.webkit.org/show_bug.cgi?id=164251
Comment 2 Chris Dumez 2017-01-07 19:51:55 PST
Thanks, I'll take a look.
Comment 3 Chris Dumez 2017-01-08 15:10:19 PST
I can reproduce the problem.

The Makefile looks like:
InternalSettingsGenerated.idl InternalSettingsGenerated.cpp InternalSettingsGenerated.h SettingsMacros.h : MakeSettings.intermediate
.INTERMEDIATE : MakeSettings.intermediate
MakeSettings.intermediate : page/make_settings.pl page/Settings.in
        $(PERL) $< --input $(WebCore)/page/Settings.in

JS%.h : %.idl $(JS_BINDINGS_SCRIPTS) $(IDL_ATTRIBUTES_FILE) $(WINDOW_CONSTRUCTORS_FILE) $(WORKERGLOBALSCOPE_CONSTRUCTORS_FILE) $(PLATFORM_FEATURE_DEFINES)
        $(PERL) $(WebCore)/bindings/scripts/generate-bindings.pl $(IDL_COMMON_ARGS) --defines "$(FEATURE_DEFINES) $(ADDITIONAL_IDL_DEFINES) LANGUAGE_JAVASCRIPT" --generator JS --idlAttributesFile $(IDL_ATTRIBUTES_FILE) --supplementalDependencyFile $(SUPPLEMENTAL_DEPENDENCY_FILE) $<

If you touch Settings.in, it regenerates InternalSettingsGenerated.idl but not InternalSettingsGenerated.cpp unless you run make twice.

I don't know yet why the dependency chain is broken. I am investigating.
Comment 4 Chris Dumez 2017-01-08 18:35:54 PST
Created attachment 298325 [details]
Patch
Comment 5 Chris Dumez 2017-01-08 18:39:29 PST
Link to StackOverflow that suggested both solutions (.INTERMEDIATE and pattern rules):
- http://stackoverflow.com/questions/2973445/gnu-makefile-rule-generating-a-few-targets-from-a-single-source-file#answer-10609434
Comment 6 Darin Adler 2017-01-08 21:09:19 PST
Comment on attachment 298325 [details]
Patch

Seems risky of any file name happens to match one of these patterns. For example HTMLNamesPrivate.h would match the pattern HTMLNames%h. But I don’t know a better solution.
Comment 7 Myles C. Maxfield 2017-01-08 21:50:35 PST
Comment on attachment 298325 [details]
Patch

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

rs=me

> Source/WebCore/ChangeLog:16
> +         act as many different rules with the same prerequisites and recipe. If a pattern

Left spacing doesn't line up

> Source/WebCore/DerivedSources.make:962
> +all : CSSValueKeywords.h CSSValueKeywords.cpp

What? The "all" target is already defined on line 957 above. How does this interact with the "all" that is defined on line 870?

> Source/WebCore/DerivedSources.make:1224
> +JSMathMLElementWrapperFactory%cpp JSMathMLElementWrapperFactory%h MathMLElementFactory%cpp MathMLElementFactory%h MathMLElementTypeHelpers%h MathMLNames%cpp MathMLNames%h : dom/make_names.pl bindings/scripts/Hasher.pm bindings/scripts/StaticString.pm mathml/mathtags.in mathml/mathattrs.in

Using % for . ? Scary. I wish there was a more elegant way to do this. (But even if there is, it doesn't have to be done in this patch.)
Comment 8 Darin Adler 2017-01-08 22:21:30 PST
Comment on attachment 298325 [details]
Patch

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

>> Source/WebCore/ChangeLog:16
>> +         act as many different rules with the same prerequisites and recipe. If a pattern
> 
> Left spacing doesn't line up

Not an accident, I don’t think. I believe Chris was lining up inside the quotation marks in a way that we normally don't do.

>> Source/WebCore/DerivedSources.make:962
>> +all : CSSValueKeywords.h CSSValueKeywords.cpp
> 
> What? The "all" target is already defined on line 957 above. How does this interact with the "all" that is defined on line 870?

This is not defining a target. It’s a rule that is adding dependencies to the "all" target; you can have any number of such rules. That’s the basic structure of this makefile. Every file that we want to build is set up as one of the dependencies of a single target named "all", which is also the first target in the makefile (which makes it the default) and is also marked as phony using .PHONY in a build rule.

>> Source/WebCore/DerivedSources.make:1224
>> +JSMathMLElementWrapperFactory%cpp JSMathMLElementWrapperFactory%h MathMLElementFactory%cpp MathMLElementFactory%h MathMLElementTypeHelpers%h MathMLNames%cpp MathMLNames%h : dom/make_names.pl bindings/scripts/Hasher.pm bindings/scripts/StaticString.pm mathml/mathtags.in mathml/mathattrs.in
> 
> Using % for . ? Scary. I wish there was a more elegant way to do this. (But even if there is, it doesn't have to be done in this patch.)

It’s not just scary, there is a danger there as I mention in my earlier comment. These patterns can match additional file names.
Comment 9 Chris Dumez 2017-01-09 08:29:17 PST
Comment on attachment 298325 [details]
Patch

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

>>> Source/WebCore/DerivedSources.make:1224
>>> +JSMathMLElementWrapperFactory%cpp JSMathMLElementWrapperFactory%h MathMLElementFactory%cpp MathMLElementFactory%h MathMLElementTypeHelpers%h MathMLNames%cpp MathMLNames%h : dom/make_names.pl bindings/scripts/Hasher.pm bindings/scripts/StaticString.pm mathml/mathtags.in mathml/mathattrs.in
>> 
>> Using % for . ? Scary. I wish there was a more elegant way to do this. (But even if there is, it doesn't have to be done in this patch.)
> 
> It’s not just scary, there is a danger there as I mention in my earlier comment. These patterns can match additional file names.

Yes, this is not ideal. The elegant way to do this was .INTERMEDIATE. It is really sad that .INTERMEDIATE causes make to not handle dependency chains properly. Maybe we can try using .INTERMEDIATE again when our GNU make gets updated.
Comment 10 Chris Dumez 2017-01-09 08:34:02 PST
Comment on attachment 298325 [details]
Patch

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

> Source/WebCore/DerivedSources.make:1295
> +JS%.cpp JS%.h : %.idl $(JS_BINDINGS_SCRIPTS) $(IDL_ATTRIBUTES_FILE) $(WINDOW_CONSTRUCTORS_FILE) $(WORKERGLOBALSCOPE_CONSTRUCTORS_FILE) $(PLATFORM_FEATURE_DEFINES)

For the record, another approach that worked was adding:
JSInternalSettingsGenerated.cpp JSInternalSettingsGenerated.h: JSInternalSettingsGenerated.intermediate
 .INTERMEDIATE : JSInternalSettingsGenerated.intermediate
JSInternalSettingsGenerated.intermediate: InternalSettingsGenerated.idl $(JS_BINDINGS_SCRIPTS) $(IDL_ATTRIBUTES_FILE) $(PLATFORM_FEATURE_DEFINES) MakeSettings.intermediate
         $(PERL) $(WebCore)/bindings/scripts/generate-bindings.pl $(IDL_COMMON_ARGS) --defines "$(FEATURE_DEFINES) $(ADDITIONAL_IDL_DEFINES) LANGUAGE_JAVASCRIPT" --generator JS --idlAttributesFile $(IDL_ATTRIBUTES_FILE) --supplementalDependencyFile $(SUPPLEMENTAL_DEPENDENCY_FILE) $<

Basically, adding MakeSettings.intermediate to the dependencies of JSInternalSettingsGenerated.cpp to help make figure out the dependency again. The InternalSettingsGenerated.idl should suffice but does not.
Comment 11 WebKit Commit Bot 2017-01-09 08:56:00 PST
Comment on attachment 298325 [details]
Patch

Clearing flags on attachment: 298325

Committed r210507: <http://trac.webkit.org/changeset/210507>
Comment 12 WebKit Commit Bot 2017-01-09 08:56:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Darin Adler 2017-01-09 09:24:35 PST
(In reply to comment #10)
> For the record, another approach that worked was adding:
> JSInternalSettingsGenerated.cpp JSInternalSettingsGenerated.h:
> JSInternalSettingsGenerated.intermediate
>  .INTERMEDIATE : JSInternalSettingsGenerated.intermediate
> JSInternalSettingsGenerated.intermediate: InternalSettingsGenerated.idl
> $(JS_BINDINGS_SCRIPTS) $(IDL_ATTRIBUTES_FILE) $(PLATFORM_FEATURE_DEFINES)
> MakeSettings.intermediate
>          $(PERL) $(WebCore)/bindings/scripts/generate-bindings.pl
> $(IDL_COMMON_ARGS) --defines "$(FEATURE_DEFINES) $(ADDITIONAL_IDL_DEFINES)
> LANGUAGE_JAVASCRIPT" --generator JS --idlAttributesFile
> $(IDL_ATTRIBUTES_FILE) --supplementalDependencyFile
> $(SUPPLEMENTAL_DEPENDENCY_FILE) $<
> 
> Basically, adding MakeSettings.intermediate to the dependencies of
> JSInternalSettingsGenerated.cpp to help make figure out the dependency
> again. The InternalSettingsGenerated.idl should suffice but does not.

Why did you decide not to do it that way?
Comment 14 Chris Dumez 2017-01-09 09:27:28 PST
(In reply to comment #13)
> (In reply to comment #10)
> > For the record, another approach that worked was adding:
> > JSInternalSettingsGenerated.cpp JSInternalSettingsGenerated.h:
> > JSInternalSettingsGenerated.intermediate
> >  .INTERMEDIATE : JSInternalSettingsGenerated.intermediate
> > JSInternalSettingsGenerated.intermediate: InternalSettingsGenerated.idl
> > $(JS_BINDINGS_SCRIPTS) $(IDL_ATTRIBUTES_FILE) $(PLATFORM_FEATURE_DEFINES)
> > MakeSettings.intermediate
> >          $(PERL) $(WebCore)/bindings/scripts/generate-bindings.pl
> > $(IDL_COMMON_ARGS) --defines "$(FEATURE_DEFINES) $(ADDITIONAL_IDL_DEFINES)
> > LANGUAGE_JAVASCRIPT" --generator JS --idlAttributesFile
> > $(IDL_ATTRIBUTES_FILE) --supplementalDependencyFile
> > $(SUPPLEMENTAL_DEPENDENCY_FILE) $<
> > 
> > Basically, adding MakeSettings.intermediate to the dependencies of
> > JSInternalSettingsGenerated.cpp to help make figure out the dependency
> > again. The InternalSettingsGenerated.idl should suffice but does not.
> 
> Why did you decide not to do it that way?

It felt hackish to have to duplicate the rule for generating bindings and having to explicitly add MakeSettings.intermediate to the dependencies. Also, this only fixes the issue for JSInternalSettingsGenerated.cpp. We would have to carefully review other uses of .INTERMEDIATE to see if they need hacks.

That said, I could be convinced to go this way.
Comment 15 Said Abou-Hallawa 2017-02-08 17:59:06 PST
*** Bug 165220 has been marked as a duplicate of this bug. ***