Lessen the reliance on VPATH in WebCore/DerivedSources.make

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Keith Rollin]

Due to the way vpath/VPATH works in `make`, we can get into a situation where necessarily IDL files are not copied to their correct locations.

WebKit comes with many IDL files. When building for internal purposes, Apple can add to or modify this set of IDL files. When creating Engineering builds, these additional IDL files are copied to .../WebKitBuild/<configuration>/usr/local/includes. When performing Production builds, these IDL files are included in the SDK. This arrangement can be seen in DerivedSources.make:

    vpath %.idl $(BUILT_PRODUCTS_DIR)/usr/local/include $(SDKROOT)/usr/local/include

In order to get these IDL files into a location where they can be uniformly found and handled, there is a step in DerivedSources.make that copies them to the current directory, which is WebKitBuild/<configuration>/DerivedSources/WebCore:

    $(ADDITIONAL_BINDING_IDLS) : % : WebKitAdditions/%
       cp $< .

The IDL files specified in ADDITIONAL_BINDING_IDLS are specified as bare files names, meaning that vpath and VPATH are employed to find them. Given the vpath specification previously shown, if the IDL files can be found in $(BUILT_PRODUCTS_DIR)/usr/local/include, they are copied locally. Otherwise, they should be found in $(SDKROOT)/usr/local/include and copied locally.

As it turns out, vpath/VPATH resolution is performed not only on the prerequisites of the build rule, but also the target. This means that the files specified on the left side of the `cp` rule above are also searched for. And since the standard IDL files (the ones that are being replaced with the Apple-specific versions) can be found on VPATH as it's defined in DerivedSources.make (specifically, in $(WebCore)/dom), then those files participate in determining if the `cp` rule is executed.

Consider the normal build case: repositories are checked out (applying the current modification timestamp to the files). During the build, the files in $(ADDITIONAL_BINDING_IDLS) are copied to either BUILT_PRODUCTS_DIR or to SDKROOT, which again modifies their timestamps. We eventually get to the build rule for the `cp` operation. Because the files in WebKitAdditions (be it in the local build directory or the SDK) are *newer* than the checked-out ones in $(WebCore)/dom due to their having been copied after they were checked-out, then the `cp` command is invoked and then files are copied to the current build directory.

However, consider a slightly different build case. In this case, projects are checked out and built one at a time. So the project producing the files in ADDITIONAL_BINDING_IDLS is checked out and then the IDL files are copied to their intermediate location and have their timestamps set to that time. Later, WebCore is checked out, and DerivedSources.make is eventually invoked. Now, the files in $(WebCore)/dom are *newer* than those in WebKitAdditions. `make` determines that the IDL files are up-to-date and does not execute the `cp` commands. The IDL files are not copied, and the build then either fails (because of missing files) or proceeds incorrectly (because the wrong IDL files are found).

The solution to this issue is to lessen the reliance on vpath/VPATH in DerivedSources.make. Instead, be more explicit about where files can be found and trim down vpath/VPATH to it's bare minimum. With vpath/VPATH reduced, we remove the issue of files being accidentally discovered that we don't want discovered.

[Radar WebKit Bug Importer]

<rdar://problem/70279941>

[Keith Rollin]

Created attachment 411303 [details]
Patch

[Alexey Proskuryakov]

Sam, I think that this fixes the root cause of troubles in bug 216831.

[Sam Weinig]

(In reply to Alexey Proskuryakov from comment #3)
> Sam, I think that this fixes the root cause of troubles in bug 216831.

Awesome.

[Darin Adler]

Comment on attachment 411303 [details]
Patch

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

I wonder if we should use $@ and $< more often to avoid repeating path names.

> Source/WebCore/DerivedSources.make:1299
> +# in ADDITIONAL_BINDING_IDLS_PATHS and the existance of the IDL file at each

existence misspelled here

> Source/WebCore/DerivedSources.make:1316
> +            $(path)/usr/local/include/WebKitAdditions/$(idl)))))

Was there an explicit version of this /usr/local/include path already present elsewhere in this makefile?

> Source/WebCore/DerivedSources.make:1850
> -WebCore_BUILTINS_SOURCES_LIST : $(JavaScriptCore_SCRIPTS_DIR)/UpdateContents.py DerivedSources.make
> +WebCore_BUILTINS_SOURCES_LIST : $(JavaScriptCore_SCRIPTS_DIR)/UpdateContents.py $(FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES)

I don’t think this is right. I plan to eventually replace FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES with a different approach; a single generated file. But specifically here, is the dependency really on feature and platform defines? Or is there some other reason to rebuild any time we touch the makefile?

> Source/WebCore/DerivedSources.make:1853
> -WebCore_BUILTINS_DEPENDENCIES_LIST : $(JavaScriptCore_SCRIPTS_DIR)/UpdateContents.py DerivedSources.make
> +WebCore_BUILTINS_DEPENDENCIES_LIST : $(JavaScriptCore_SCRIPTS_DIR)/UpdateContents.py $(FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES)

Ditto.

[Keith Rollin]

(In reply to Darin Adler from comment #5)
> Comment on attachment 411303 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411303&action=review
> 
> I wonder if we should use $@ and $< more often to avoid repeating path names.
> 
> > Source/WebCore/DerivedSources.make:1299
> > +# in ADDITIONAL_BINDING_IDLS_PATHS and the existance of the IDL file at each
> 
> existence misspelled here

Thanks. I thought I spell-checked that comment, but I guess I was thinking of a different comment.

> > Source/WebCore/DerivedSources.make:1316
> > +            $(path)/usr/local/include/WebKitAdditions/$(idl)))))
> 
> Was there an explicit version of this /usr/local/include path already
> present elsewhere in this makefile?\

The use of that path was derived from the following line (being removed in this patch:

 vpath %.idl $(BUILT_PRODUCTS_DIR)/usr/local/include $(SDKROOT)/usr/local/include

> > Source/WebCore/DerivedSources.make:1850
> > -WebCore_BUILTINS_SOURCES_LIST : $(JavaScriptCore_SCRIPTS_DIR)/UpdateContents.py DerivedSources.make
> > +WebCore_BUILTINS_SOURCES_LIST : $(JavaScriptCore_SCRIPTS_DIR)/UpdateContents.py $(FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES)
> 
> I don’t think this is right. I plan to eventually replace
> FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES with a different approach; a single
> generated file. But specifically here, is the dependency really on feature
> and platform defines? Or is there some other reason to rebuild any time we
> touch the makefile?

My attention was drawn to those instances of DerivedSources.make when I had to prefix them with $(WebCore). Since it looked like they were just overlooked or otherwise missed in the face of $(FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES), I just made the mindless substitution, making it consistent with other places where $(FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES) was used. I'll revert since it sounds like that change was inappropriate.

[Keith Rollin]

Created attachment 411510 [details]
Patch

[EWS]

Committed r268566: <https://trac.webkit.org/changeset/268566>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411510 [details].