Bug 166814 – REGRESSION(r178955): Touching Settings.in doesn't cause JSInternalSettingsGenerated.cpp to be updated on first build

• Component: Bindings
• Status: RESOLVED FIXED

[Myles C. Maxfield]

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

[Myles C. Maxfield]

This caused the build failures in https://bugs.webkit.org/show_bug.cgi?id=164251

[Chris Dumez]

Thanks, I'll take a look.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 298325 [details]
Patch

[Chris Dumez]

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

[Darin Adler]

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.

[Myles C. Maxfield]

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.)

[Darin Adler]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[WebKit Commit Bot]

Comment on attachment 298325 [details]
Patch

Clearing flags on attachment: 298325

Committed r210507: <http://trac.webkit.org/changeset/210507>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(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?

[Chris Dumez]

(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.

[Said Abou-Hallawa]

*** Bug 165220 has been marked as a duplicate of this bug. ***