Bug 212451 – [Cocoa] Pass all defines from Platform.h to various scripts, not just the ones from .xcconfig

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Darin Adler]

I’d like to move things from .xcconfig to PlatformEnableCocoa.h files. To make that possible the makefile needs to pass defines from there to the various derived sources scripts, not just the ones from the .xcconfig file. Found a good way to do it.

[Darin Adler]

Created attachment 400424 [details]
Patch

[Darin Adler]

Hmm, seems like there’s a problem in JavaScriptCore. I’ll fix and upload a new patch.

[Darin Adler]

Created attachment 400425 [details]
Patch

[Darin Adler]

OK, it’s going to work now.

[Darin Adler]

Created attachment 400426 [details]
Patch

[Darin Adler]

Created attachment 400461 [details]
Patch

[Sam Weinig]

Comment on attachment 400461 [details]
Patch

Nice.

[Sam Weinig]

One thing I was considering when I looked into this problem space earlier this year was trying to move away from doing any preprocessing of input files at all, and moving to a model where the script phases always produce the same output, but with the correct #ifdefs inserted into them. The potential benefit here would be that when doing something like switching from building iOS to building macOS, the generated files would not have to be regenerated (something that continues to take too long). 

I am curious if you think that is a direction worth pursing?

[Darin Adler]

(In reply to Sam Weinig from comment #8)
> One thing I was considering when I looked into this problem space earlier
> this year was trying to move away from doing any preprocessing of input
> files at all, and moving to a model where the script phases always produce
> the same output, but with the correct #ifdefs inserted into them. The
> potential benefit here would be that when doing something like switching
> from building iOS to building macOS, the generated files would not have to
> be regenerated (something that continues to take too long).
>
> I am curious if you think that is a direction worth pursing?

I tried this first, but found too many cases that were hard to convert, so it wasn’t easy to do yet.

[Darin Adler]

I have a fancier version of this patch that uses -MD to figure out the contents of FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES, but I am having trouble making that work without unwanted paths to specific files in the SDK finding their way into .xcfilelist files, so I will set it aside for now.

[Darin Adler]

(In reply to Darin Adler from comment #9)
> (In reply to Sam Weinig from comment #8)
> > I am curious if you think that is a direction worth pursing?
> 
> I tried this first, but found too many cases that were hard to convert, so
> it wasn’t easy to do yet.

To be clear, I do find this direction appealing, but it seems to be a moderately long road. We have a lot of different kinds of generated code!

[Darin Adler]

Committed r262245: <https://trac.webkit.org/changeset/262245>

[Radar WebKit Bug Importer]

<rdar://problem/63722207>

[Sam Weinig]

(In reply to Darin Adler from comment #11)
> (In reply to Darin Adler from comment #9)
> > (In reply to Sam Weinig from comment #8)
> > > I am curious if you think that is a direction worth pursing?
> > 
> > I tried this first, but found too many cases that were hard to convert, so
> > it wasn’t easy to do yet.
> 
> To be clear, I do find this direction appealing, but it seems to be a
> moderately long road. We have a lot of different kinds of generated code!

Oh for sure. Just wanted to make sure we were thinking of a similar long term goal, even if it is far out in time. Glad we are on the same page.

[David Kilzer (:ddkilzer)]

Comment on attachment 400461 [details]
Patch

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

> Source/JavaScriptCore/DerivedSources.make:34
> +FRAMEWORK_FLAGS = $(shell echo $(BUILT_PRODUCTS_DIR) $(FRAMEWORK_SEARCH_PATHS) $(SYSTEM_FRAMEWORK_SEARCH_PATHS) | $(PERL) -e 'print "-F " . join(" -F ", split(" ", <>));')
> +HEADER_FLAGS = $(shell echo $(BUILT_PRODUCTS_DIR) $(HEADER_SEARCH_PATHS) $(SYSTEM_HEADER_SEARCH_PATHS) | $(PERL) -e 'print "-I" . join(" -I", split(" ", <>));')
> +FEATURE_DEFINE_FLAGS = $(shell echo $(FEATURE_DEFINES) | $(PERL) -e 'print "-D" . join(" -D", split(" ", <>));')

You could use map() instead of join() to remove the need to specify strings like "-F", "-I" and "-D" twice:

$ echo "A B C D" | perl -e 'print "-I" . join(" -I", split(" ", <>));'
-IA -IB -IC -ID

$ echo "A B C D" | perl -e 'print join(" ", map("-I$_", split(" ", <>)));'
-IA -IB -IC -ID

Not necessary to change to land the patch.  I think it's easier to read the code using map() because it treats every string the same way rather than trying to be clever.

> Source/JavaScriptCore/DerivedSources.make:48
> +# FIXME: Should generate the list of everything included by Platform.h as a side effect of the above command.
> +FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES = Configurations/FeatureDefines.xcconfig DerivedSources.make

Here's one way to do that (may need to adjust the grep string).  Note that "-H" replaces "-E -P -dM".

$ clang -H [args] -include "wtf/Platform.h" /dev/null 2>&1 | sed -e 's/^\.\.* //' | grep /Source/WTF/

> Source/WebCore/DerivedSources.make:36
> +FRAMEWORK_FLAGS = $(shell echo $(BUILT_PRODUCTS_DIR) $(FRAMEWORK_SEARCH_PATHS) $(SYSTEM_FRAMEWORK_SEARCH_PATHS) | perl -e 'print "-F " . join(" -F ", split(" ", <>));')
> +HEADER_FLAGS = $(shell echo $(BUILT_PRODUCTS_DIR) $(HEADER_SEARCH_PATHS) $(SYSTEM_HEADER_SEARCH_PATHS) | perl -e 'print "-I" . join(" -I", split(" ", <>));')
> +FEATURE_DEFINE_FLAGS = $(shell echo $(FEATURE_DEFINES) | perl -e 'print "-D" . join(" -D", split(" ", <>));')

Ditto re: using map().

> Source/WebCore/DerivedSources.make:40
> +ifneq ($(SDKROOT),)
> +    SDK_FLAGS=-isysroot $(SDKROOT)
> +endif

Source/WebCore/Scripts/generate-derived-sources.sh passes SDKROOT on the make command-line, so this will NOT cause the same issue as Bug 212436.

> Source/WebCore/DerivedSources.make:50
> +# FIXME: Should generate the list of everything included by Platform.h as a side effect of the above command.
> +FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES = Configurations/FeatureDefines.xcconfig DerivedSources.make

Here's one way to do that (may need to adjust the grep string).  Note that "-H" replaces "-E -P -dM".

$ clang -H [args] -include "wtf/Platform.h" /dev/null 2>&1 | sed -e 's/^\.\.* //' | grep /Source/WTF/

> Source/WebKitLegacy/mac/MigrateHeaders.make:32
> +FRAMEWORK_FLAGS = $(shell echo $(BUILT_PRODUCTS_DIR) $(FRAMEWORK_SEARCH_PATHS) $(SYSTEM_FRAMEWORK_SEARCH_PATHS) | $(PERL) -e 'print "-F " . join(" -F ", split(" ", <>));')
> +HEADER_FLAGS = $(shell echo $(BUILT_PRODUCTS_DIR) $(HEADER_SEARCH_PATHS) $(SYSTEM_HEADER_SEARCH_PATHS) | $(PERL) -e 'print "-I" . join(" -I", split(" ", <>));')
> +FEATURE_DEFINE_FLAGS = $(shell echo $(FEATURE_DEFINES) | $(PERL) -e 'print "-D" . join(" -D", split(" ", <>));')

Ditto re: using map().

> Source/WebKitLegacy/mac/MigrateHeaders.make:36
> +ifneq ($(SDKROOT),)
> +    SDK_FLAGS=-isysroot $(SDKROOT)
> +endif

Source/WebKitLegacy//mac/migrate-headers.sh does NOT include SDKROOT="$(SDKROOT)" when invoking make, so this could cause the same regression as Bug 212436.

> Source/WebKitLegacy/mac/MigrateHeaders.make:46
> +# FIXME: Should generate the list of everything included by Platform.h as a side effect of the above command.
> +FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES = Configurations/FeatureDefines.xcconfig DerivedSources.make

Here's one way to do that (may need to adjust the grep string).  Note that "-H" replaces "-E -P -dM".

$ clang -H [args] -include "wtf/Platform.h" /dev/null 2>&1 | sed -e 's/^\.\.* //' | grep /Source/WTF/

[Darin Adler]

Comment on attachment 400461 [details]
Patch

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

>> Source/JavaScriptCore/DerivedSources.make:34
>> +FEATURE_DEFINE_FLAGS = $(shell echo $(FEATURE_DEFINES) | $(PERL) -e 'print "-D" . join(" -D", split(" ", <>));')
> 
> You could use map() instead of join() to remove the need to specify strings like "-F", "-I" and "-D" twice:
> 
> $ echo "A B C D" | perl -e 'print "-I" . join(" -I", split(" ", <>));'
> -IA -IB -IC -ID
> 
> $ echo "A B C D" | perl -e 'print join(" ", map("-I$_", split(" ", <>)));'
> -IA -IB -IC -ID
> 
> Not necessary to change to land the patch.  I think it's easier to read the code using map() because it treats every string the same way rather than trying to be clever.

I’d welcome this refinement, although I probably won’t do it myself. I just followed the pattern that was already present.

>> Source/JavaScriptCore/DerivedSources.make:48
>> +FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES = Configurations/FeatureDefines.xcconfig DerivedSources.make
> 
> Here's one way to do that (may need to adjust the grep string).  Note that "-H" replaces "-E -P -dM".
> 
> $ clang -H [args] -include "wtf/Platform.h" /dev/null 2>&1 | sed -e 's/^\.\.* //' | grep /Source/WTF/

I have a local set of changes that make this work using "-MD -MF Platform.deps" in the compile line. Good to generate the dependencies with the same command that is actually used, so we don’t have flags differences or that sort of thing:

FEATURE_AND_PLATFORM_DEFINES = $(shell $(CC) -std=gnu++1z -x c++ -E -P -dM $(SDK_FLAGS) $(TARGET_TRIPLE_FLAGS) $(FRAMEWORK_FLAGS) $(HEADER_FLAGS) $(FEATURE_DEFINE_FLAGS) -MD -MF Platform.deps -include "wtf/Platform.h" /dev/null | $(PERL) -ne "print if s/\#define ((HAVE_|USE_|ENABLE_|WTF_PLATFORM_)\w+) 1/\1/")
FEATURE_AND_PLATFORM_DEFINE_DEPENDENCIES = Configurations/FeatureDefines.xcconfig DerivedSources.make $(shell $(PERL) -ne "s| \\||; print if m|$BUILT_PRODUCTS_DIR|" Platform.deps)

However, I don’t plan to pursue this any time soon. You are welcome to if you like.

>> Source/WebCore/DerivedSources.make:36
>> +FEATURE_DEFINE_FLAGS = $(shell echo $(FEATURE_DEFINES) | perl -e 'print "-D" . join(" -D", split(" ", <>));')
> 
> Ditto re: using map().

Yes, there are three copies of the identical code in these three makefiles.

>> Source/WebKitLegacy/mac/MigrateHeaders.make:32
>> +FEATURE_DEFINE_FLAGS = $(shell echo $(FEATURE_DEFINES) | $(PERL) -e 'print "-D" . join(" -D", split(" ", <>));')
> 
> Ditto re: using map().

Yes, third copy of the same code.

>> Source/WebKitLegacy/mac/MigrateHeaders.make:36
>> +endif
> 
> Source/WebKitLegacy//mac/migrate-headers.sh does NOT include SDKROOT="$(SDKROOT)" when invoking make, so this could cause the same regression as Bug 212436.

Would you be willing to fix that for me, or would you like me to handle it?

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #16)
> Comment on attachment 400461 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400461&action=review
> 
> >> Source/WebKitLegacy/mac/MigrateHeaders.make:36
> >> +endif
> > 
> > Source/WebKitLegacy//mac/migrate-headers.sh does NOT include SDKROOT="$(SDKROOT)" when invoking make, so this could cause the same regression as Bug 212436.
> 
> Would you be willing to fix that for me, or would you like me to handle it?

Yes.  I have to now to fix my build:

[...]

=== BUILD TARGET WebKitLegacy OF PROJECT WebKitLegacy WITH CONFIGURATION Debug ===

Check dependencies

PhaseScriptExecution Migrate\ Headers /var/build/WebKitLegacy.build/Debug/WebKitLegacy.build/Script-1C6CB0510AA63EB000D23BFD.sh
    cd /Users/ddkilzer/Data/WebKit.git/Source/WebKitLegacy
    /bin/sh -c /var/build/WebKitLegacy.build/Debug/WebKitLegacy.build/Script-1C6CB0510AA63EB000D23BFD.sh
clang: warning: no such sysroot directory: 'macosx.internal' [-Wmissing-sysroot]
In file included from <built-in>:1:
In file included from /var/build/Debug/usr/local/include/wtf/Platform.h:44:
/var/build/Debug/usr/local/include/wtf/PlatformOS.h:36:10: fatal error: 'Availability.h' file not found
#include <Availability.h>
         ^~~~~~~~~~~~~~~~
1 error generated.
sed -E -e 's/<WebCore\//<WebKitLegacy\//' -e "s/(^ *)WEBCORE_EXPORT /\1/" /var/build/Debug/DerivedSources/WebKitLegacy/WebCorePrivateHeaders/WebKitAvailability.h > /var/build/Debug/WebKitLegacy.framework/Versions/A/PrivateHeaders/WebKitAvailability.h; touch /var/build/Debug/WebKitLegacy.framework/Versions/A/PrivateHeaders
sed -E -e 's/<WebCore\//<WebKitLegacy\//' -e "s/(^ *)WEBCORE_EXPORT /\1/" /var/build/Debug/DerivedSources/WebKitLegacy/WebCorePrivateHeaders/WebScriptObject.h > /var/build/Debug/WebKitLegacy.framework/Versions/A/PrivateHeaders/WebScriptObject.h; touch /var/build/Debug/WebKitLegacy.framework/Versions/A/PrivateHeaders
sed -E -e 's/<WebCore\//<WebKitLegacy\//' -e "s/(^ *)WEBCORE_EXPORT /\1/" /var/build/Debug/DerivedSources/WebKitLegacy/WebCorePrivateHeaders/npapi.h > /var/build/Debug/WebKitLegacy.framework/Versions/A/PrivateHeaders/npapi.h; touch /var/build/Debug/WebKitLegacy.framework/Versions/A/PrivateHeaders
sed -E -e 's/<WebCore\//<WebKitLegacy\//' -e "s/(^ *)WEBCORE_EXPORT /\1/" /var/build/Debug/DerivedSources/WebKitLegacy/WebCorePrivateHeaders/npfunctions.h > /var/build/Debug/WebKitLegacy.framework/Versions/A/PrivateHeaders/npfunctions.h; touch /var/build/Debug/WebKitLegacy.framework/Versions/A/PrivateHeaders
sed -E -e 's/<WebCore\//<WebKitLegacy\//' -e "s/(^ *)WEBCORE_EXPORT /\1/" /var/build/Debug/DerivedSources/WebKitLegacy/WebCorePrivateHeaders/npruntime.h > /var/build/Debug/WebKitLegacy.framework/Versions/A/PrivateHeaders/npruntime.h; touch /var/build/Debug/WebKitLegacy.framework/Versions/A/PrivateHeaders
sed -E -e 's/<WebCore\//<WebKitLegacy\//' -e "s/(^ *)WEBCORE_EXPORT /\1/" /var/build/Debug/DerivedSources/WebKitLegacy/WebCorePrivateHeaders/nptypes.h > /var/build/Debug/WebKitLegacy.framework/Versions/A/PrivateHeaders/nptypes.h; touch /var/build/Debug/WebKitLegacy.framework/Versions/A/PrivateHeaders
clang: warning: no such sysroot directory: 'macosx.internal' [-Wmissing-sysroot]
In file included from <built-in>:1:
In file included from /var/build/Debug/usr/local/include/wtf/Platform.h:44:
/var/build/Debug/usr/local/include/wtf/PlatformOS.h:36:10: fatal error: 'Availability.h' file not found
#include <Availability.h>
         ^~~~~~~~~~~~~~~~
1 error generated.
make[4]: Nothing to be done for `reexport_headers'.
Command /bin/sh emitted errors but did not return a nonzero exit code to indicate failure

[...]

Will commit as a follow-up/build fix to this momentarily.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #12)
> Committed r262245: <https://trac.webkit.org/changeset/262245>

Follow-up build fix:
Committed r262253: <https://trac.webkit.org/changeset/262253>

[Andy Estes]

With this change, WebCore's "Check .xcfilelists" build phase went from taking less than a second to run on a no-op incremental build to taking over 95 seconds.

Filed <https://webkit.org/b/212500> REGRESSION (r262245): WebCore's "Check .xcfilelists" build phase is ~100x slower

[Yusuke Suzuki]

Re-opened since this is blocked by bug 212531

[Darin Adler]

Preparing a new patch that uses ":=" to fix the performance problem.

[Darin Adler]

Created attachment 400602 [details]
Patch

[Andy Estes]

(In reply to Darin Adler from comment #21)
> Preparing a new patch that uses ":=" to fix the performance problem.

Can confirm your latest patch fixes the performance problem. Thanks!

[Darin Adler]

Committed r262309: <https://trac.webkit.org/changeset/262309>

[Andy Estes]

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