Bug 235744 – [XCBuild] WTF's headers are copied via a script and are invisible to the build system

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

[Elliott Williams]

WTF doesn't use a native Headers phase or Copy Files phase to populate /usr/local/include/wtf. Its "Copy WTF Headers" phase is an rsync script which moves headers into the proper location. This means that the individual headers are invisible to the build system and do not participate in dependency discovery or target ordering, and using WTF headers as inputs (https://commits.webkit.org/246494@main) will not reliably influence build order.

[Radar WebKit Bug Importer]

<rdar://problem/88160652>

[Elliott Williams]

Created attachment 450294 [details]
Patch

[Elliott Williams]

Created attachment 450297 [details]
Patch

[Elliott Williams]

Comment on attachment 450297 [details]
Patch

Removing this from the review queue temporarily while I try something that might dramatically simplify the patch.

[Elliott Williams]

Comment on attachment 450297 [details]
Patch

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

Back to r?

This approach is obtuse. But as far as I can tell, the best way to get what we need (native PbxCp tasks for each header) is to create one copy files build phase per header subdirectory.

Alternatives considered:

1. Copy the entire `wtf/` directory using a copy files phase: This generates the right task information for each header (letting downstream tasks depend on specific headers), but it copies the entire wtf/ directory, including source code. As far as I can tell, there's no way to exclude sources from the copy without having a separate script phase that removes non-headers from /usr/local/include/wtf. I experimented with such a script phase, but it would run redundantly and cause spurious recompilation of source files.

2. Modify the existing rsync-based script to generate input and output xcfilelists: This would give us input/output xcfilelists to provide to the build system, but they would need to be committed since we use them for build task ordering. A proof-of-concept is easy, but to get expected functionality (like forcing a rebuild when the list of headers changes), we'd need to hook this into generate_xcfilelists_lib which would be less trivial.

> Source/WTF/WTF.xcodeproj/project.pbxproj:1314
> +		DD3DC9C127A4C4BD007E5B61 /* spi */ = {isa = PBXFileReference; lastKnownFileType = folder; path = spi; sourceTree = "<group>"; };

The `spi/` headers are a special case: The SPI directory _only_ contains headers, and it's highly nested. It'd be annoying to have separate build phases for each subdirectory within spi/, but since it only has headers we can tell Xcode to copy the whole directory and its contents. Switching to a folder reference removes the PBXFileReferences for each header and replaces it with this reference, which appears as a blue folder in the project navigator.

[Elliott Williams]

Created attachment 450301 [details]
Add native file references for Scripts/

[Alexey Proskuryakov]

Comment on attachment 450301 [details]
Add native file references for Scripts/

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

> we can tell Xcode to copy the whole directory and its contents

Seems a little surprising if Xcode does proper dependency tracking for each file in this case, even though they are in not mentioned in the project individually. Is there a way to confirm that it does?

> This approach is obtuse. But as far as I can tell, the best way to get what we need (native PbxCp tasks for each header) is to create one copy files build phase per header subdirectory.

Indeed. Can you post radar numbers (without other details) of what will let us make it more elegant in the future?

What will WebKit engineers need to know about this approach when adding or removing WTF headers?

I wonder if flattening the hierarchy would be a better solution for now. That would require webkit-dev discussion if proposed, I think.

> Source/WTF/WTF.xcodeproj/project.pbxproj:162
> +		DD3DC85E27A4BF8E007E5B61 /* WeakHashSet.h in Headers */ = {isa = PBXBuildFile; fileRef = 9B67F3F12228D5310030DE9C /* WeakHashSet.h */; settings = {ATTRIBUTES = (Public, ); }; };

Nothing in WTF is public API, is it OK for these headers to have the Public attribute?

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #7)
> > This approach is obtuse. But as far as I can tell, the best way to get what we need (native PbxCp tasks for each header) is to create one copy files build phase per header subdirectory.
> 
> Indeed. Can you post radar numbers (without other details) of what will let
> us make it more elegant in the future?

See rdar://88293015 and rdar://88293050.
 
> What will WebKit engineers need to know about this approach when adding or
> removing WTF headers?

When adding headers to WTF:
- You must add it to WTF.xcodeproj (like you would for a source file).
- You must add it to the copy files phase which corresponds to the directory the header belongs to.
- If you don't, Xcode won't copy the header, and targets which import it will fail to build.

When removing headers to WTF:
- You must remove the header's file reference from WTF.xcodeproj (like you would for a source file).
- If you don't, Xcode will fail to build WTF.


> I wonder if flattening the hierarchy would be a better solution for now.
> That would require webkit-dev discussion if proposed, I think.

One thing that occurred to me while working on this is that I _thought_ WebKit's convention was to have flat header hierarchies. I don't want to stall XCBuild work on this too much, but if you think we could do it without opposition, it totally makes sense to me.

Note WTF has some headers with the same basename, so flattening the hierarchy would mean we'd need to rename them:

    /usr/local/include/wtf/win/SoftLinking.h
    /usr/local/include/wtf/spi/darwin/ProcessMemoryFootprint.h
    /usr/local/include/wtf/cocoa/SoftLinking.h
    /usr/local/include/wtf/linux/ProcessMemoryFootprint.h
    /usr/local/include/wtf/SoftLinking.h


> > Source/WTF/WTF.xcodeproj/project.pbxproj:162
> > +		DD3DC85E27A4BF8E007E5B61 /* WeakHashSet.h in Headers */ = {isa = PBXBuildFile; fileRef = 9B67F3F12228D5310030DE9C /* WeakHashSet.h */; settings = {ATTRIBUTES = (Public, ); }; };
> 
> Nothing in WTF is public API, is it OK for these headers to have the Public
> attribute?

I don't think it matters per se -- this distinction exists only because framework bundles have separate `Headers` and `PrivateHeaders` directories -- but I can switch to marking them "private" for semantic clarity.

[Alexey Proskuryakov]

Comment on attachment 450301 [details]
Add native file references for Scripts/

Sounds like the engineering workflow is straightforward, and there aren't any subtle mistakes to make.

> if you think we could do it without opposition, it totally makes sense to me.

It seems worth checking with webkit-dev to see if there is opposition!

[Elliott Williams]

Created attachment 450566 [details]
Generate xcfilelists for rsync

Here's an alternative approach ("alternative 2" in my comment above) which scripts the creation of xcfilelists based on what rsync transfers. The idea is that we run rsync with `-ii` to generate itemized logs, and pipe the output to `generate-rsync-xcfilelists`. Xcode uses these xcfilelists to figure out when to run rsync (if at all).

The xcfilelists themselves are descriptive enough to get Xcode to re-run rsync when a file is added, so adding/removing headers to WTF will trigger it without project file changes.

I think this *might* be the preferred approach because we have other targets which use rsync, and while it may be viable to move away from rsync in this particular case, I'm not sure about the others. They are:

    JavaScriptCore - testapi
    PAL - Copy PAL Headers
    libwebrtc - Copy webrtc headers

[Elliott Williams]

Created attachment 450575 [details]
Generate xcfilelists for rsync r2

Should fix EWS.

[Jonathan Bedard]

Comment on attachment 450575 [details]
Generate xcfilelists for rsync r2

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

> Source/WTF/ChangeLog:16
> +        * Scripts/generate-rsync-xcfilelists: Added.

It does feel like we should have a better place to put intermediate scripts like this than `Tools/Scripts`, we clearly don't expect engineers to run either of these. But I'm not sure where that would be.

[Elliott Williams]

(In reply to Jonathan Bedard from comment #12)
> It does feel like we should have a better place to put intermediate scripts
> like this than `Tools/Scripts`, we clearly don't expect engineers to run
> either of these. But I'm not sure where that would be.

Perhaps the precedent to follow is to put this script in WTF's project, and copy it into /usr/local/include/wtf/Scripts, like we do with generate-unified-source-bundles.rb. Other targets that need it can use it from there.

[Elliott Williams]

Created attachment 450590 [details]
Generate xcfilelists for rsync r3

Moves the script into WTF (not Tools/Scripts), and copies it into /usr/local/include/wtf/Scripts for other targets to consume.

[Alexey Proskuryakov]

Comment on attachment 450590 [details]
Generate xcfilelists for rsync r3

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

I'm curious if this in particular would be a good fit for pre-build action.

> Source/WTF/Scripts/copy-wtf-headers.sh:14
> +if [[ "${DEPLOYMENT_LOCATION}" == "NO" ]]; then
> +    PRIVATE_HEADERS_PATH="${TARGET_BUILD_DIR%/}/${PRIVATE_HEADERS_FOLDER_PATH}"
> +    ICU_PRIVATE_HEADERS_PATH="${TARGET_BUILD_DIR%/}/${ICU_PRIVATE_HEADERS_FOLDER_PATH}"
> +else
> +    PRIVATE_HEADERS_PATH="${DSTROOT}/${PRIVATE_HEADERS_FOLDER_PATH}"
> +    ICU_PRIVATE_HEADERS_PATH="${DSTROOT}/${ICU_PRIVATE_HEADERS_FOLDER_PATH}"
> +fi;

This makes me wonder if we'll generate different xcfilelists between these cases.

> Source/WTF/WTF.xcodeproj/project.pbxproj:1745
> +				"$(SRCROOT)/CopyWTFHeaders-Scripts-output.xcfilelist",
> +				"$(SRCROOT)/CopyWTFHeaders-wtf-output.xcfilelist",

Modifying source is sad. I understand that we have precedent, but I'd like to understand more about why it's OK - why we can never encounter read-only source, and why this won't cause SCM conflicts on bots as local changes clash with checkouts.

> Source/WTF/WTF.xcodeproj/project.pbxproj:1751
> +			shellScript = "set -eo pipefail\nScripts/copy-wtf-headers.sh\n";

"set -eo pipefail" isn't necessary here.

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #15)
> Comment on attachment 450590 [details]
> Generate xcfilelists for rsync r3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450590&action=review
> 
> I'm curious if this in particular would be a good fit for pre-build action.

Assuming we're talking about scheme actions (the only "pre-build action" I'm aware of in Xcode), I think this is something worth exploring but should not block this change -- there are a lot of design questions I'd have about how best to set this up.

Filed https://bugs.webkit.org/show_bug.cgi?id=236056 to track this.

[Alexey Proskuryakov]

Agreed. I suggested that as a way to sidestep modifying the sources, as we would be always generating an xcfilelist before the build in this world, so it wouldn't need the be checked in at all.

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #15)
> Modifying source is sad. I understand that we have precedent, but I'd like
> to understand more about why it's OK - why we can never encounter read-only
> source, and why this won't cause SCM conflicts on bots as local changes
> clash with checkouts.

It's "OK" because the xcfilelists should not change, absent changes to the script's inputs. The precedent we have for this behavior (generate_xcfilelists_lib) tries to force devs to catch and commit changes, by...

- Failing the build when the xcfilelists _do_ change, so it's not possible for the files to silently get mutated on a CI machine. 
- Checking (and potentially regenerating) xcfilelists in every build, so that regenerations are caught early.

In the status quo, it's possible to send a patch to EWS that regenerates xcfilelists, but that would mean you had _never_ completed a build at your desk first. I'm guessing that doesn't happen often, which is why we don't regularly see source conflicts on CI machines.

--

With this in mind, I think I should reimplement my rsync generator as part of generate_xcfilelists_lib, so that we stick to all the same xcfilelist behaviors and guarantees that we have been living with.

Does that sound reasonable, or do you think I should abandon this and instead land the native build phase approach from the first patch?

[Alexey Proskuryakov]

> - Failing the build when the xcfilelists _do_ change, so it's not possible
> for the files to silently get mutated on a CI machine. 

I do not understand why mutating in CI is impossible. Don't our scripts mutate the source xcfilelists before failing the build?

> - Checking (and potentially regenerating) xcfilelists in every build, so
> that regenerations are caught early.

What are the configurations where we check xcfilelists? I thought that this was disabled in many places.

> In the status quo, it's possible to send a patch to EWS that regenerates
> xcfilelists, but that would mean you had _never_ completed a build at your
> desk first. I'm guessing that doesn't happen often, which is why we don't
> regularly see source conflicts on CI machines.

This explanation seems unlikely to me. People who work on non-Mac platforms would not be building with Xcode at all before EWS. And those who do wouldn't build for all platforms, which can well have different inputs and outputs due to different features being enabled.

Perhaps subtle reasons like these are why we aren't hitting problems all the time, but this means that we are walking a very thin line.

> Does that sound reasonable, or do you think I should abandon this and
> instead land the native build phase approach from the first patch?

I still don't feel like I understand the situation enough to confidently suggest what's right. The first patch feels safer at this point FWIW.

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #19)
> > - Failing the build when the xcfilelists _do_ change, so it's not possible
> > for the files to silently get mutated on a CI machine. 
> 
> I do not understand why mutating in CI is impossible. Don't our scripts
> mutate the source xcfilelists before failing the build?

Yes, they do. I didn't mean to suggest that we somehow avoid mutations altogether, only that we avoid silently-successful mutations.


> > - Checking (and potentially regenerating) xcfilelists in every build, so
> > that regenerations are caught early.
> 
> What are the configurations where we check xcfilelists? I thought that this
> was disabled in many places.
> […]
> This explanation seems unlikely to me. People who work on non-Mac platforms
> would not be building with Xcode at all before EWS. And those who do
> wouldn't build for all platforms, which can well have different inputs and
> outputs due to different features being enabled.

It turns out the check is skipped if Xcode is making a production build or using a public SDK: https://github.com/WebKit/WebKit/blob/f370fc201e04319d45a75961233022e1f46d4f5b/Source/JavaScriptCore/Scripts/check-xcfilelists.sh#L4

This is probably a big part of why we don't see infra instability due to the generator, but doesn't really explain how changes from non-Mac contributors eventually make it into xcfilelists.

> Perhaps subtle reasons like these are why we aren't hitting problems all the
> time, but this means that we are walking a very thin line.
>
> […]
> 
> I still don't feel like I understand the situation enough to confidently
> suggest what's right. The first patch feels safer at this point FWIW.

Agreed. I'm going to move forward with the first patch, and plan to revisit xcfilelist generation later. Thank you very much for talking this through!

[Elliott Williams]

Created attachment 451177 [details]
Native build phases

Updated the first patch (native build phases) to incorporate header that's been added recently, and incorporate Alexey's request to mark these as private headers.

I don't think this needs a re-review, so: cq+

[EWS]

Committed r289256 (246940@main): <https://commits.webkit.org/246940@main>

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 236284

[Elliott Williams]

Created attachment 451329 [details]
Relanding: Native copy phase for ICU headers

This broke EWS because we weren't copying headers from icu/unicode/*, which are needed when building from a public SDK.

[Elliott Williams]

Created attachment 451330 [details]
Diff for relanding

For ease of review, here's a diff of the changes made to between https://bugs.webkit.org/attachment.cgi?id=451177 for relanding.

[Alexey Proskuryakov]

Comment on attachment 451329 [details]
Relanding: Native copy phase for ICU headers

I don't understand why EWS was green.

[EWS]

Committed r289490 (247028@main): <https://commits.webkit.org/247028@main>

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 236402

[Elliott Williams]

Created attachment 451472 [details]
Add untracked WTF headers to WTF.xcodeproj

Third time's a charm? Relanding.

I made a debug builds for Mac, iOS, and iOS Simulator, comparing the contents of /usr/local/include/wtf for this patch and trunk. I found some headers which were never added to WTF.xcodeproj. This patch adds:

	wtf/DataMutex.h
	wtf/MainThreadData.h
	wtf/NullTextBreakIterator.h
	wtf/ParallelJobsGeneric.h
	wtf/ParallelJobsOpenMP.h
	wtf/PlatformEnablePlayStation.h
	wtf/text/NullTextBreakIterator.h
	wtf/UniStdExtras.h
	wtf/WindowsExtras.h

There are some headers which were previously being copied that I am still excluding, because they _should_ only be used on non-Apple platforms. Everything from these directories remains untracked:

	wtf/glib
	wtf/linux
	wtf/text/win
	wtf/win

Since EWS has proven untrustworthy, I've made clean Debug builds with an internal SDK to confirm. It'd probably be wise to do a clean build with an external SDK, too. Open to other testing ideas if you have them.

--

> I don't understand why EWS was green.
My theory is that since EWS does not clean between builds, builders might have old headers in /usr/local/include from previous builds. I'm not sure why those stale headers are eventually getting cleaned out, though -- without cleaning between builds, how does EWS handle _any_ build product deletion?

[Alexey Proskuryakov]

> I'm not sure why those stale headers are eventually getting cleaned out

EWS does a clean build on failure.

1. incremental build
2. clean build if that failed
3. clean(?) build without the patch if that failed too, to confirm that it's a new issue introduced by the patch.

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #30)
> > I'm not sure why those stale headers are eventually getting cleaned out
> 
> EWS does a clean build on failure.
> 
> 1. incremental build
> 2. clean build if that failed
> 3. clean(?) build without the patch if that failed too, to confirm that it's
> a new issue introduced by the patch.

Great, so that would mean that when we've landed this before, a builder would fail for an unrelated reason once and then start failing due to our missing headers.

[EWS]

Committed r289583 (247098@main): <https://commits.webkit.org/247098@main>

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 236472

[Elliott Williams]

Created attachment 451640 [details]
Fix install header paths, add SignedPtr.h

Here's another attempt, incorporating <https://bugs.webkit.org/show_bug.cgi?id=236471> and <https://bugs.webkit.org/show_bug.cgi?id=236466>, which were also reverted.

The major new insight is that during `install` builds, the Copy Files phases need to copy to an absolute path, not "Relative to Build Products". To do that, they need to be based off a separate path variable than the main Headers phase, so that they still copy relative to build products during engineering builds.

[Elliott Williams]

Created attachment 451641 [details]
Diff for relanding

Here's a diff against the last landed patch <https://bugs.webkit.org/attachment.cgi?id=451472>

[EWS]

Committed r289644 (247148@main): <https://commits.webkit.org/247148@main>

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