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.
Created attachment 400424 [details] Patch
Hmm, seems like there’s a problem in JavaScriptCore. I’ll fix and upload a new patch.
Created attachment 400425 [details] Patch
OK, it’s going to work now.
Created attachment 400426 [details] Patch
Created attachment 400461 [details] Patch
Comment on attachment 400461 [details] Patch Nice.
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?
(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.
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.
(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!
Committed r262245: <https://trac.webkit.org/changeset/262245>
<rdar://problem/63722207>
(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.
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/
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?
(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.
(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>
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
Re-opened since this is blocked by bug 212531
Preparing a new patch that uses ":=" to fix the performance problem.
Created attachment 400602 [details] Patch
(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!
Committed r262309: <https://trac.webkit.org/changeset/262309>
*** Bug 212500 has been marked as a duplicate of this bug. ***