[CMake] Remove unused dependency on Foundation build directory

[finagolfin]

See if this helps with swiftlang/swift#79621

[finagolfin]

@swift-ci test

[stmontgomery]

I thought these linker flags were necessary, so if your investigation leads you to believe we should remove them, please elaborate since I don't quite understand all the factors involved here. I skimmed the linked Swift PR but it looks like it's gone in a few different directions so I'm unclear where things stand there, currently.

[finagolfin]

My understanding is that these flags add a dependency to the CMake target, which is in the Foundation build directory, eg foundation-linux-x86_64. However, both build-script and build.ps1 install all prior corelibs to a separate single toolchain or SDK directory before Testing is built, so it isn't necessary to pass in the original build directories too.

Of course, this didn't break anything so it was fine so far, but my linked frontend pull enforces that the compiler looks in the passed in -sdk and the Windows CI builds and uses that SDK early, from the trunk stdlib build onwards, so these flags confuse it when it gets to building Testing:

"C:\\Users\\swift-ci\\jenkins\\workspace\\swift-PR-windows\\build\\5\\bin\\swiftc.exe" -frontend -typecheck-module-from-interface T:/x86_64-unknown-windows-msvc/Testing/swift/Testing.swiftinterface -target x86_64-unknown-windows-msvc -disable-objc-interop -sdk "T:/Program Files/Swift/Platforms/Windows.platform/Developer/SDKs/Windows.sdk" -I "T:\\x86_64-unknown-windows-msvc\\Testing\\swift" -I "C:\\Users\\swift-ci\\jenkins\\workspace\\swift-PR-windows\\swift-testing\\Sources\\_TestingInternals\\include" -I "T:\\x86_64-unknown-windows-msvc\\Dispatch" -I "C:\\Users\\swift-ci\\jenkins\\workspace\\swift-PR-windows\\swift-corelibs-libdispatch" -I "C:\\Users\\swift-ci\\jenkins\\workspace\\swift-PR-windows\\swift-corelibs-libdispatch\\src" -I "T:\\x86_64-unknown-windows-msvc\\Dispatch\\src" -I "C:\\Users\\swift-ci\\jenkins\\workspace\\swift-PR-windows\\swift-corelibs-libdispatch\\src\\BlocksRuntime" -I "C:\\Users\\swift-ci\\jenkins\\workspace\\swift-PR-windows\\swift-foundation\\Sources\\_FoundationCShims\\include" -I "T:\\x86_64-unknown-windows-msvc\\DynamicFoundation\\swift" -I "C:\\Users\\swift-ci\\jenkins\\workspace\\swift-PR-windows\\swift-foundation-icu\\icuSources\\include" -I "T:\\x86_64-unknown-windows-msvc\\Dispatch\\src\\swift\\swift" -enable-library-evolution -gnone -module-link-name Testing -package-name org.swift.testing -swift-version 6 -O -D SWT_NO_FOUNDATION_FILE_COORDINATION -D SWT_NO_SNAPSHOT_TYPES -D Testing_EXPORTS -define-availability "_mangledTypeNameAPI:macOS 11.0, iOS 14.0, watchOS 7.0, tvOS 14.0" -define-availability "_uttypesAPI:macOS 11.0, iOS 14.0, watchOS 7.0, tvOS 14.0" -define-availability "_backtraceAsyncAPI:macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0" -define-availability "_clockAPI:macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0" -define-availability "_regexAPI:macOS 13.0, iOS 16.0, tvOS 16.0, watchOS 9.0" -define-availability "_swiftVersionAPI:macOS 13.0, iOS 16.0, tvOS 16.0, watchOS 9.0" -define-availability "_typedThrowsAPI:macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0" -define-availability "_distantFuture:macOS 99.0, iOS 99.0, watchOS 99.0, tvOS 99.0, visionOS 99.0" -require-explicit-sendable -enable-experimental-feature AccessLevelOnImport -enable-upcoming-feature ExistentialAny -enable-upcoming-feature InternalImportsByDefault -enable-upcoming-feature MemberImportVisibility -enable-upcoming-feature InferIsolatedConformances -load-plugin-library T:/x86_64-unknown-windows-msvc/TestingMacros/TestingMacros.dll -in-process-plugin-server-path "C:\\Users\\swift-ci\\jenkins\\workspace\\swift-PR-windows\\build\\5\\bin\\SwiftInProcPluginServer.dll" -plugin-path "C:\\Users\\swift-ci\\jenkins\\workspace\\swift-PR-windows\\build\\5\\bin" -Xcc -v -Xcc -fmodule-map-file=C:/Users/swift-ci/jenkins/workspace/swift-PR-windows/swift-foundation/Sources/_FoundationCShims/include/module.modulemap -Xcc -fmodule-map-file=C:/Users/swift-ci/jenkins/workspace/swift-PR-windows/swift-foundation-icu/icuSources/include/_foundation_unicode/module.modulemap -autolink-library oldnames -autolink-library msvcrt -Xcc -D_MT -Xcc -D_DLL -module-name Testing
T:\Program Files\Swift\Platforms\Windows.platform\Developer\SDKs\Windows.sdk\usr\lib\swift\_FoundationCShims\module.modulemap:1:8: error: redefinition of module '_FoundationCShims'
1 | module _FoundationCShims {
  |        `- error: redefinition of module '_FoundationCShims'
2 |     header "_FoundationCShims.h"
3 | 

C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift-foundation\Sources\_FoundationCShims\include\module.modulemap:1:8: note: previously defined here
1 | module _FoundationCShims {
  |        `- note: previously defined here
2 |     header "_FoundationCShims.h"
3 | 

T:\Program Files\Swift\Platforms\Windows.platform\Developer\SDKs\Windows.sdk\usr\lib\swift\_foundation_unicode\module.modulemap:1:8: error: redefinition of module '_FoundationICU'
  1 | module _FoundationICU {
    |        `- error: redefinition of module '_FoundationICU'
  2 |     header "translit.h"
  3 |     header "ustdio.h"

C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift-foundation-icu\icuSources\include\_foundation_unicode\module.modulemap:1:8: note: previously defined here
  1 | module _FoundationICU {
    |        `- note: previously defined here
  2 |     header "translit.h"
  3 |     header "ustdio.h"

Note how it gets confused between the SDK path and the module map files from the source directories that these CMake targets are transitively adding.

Basically, the Windows CI was mixing and matching files from the build directories, the source directories, and the final installed SDK directory to build each new core library like Foundation or XCTest. Once my frontend pull makes sure non-Darwin platforms use only the installed SDK directory, these flags broke the Windows CI, because it is the only platform CI that uses that SDK directory very early on with the -sdk flag.

I went ahead and got rid of it for linux and other build-script platforms too, as build-script also installs everything to a toolchain directory before building Testing, so these flags aren't needed, but they don't break the linux CI because it doesn't pass an explicit -sdk flag, like the Windows CI does.

[finagolfin]

Good news is that previously failing command now passed on the Windows CI with my frontend pull, bad news is the CI now can't find Foundation.lib for a subsequent link command. I will iterate on this a bit more and let you all know when it works.

[grynspan]

@compnerd Please review when @finagolfin is ready.

[finagolfin]

This passed the linux CI and has no effect on the mac CI, now to see if I can get it working on Windows.

[finagolfin]

As explained above, these removed lines add CMake dependencies that are unneeded by the current build{-script,.ps1} that we use to build the toolchain, because both now consolidate all prior corelibs into an SDK installation before building this repo.

On the other hand, leaving this in also doesn't break anything anymore, both with my linked pull and obviously for trunk. So it's now just a question of removing superfluous config, will leave that up to @rintaro and @stmontgomery to decide, as they originally added these lines, plus @compnerd's input is welcome.

[grynspan]

Can you run a toolchain build with this PR? If it passes, I have no problem taking the change.

[grynspan]

(I think you ran something but it's been decached already.)

[finagolfin]

Ran the toolchain builds, no problem. 👍

[stmontgomery]

Given the successful toolchain builds this seems fine to me. I'd love for @compnerd to comment as well

[compnerd]

I think that we should prefer to leave this as is. This is ensuring that we are properly declaring the dependencies and thus the build graph. Additionally, when find_package(Foundation) is present, this allows for tracking inter-project dependencies.

CC: @etcwilde

[etcwilde]

I agree with @compnerd that we should not be removing real dependency edges and hoping that people "do the right thing".

This looks like a bug in the swift-foundation and swift-foundation-icu builds.
These two flags are the culprits: -Xcc -fmodule-map-file=C:/Users/swift-ci/jenkins/workspace/swift-PR-windows/swift-foundation/Sources/_FoundationCShims/include/module.modulemap -Xcc -fmodule-map-file=C:/Users/swift-ci/jenkins/workspace/swift-PR-windows/swift-foundation-icu/icuSources/include/_foundation_unicode/module.modulemap
They should only be applied as part of the build interface, but are also being kept in the install interface.

Just posted these two PRs that should fix this issue:

• CMake: modulemap override in build interface swift-foundation#1539
• CMake: modulemap file override as build interface swift-foundation-icu#78

[grynspan]

@finagolfin Given @etcwilde's diagnosis, does it make sense to close this PR?

[finagolfin]

I think that we should prefer to leave this as is. This is ensuring that we are properly declaring the dependencies and thus the build graph. Additionally, when find_package(Foundation) is present, this allows for tracking inter-project dependencies.

Not sure what you mean, are you suggesting that any arbitrary Swift repo that imports Foundation should always explicitly list Foundation in its CMake config?

The whole point of shipping Foundation as a Swift runtime library is that it's assumed that it's there. Since Testing is now always built on the CI only after the platform SDK for non-Darwin platforms has been built and locally installed, these dependencies on the Foundation build directory are useless, which is why all the toolchain builds passed.

This looks like a bug in the swift-foundation and swift-foundation-icu builds.

No idea what this refers to, as this pull does not touch those module maps and everything works with or without this pull.

The reason to remove these lines is because these dependencies are completely unnecessary on the CI, because the host platform SDK is always assembled on the CI before building Testing. That is not the case for XCTest and other corelibs, which is where this dependency was likely copied from, so they need the dependency to find the Foundation build directory to build against. This library doesn't.

[etcwilde]

No idea what this refers to, as this pull does not touch those module maps and everything works with or without this pull.

1 | module _FoundationCShims {
  |        `- error: redefinition of module '_FoundationCShims'
2 |     header "_FoundationCShims.h"
3 | 

C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift-foundation\Sources\_FoundationCShims\include\module.modulemap:1:8: note: previously defined here
1 | module _FoundationCShims {
  |        `- note: previously defined here
2 |     header "_FoundationCShims.h"
3 | 

T:\Program Files\Swift\Platforms\Windows.platform\Developer\SDKs\Windows.sdk\usr\lib\swift\_foundation_unicode\module.modulemap:1:8: error: redefinition of module '_FoundationICU'
  1 | module _FoundationICU {
    |        `- error: redefinition of module '_FoundationICU'
  2 |     header "translit.h"
  3 |     header "ustdio.h"

C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\swift-foundation-icu\icuSources\include\_foundation_unicode\module.modulemap:1:8: note: previously defined here
  1 | module _FoundationICU {
    |        `- note: previously defined here
  2 |     header "translit.h"
  3 |     header "ustdio.h"

The failure you're reporting is because the compiler is seeing re-definitions of the _FoundationICU and _FoundationCShims modules. That's happening because those two projects are including the modulemaps from their source directories. By removing the link, it's not including the interface flags that are exported by swift-foundation.

[finagolfin]

@etcwilde, ah, I see, you are referring to my original comment from two months ago, when I needed this change for Windows alone with my frontend pull, but since then Saleem changed how Testing is built on the Windows CI and this pull, whether it is applied or not, no longer has any effect, as I noted yesterday.

Thanks for those Foundation module map pulls: I don't know enough about that aspect, but if that helps in other ways, great.

Since these dependencies I am removing here have zero effect, as my successful toolchain builds on the CI show, I am proposing removing them.