Bug 238901 – [XCBuild] Enable dependency validation by default

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Williams]

[XCBuild] Enable dependency validation by default

[Elliott Williams]

Created attachment 456873 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/91379968>

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Elliott Williams]

One thing to note: In my testing, the internal dependency validation logic does not fail the build. It'll emit an error in the build log like:

    Missing creator task for discovered dependency input node: ... Did you forget to declare this node as an output of a script phase or custom build rule which produces it?
    
    Command CompileC emitted errors but did not return a nonzero exit code to indicate failure


On my machine, Xcode reports this as "Build finished with errors". Command line xcodebuild emits the error, but prints "BUILD SUCCEEDED" and exit code 0.

I'm not sure what EWS will do in this situation, but I'd like for it to show as failing. I haven't checked, but that might require us to change how the bots detect errors.

[Alexey Proskuryakov]

Comment on attachment 456873 [details]
Patch

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

Looking at the build log in EWS, I see lines like:

Check dependencies
Warning: Multiple build commands for output file /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/Release/usr/local/include/absl_flattened/config.h
Warning: Multiple build commands for output file /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/Release/usr/local/include/absl_flattened/escaping.h
Warning: Multiple build commands for output file /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/Release/usr/local/include/absl_flattened/inlined_vector.h
Warning: Multiple build commands for output file /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/Release/usr/local/include/absl_flattened/optional.h
Warning: Multiple build commands for output file /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/Release/usr/local/include/absl_flattened/span.h
Warning: Multiple build commands for output file /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/Release/usr/local/include/absl_flattened/variant.h

Are these part of this new validation, or something pre-existing? Also, should we be concerned, and can these be turned into errors?

> Tools/Scripts/build-webkit:358
> +    setCreatedByXcodeBuildSystem();

Not new in this patch AFAICT, but the name doesn't quite parse. Maybe "markBaseProductDirectoryAsCreatedByXcodeBuildSystem"?

> Tools/Scripts/set-webkit-configuration:90
> +if (isAppleCocoaWebKit()) {
> +    setCreatedByXcodeBuildSystem();

What do we want the behavior to be when building with CMake for Cocoa platforms?

> ChangeLog:19
> +        Always call through to set-webkit-configuration, because that's where
> +        we verify that CreatedByBuildSystem is set on the build directory.

Is this Makefile.shared used for installsrc? It would be weird to create and mark a build directory for that.

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #5)
> Comment on attachment 456873 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456873&action=review
> 
> Looking at the build log in EWS, I see lines like:
> 
> Check dependencies
> Warning: Multiple build commands for output file
> /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/
> Release/usr/local/include/absl_flattened/config.h
> Warning: Multiple build commands for output file
> /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/
> Release/usr/local/include/absl_flattened/escaping.h
> Warning: Multiple build commands for output file
> /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/
> Release/usr/local/include/absl_flattened/inlined_vector.h
> Warning: Multiple build commands for output file
> /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/
> Release/usr/local/include/absl_flattened/optional.h
> Warning: Multiple build commands for output file
> /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/
> Release/usr/local/include/absl_flattened/span.h
> Warning: Multiple build commands for output file
> /Volumes/Data/worker/macOS-BigSur-Release-Build-EWS/build/WebKitBuild/
> Release/usr/local/include/absl_flattened/variant.h
> 
> Are these part of this new validation, or something pre-existing? Also,
> should we be concerned, and can these be turned into errors?

These are recent but not new, and they will go away when we switch to XCBuild, so I think it's best to leave them for now!

> > Tools/Scripts/build-webkit:358
> > +    setCreatedByXcodeBuildSystem();
> 
> Not new in this patch AFAICT, but the name doesn't quite parse. Maybe
> "markBaseProductDirectoryAsCreatedByXcodeBuildSystem"?

Sure!

> > Tools/Scripts/set-webkit-configuration:90
> > +if (isAppleCocoaWebKit()) {
> > +    setCreatedByXcodeBuildSystem();
> 
> What do we want the behavior to be when building with CMake for Cocoa
> platforms?

It doesn't make sense to do this under CMake, plus I don't think CMake users would run set-webkit-configuration automatically, like Make does. I see `isAppleCocoaWebKit() && !isCMakeBuild()` is an idiom we use elsewhere; I'll use it here.

> > ChangeLog:19
> > +        Always call through to set-webkit-configuration, because that's where
> > +        we verify that CreatedByBuildSystem is set on the build directory.
> 
> Is this Makefile.shared used for installsrc? It would be weird to create and
> mark a build directory for that.

Makefile.shared is used, but the `installsrc` recipe specifically does not call set-webkit-configuration, so this is fine.

[Elliott Williams]

Created attachment 457000 [details]
Patch

[Elliott Williams]

Comment on attachment 457000 [details]
Patch

Review feedback, and a last-minute fix to make `set-webkit-configuration` behave better when it isn't given any arguments.

I think this is a minor enough change to retain Alexey's r+.

[EWS]

Committed r292591 (249424@main): <https://commits.webkit.org/249424@main>

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

[Elliott Williams]

Reverted r292591 for reason:

Causing spurious EWS errors

Committed r292626 (249446@trunk): <https://commits.webkit.org/249446@trunk>

[Elliott Williams]

So there are a few problems we'll need to sort out before this can re-land:

1. Dependency validation doesn't work and isn't meaningful in sequential, project-by-project workflows, which is how Make and build-webkit currently work. This is because dependency validation relies on XCBuild's graph having an edge for every file in the build directory. It needs a "full picture" of the build process to know if any edges are missing, and sequential builds prevent that knowledge because each project builds independently of the others. 

It'd be nice to only enable it in workspace builds, but there's no way to tell whether or not we're in a workspace via build settings. We could tie it to the USE_WORKSPACE command-line variable, and only perform validation in command line builds, which would provide verification via EWS. But that leads to the second problem...


2. Some script phases, mostly our scripts with generated input xcfilelists, refer to files that are only present in internal builds. For example: JavaScriptCore depends on internal-only profdata and WebCore DerivedSources depends on AdditionalFeatureDefines.h.

This is because our input xcfilelists are a union of every possible dependency, which is fine for incremental builds (the script phase will only be scheduled if an existing file changes or a missing file appears), but not for dependency validation, because xcbuild can't know those internal-only paths are _supposed to_ be missing on external builds.

We'd like to derive or completely get rid of generated xcfilelists <https://bugs.webkit.org/show_bug.cgi?id=236056>, but that's a substantial effort that shouldn't block this change.

But until we do something about these internal-only paths, this logic will report spurious errors on EWS.


--

What options do we have? I think we could:

- Only enable dependency validation for internal builds where USE_WORKSPACE is defined. This would give Apple some basic verification.

- Disable dependency validation in script phases. I think this could be done using some xcconfig hacks. It would provide validation of generated headers, IDLs, etc., but not script inputs.

These options are not mutually exclusive: we could disable script phase validation externally, but keep it on for internal builds.

[Elliott Williams]

Created attachment 457296 [details]
Patch

[Elliott Williams]

Comment on attachment 457296 [details]
Patch

Turns out this doesn't actually work to disable validation for script phases only. Dependency validation appears to be determined for the whole target at once, so it's not possible to get it to behave one way for compiler tasks and another way for scripts :(

[Elliott Williams]

Created attachment 457341 [details]
Patch

[Elliott Williams]

Comment on attachment 457341 [details]
Patch

This should do it. Instead of disabling VALIDATE_DEPENDENCIES on _all_ script phases, take advantage of how all our generated xcfilelist scripts are in their own aggregate targets, and disable validation in anything that's not a native target.

Native-target-only validation addresses all open-source validation errors except for the PGO profdata-copying script in some projects. That script can easily be rewritten to emit discovered dependencies after we turn on XCBuild everywhere (https://bugs.webkit.org/show_bug.cgi?id=238916) but before we switch to workspace builds.

[Alexey Proskuryakov]

Comment on attachment 457341 [details]
Patch

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

> Tools/Scripts/set-webkit-configuration:-100
> -if ((!$configuration && !$architecture && !$enableASAN && !$disableASAN && !$enableCoverage && !$disableCoverage && !$enableTSAN && !$disableTSAN && !$enableUBSAN && !$disableUBSAN && !$ltoMode && !$forceOptimizationLevel)

Not sure why this is removed, this doesn't appear to be mentioned ChangeLog.

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #16)
> Comment on attachment 457341 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457341&action=review
> 
> > Tools/Scripts/set-webkit-configuration:-100
> > -if ((!$configuration && !$architecture && !$enableASAN && !$disableASAN && !$enableCoverage && !$disableCoverage && !$enableTSAN && !$disableTSAN && !$enableUBSAN && !$disableUBSAN && !$ltoMode && !$forceOptimizationLevel)
> 
> Not sure why this is removed, this doesn't appear to be mentioned ChangeLog.


This is hinted at in the previous ChangeLog of the patch which was reverted, so it's further down in the file:

    When run with no arguments, checks
    the base product directory, prints the configuration, and exits.
    Recognizes -h and --help flags to show usage.

It's removed so that running set-webkit-configuration with no arguments doesn't print usage and exit 1. The check for `-h` and `--help` arguments is moved up to L78.

[EWS]

Committed r292791 (249574@main): <https://commits.webkit.org/249574@main>

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