| Summary: | REGRESSION(r178955): Touching Settings.in doesn't cause JSInternalSettingsGenerated.cpp to be updated on first build | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||
| Component: | Bindings | Assignee: | Chris Dumez <cdumez> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | ap, cdumez, commit-queue, darin, ddkilzer, sabouhallawa | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| URL: | http://stackoverflow.com/questions/2973445/gnu-makefile-rule-generating-a-few-targets-from-a-single-source-file#answer-10609434 | ||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=165220 | ||||||
| Attachments: |
|
||||||
This caused the build failures in https://bugs.webkit.org/show_bug.cgi?id=164251 Thanks, I'll take a look. 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.
Created attachment 298325 [details]
Patch
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 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 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 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 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 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 on attachment 298325 [details] Patch Clearing flags on attachment: 298325 Committed r210507: <http://trac.webkit.org/changeset/210507> All reviewed patches have been landed. Closing bug. (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? (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. *** Bug 165220 has been marked as a duplicate of this bug. *** |
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