| Summary: | Add missing dependencies for <wtf/Platform.h> when generating derived sources | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
| Component: | WebCore Misc. | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ap, bfulgham, darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | 212451, 213976, 213979, 213980 | ||||||||
| Bug Blocks: | 233152 | ||||||||
| Attachments: |
|
||||||||
|
Description
David Kilzer (:ddkilzer)
2021-11-17 20:44:31 PST
Created attachment 444634 [details]
Patch v1
(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). (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. 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. 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. 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. Created attachment 444689 [details]
Patch for landing
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 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.) Comment on attachment 444689 [details]
Patch for landing
Marking cq+ since all Apple OSes have built successfully at least once.
Comment on attachment 444689 [details]
Patch for landing
Off by one!
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]. (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. |