Bug 233304 – Add missing dependencies for <wtf/Platform.h> when generating derived sources

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Add missing dependencies for <wtf/Platform.h> when generating derived sources in JavaScriptCore and WebCore.

This addresses a FIXME comment added for Bug 213980 in r263961.

[Radar WebKit Bug Importer]

<rdar://problem/85533245>

[David Kilzer (:ddkilzer)]

Created attachment 444634 [details]
Patch v1

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #0)
> This addresses a FIXME comment added for Bug 213980 in r263961.

The original FIXME was added for Bug 212451 in r262309.

Hmmm...I see Source/WebKitLegacy/mac/MigrateHeaders.make contains a similar FIXME.

I could fix that as well (tomorrow).

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #3)
> The original FIXME was added for Bug 212451 in r262309.
> 
> Hmmm...I see Source/WebKitLegacy/mac/MigrateHeaders.make contains a similar
> FIXME.
> 
> I could fix that as well (tomorrow).

That FIXME has since been removed, so it looks like it's just the two DerivedSources.make files that need this change.

[Darin Adler]

Comment on attachment 444634 [details]
Patch v1

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

I’ll say r+ even though I have questions, assuming the answers are all good

> Source/JavaScriptCore/ChangeLog:3
> +        Add missing dependencies for <wtf/Platform.h> when generating derived sources

Hard coding Platorm.h doesn’t seem exactly right. But I suppose it’s better than missing these dependencies. Maybe it’s really literally just Platform.h though, in which case it’s fine.

> Source/WebCore/ChangeLog:21
> +        - Add an input depdency on the script run from the build phase

This dependency only mentions the main script source files themselves. The scripts don’t in turn depend on any additional files? Our perl and python scripts typically do but maybe these shell scripts are simpler than that?

Also typo: “depdency”

Comments apply to the other similar case above too.

> Source/JavaScriptCore/DerivedSources-input.xcfilelist:2
> +$(BUILT_PRODUCTS_DIR)/usr/local/include/WebKitAdditions/AdditionalFeatureDefines.h

How can this be right? These files are installed as part of JavaScriptCore build, aren’t they? Later than this. So how can they be inputs? Or is this all installed as part of building other projects earlier?

If you understand why this is right, that’s good. To me it looks obviously wrong, but this could just be me getting educated.

[David Kilzer (:ddkilzer)]

Comment on attachment 444634 [details]
Patch v1

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

>> Source/JavaScriptCore/ChangeLog:3
>> +        Add missing dependencies for <wtf/Platform.h> when generating derived sources
> 
> Hard coding Platorm.h doesn’t seem exactly right. But I suppose it’s better than missing these dependencies. Maybe it’s really literally just Platform.h though, in which case it’s fine.

This is correct because we're generating a list of header dependencies for `FEATURE_AND_PLATFORM_DEFINES` (two lines up in both DerivedSources.make files):

FEATURE_AND_PLATFORM_DEFINES := $(shell $(CC) -std=gnu++1z -x c++ -E -P -dM $(SDK_FLAGS) $(TARGET_TRIPLE_FLAGS) $(FRAMEWORK_FLAGS) $(HEADER_FLAGS) -include "wtf/Platform.h" /dev/null | $(PERL) -ne "print if s/\#define ((HAVE_|USE_|ENABLE_|WTF_PLATFORM_)\w+) 1/\1/")

The missing dependency bug occurs when only Platform.h or other #included headers change to disable a feature, but an incremental build doesn't regenerate derived sources, and then the build fails because the derived sources still reference the now-disabled feature.

This is exactly what happened in the first two patches for Bug 233152.

In case you ask, I decided it was too complicated to try to use the output from one command to generate both the macros and the list of dependencies.  (I could, however, extract most of the compiler command into another variable if that would be better..)

>> Source/WebCore/ChangeLog:21
>> +        - Add an input depdency on the script run from the build phase
> 
> This dependency only mentions the main script source files themselves. The scripts don’t in turn depend on any additional files? Our perl and python scripts typically do but maybe these shell scripts are simpler than that?
> 
> Also typo: “depdency”
> 
> Comments apply to the other similar case above too.

In the case of both "Generate Derived Sources" build phase scripts, the DerivedSources-input.xcfilelist files list the _other_ dependencies, but they don't list the dependency on the initial generate-derived-sources.sh file itself (if only it changed).

Now I wonder whether the "Check .xcfilelist" build phase scripts purposefully didn't have any dependencies listed so Xcode would have to run it every time.  I'll remove that one in the next patch for both projects.

However, I think "Generate Derived Sources" build phase scripts should have an input dependency on the initial script being run in case it changes since it has a list of generated inputs (which is what this patch is updating to include Platform.h header dependencies).

Will fix the typos.

>> Source/JavaScriptCore/DerivedSources-input.xcfilelist:2
>> +$(BUILT_PRODUCTS_DIR)/usr/local/include/WebKitAdditions/AdditionalFeatureDefines.h
> 
> How can this be right? These files are installed as part of JavaScriptCore build, aren’t they? Later than this. So how can they be inputs? Or is this all installed as part of building other projects earlier?
> 
> If you understand why this is right, that’s good. To me it looks obviously wrong, but this could just be me getting educated.

> Or is this all installed as part of building other projects earlier?

Yes.  These headers are installed as part of other projects before JavaScriptCore.  Also, these dependencies are most beneficial on incremental (engineering) builds.  On clean builds (such as Production builds), the dependencies don't matter because the scripts have to run to generate the output file(s) regardless.

[Darin Adler]

Comment on attachment 444634 [details]
Patch v1

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

>>> Source/JavaScriptCore/ChangeLog:3
>>> +        Add missing dependencies for <wtf/Platform.h> when generating derived sources
>> 
>> Hard coding Platorm.h doesn’t seem exactly right. But I suppose it’s better than missing these dependencies. Maybe it’s really literally just Platform.h though, in which case it’s fine.
> 
> This is correct because we're generating a list of header dependencies for `FEATURE_AND_PLATFORM_DEFINES` (two lines up in both DerivedSources.make files):
> 
> FEATURE_AND_PLATFORM_DEFINES := $(shell $(CC) -std=gnu++1z -x c++ -E -P -dM $(SDK_FLAGS) $(TARGET_TRIPLE_FLAGS) $(FRAMEWORK_FLAGS) $(HEADER_FLAGS) -include "wtf/Platform.h" /dev/null | $(PERL) -ne "print if s/\#define ((HAVE_|USE_|ENABLE_|WTF_PLATFORM_)\w+) 1/\1/")
> 
> The missing dependency bug occurs when only Platform.h or other #included headers change to disable a feature, but an incremental build doesn't regenerate derived sources, and then the build fails because the derived sources still reference the now-disabled feature.
> 
> This is exactly what happened in the first two patches for Bug 233152.
> 
> In case you ask, I decided it was too complicated to try to use the output from one command to generate both the macros and the list of dependencies.  (I could, however, extract most of the compiler command into another variable if that would be better..)

My only worry about compiling twice  is compile time for fast path no-changes incremental builds. if this compilation runs every time we run make.

[David Kilzer (:ddkilzer)]

Created attachment 444689 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

Comment on attachment 444634 [details]
Patch v1

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

>>>> Source/JavaScriptCore/ChangeLog:3
>>>> +        Add missing dependencies for <wtf/Platform.h> when generating derived sources
>>> 
>>> Hard coding Platorm.h doesn’t seem exactly right. But I suppose it’s better than missing these dependencies. Maybe it’s really literally just Platform.h though, in which case it’s fine.
>> 
>> This is correct because we're generating a list of header dependencies for `FEATURE_AND_PLATFORM_DEFINES` (two lines up in both DerivedSources.make files):
>> 
>> FEATURE_AND_PLATFORM_DEFINES := $(shell $(CC) -std=gnu++1z -x c++ -E -P -dM $(SDK_FLAGS) $(TARGET_TRIPLE_FLAGS) $(FRAMEWORK_FLAGS) $(HEADER_FLAGS) -include "wtf/Platform.h" /dev/null | $(PERL) -ne "print if s/\#define ((HAVE_|USE_|ENABLE_|WTF_PLATFORM_)\w+) 1/\1/")
>> 
>> The missing dependency bug occurs when only Platform.h or other #included headers change to disable a feature, but an incremental build doesn't regenerate derived sources, and then the build fails because the derived sources still reference the now-disabled feature.
>> 
>> This is exactly what happened in the first two patches for Bug 233152.
>> 
>> In case you ask, I decided it was too complicated to try to use the output from one command to generate both the macros and the list of dependencies.  (I could, however, extract most of the compiler command into another variable if that would be better..)
> 
> My only worry about compiling twice  is compile time for fast path no-changes incremental builds. if this compilation runs every time we run make.

The compiler command itself takes less than a second, and it's only run when dependency inputs change.

Here's an equivalent command on a 2015 15" MBP (which uses xcrun, so it will take longer):

$ time xcrun -sdk macosx.internal clang -std=gnu++1z -x c++ -M -IWebKitBuild/Debug/usr/local/include -include "wtf/Platform.h" /dev/null > /dev/null

real	0m0.391s
user	0m0.025s
sys	0m0.286s

The second time it's even faster:

$ time xcrun -sdk macosx.internal clang -std=gnu++1z -x c++ -M -IWebKitBuild/Debug/usr/local/include -include "wtf/Platform.h" /dev/null > /dev/null

real	0m0.048s
user	0m0.022s
sys	0m0.022s

[David Kilzer (:ddkilzer)]

Comment on attachment 444634 [details]
Patch v1

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

>>>>> Source/JavaScriptCore/ChangeLog:3
>>>>> +        Add missing dependencies for <wtf/Platform.h> when generating derived sources
>>>> 
>>>> Hard coding Platorm.h doesn’t seem exactly right. But I suppose it’s better than missing these dependencies. Maybe it’s really literally just Platform.h though, in which case it’s fine.
>>> 
>>> This is correct because we're generating a list of header dependencies for `FEATURE_AND_PLATFORM_DEFINES` (two lines up in both DerivedSources.make files):
>>> 
>>> FEATURE_AND_PLATFORM_DEFINES := $(shell $(CC) -std=gnu++1z -x c++ -E -P -dM $(SDK_FLAGS) $(TARGET_TRIPLE_FLAGS) $(FRAMEWORK_FLAGS) $(HEADER_FLAGS) -include "wtf/Platform.h" /dev/null | $(PERL) -ne "print if s/\#define ((HAVE_|USE_|ENABLE_|WTF_PLATFORM_)\w+) 1/\1/")
>>> 
>>> The missing dependency bug occurs when only Platform.h or other #included headers change to disable a feature, but an incremental build doesn't regenerate derived sources, and then the build fails because the derived sources still reference the now-disabled feature.
>>> 
>>> This is exactly what happened in the first two patches for Bug 233152.
>>> 
>>> In case you ask, I decided it was too complicated to try to use the output from one command to generate both the macros and the list of dependencies.  (I could, however, extract most of the compiler command into another variable if that would be better..)
>> 
>> My only worry about compiling twice  is compile time for fast path no-changes incremental builds. if this compilation runs every time we run make.
> 
> The compiler command itself takes less than a second, and it's only run when dependency inputs change.
> 
> Here's an equivalent command on a 2015 15" MBP (which uses xcrun, so it will take longer):
> 
> $ time xcrun -sdk macosx.internal clang -std=gnu++1z -x c++ -M -IWebKitBuild/Debug/usr/local/include -include "wtf/Platform.h" /dev/null > /dev/null
> 
> real	0m0.391s
> user	0m0.025s
> sys	0m0.286s
> 
> The second time it's even faster:
> 
> $ time xcrun -sdk macosx.internal clang -std=gnu++1z -x c++ -M -IWebKitBuild/Debug/usr/local/include -include "wtf/Platform.h" /dev/null > /dev/null
> 
> real	0m0.048s
> user	0m0.022s
> sys	0m0.022s

Also, having to do a clean build (because dependencies are missing/broken) when one of the Platform.h headers changes (but derived sources aren't regenerated correctly) is going to take longer (and be much more frustrating) than running this compiler command.

You could argue that a change to a header file might not always change the macros defined, but you could also come up with a similar scenario for any of the dependent files--not just header files--which would cause derived sources to be regenerated.  I think dependency correctness wins here.  (It may be possible to add a step to check if previously generated feature macros actually changed, too, and only regenerate if they changed--if this change regenerates derived sources too often.)

[David Kilzer (:ddkilzer)]

Comment on attachment 444689 [details]
Patch for landing

Marking cq+ since all Apple OSes have built successfully at least once.

[David Kilzer (:ddkilzer)]

Comment on attachment 444689 [details]
Patch for landing

Off by one!

[EWS]

Committed r286018 (244407@main): <https://commits.webkit.org/244407@main>

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

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #9)
> > My only worry about compiling twice  is compile time for fast path no-changes incremental builds. if this compilation runs every time we run make.
> 
> The compiler command itself takes less than a second, and it's only run when
> dependency inputs change.

Great! It’s the latter that matters more to me.