Have the frontend and new swift-driver look in an external -sdk for non-Darwin platform runtime libraries and modules too
#79621
Conversation
|
@swift-ci please test |
|
swiftlang/swift-driver#1822 |
1 similar comment
|
swiftlang/swift-driver#1822 |
lib/Frontend/CompilerInvocation.cpp
Outdated
| FrontendOpts.UseSharedResourceFolder ? "swift" : "swift_static", | ||
| getPlatformNameForTriple(Triple)); | ||
| // Check for eg <sdkRoot>/usr/lib/swift/linux/ | ||
| if (llvm::sys::fs::exists(SDKResourcePath)) { |
There was a problem hiding this comment.
I initially tried simply checking if <sdkRoot>/usr/lib/swift/, ie a Swift resource directory, existed, but around 100 tests from the compiler validation suite failed with that, as they pass in a separate -sdk that simply has a Swift resource directory with a few different API notes or something, but does not contain the platform-specific files needed in <sdkRoot>/usr/lib/swift/<os>/. Actually checking for this OS-specific directory makes sure the Swift resource directory in -sdk contains the required platform-specific modules and libraries, and falls back to the Swift resource directory in the toolchain itself otherwise. However, that implies mixing and matching files from two different Swift resource directories, so maybe it will be better to enforce that only one Swift resource directory is used and modify all those tests instead?
For an example of such mixing already taking place, the Windows CI failed with this pull, right before trying to compile the trunk Foundation macros for the Windows host:
-- Check for working Swift compiler: T:/5/bin/swiftc.exe - broken
CMake Error at C:/Program Files/CMake/share/cmake-3.29/Modules/CMakeTestSwiftCompiler.cmake:40 (message):
The Swift compiler
"T:/5/bin/swiftc.exe"
is not able to compile a simple test program.
It fails with the following output:
Change Dir: 'T:/x86_64-unknown-windows-msvc/FoundationMacros/CMakeFiles/CMakeScratch/TryCompile-6u6j6e'
Run Build Command(s): C:/PROGRA~1/MICROS~2/2022/COMMUN~1/Common7/IDE/COMMON~1/MICROS~1/CMake/Ninja/ninja.exe -v cmTC_bc87d
[1/2][ 50%][0.084s] T:\5\bin\swiftc.exe -target x86_64-unknown-windows-msvc -j 36 -num-threads 36 -c -module-name cmTC_bc87d -sdk T:/toolchains/swift-6.0.3-RELEASE-windows10/LocalApp/Programs/Swift/Platforms/6.0.3/Windows.platform/Developer/SDKs/Windows.sdk -gnone -Xlinker /INCREMENTAL:NO -Xlinker /OPT:REF -Xlinker /OPT:ICF -incremental -output-file-map CMakeFiles\cmTC_bc87d.dir\\output-file-map.json T:\x86_64-unknown-windows-msvc\FoundationMacros\CMakeFiles\CMakeScratch\TryCompile-6u6j6e\main.swift
FAILED: CMakeFiles/cmTC_bc87d.dir/main.swift.obj
T:\5\bin\swiftc.exe -target x86_64-unknown-windows-msvc -j 36 -num-threads 36 -c -module-name cmTC_bc87d -sdk T:/toolchains/swift-6.0.3-RELEASE-windows10/LocalApp/Programs/Swift/Platforms/6.0.3/Windows.platform/Developer/SDKs/Windows.sdk -gnone -Xlinker /INCREMENTAL:NO -Xlinker /OPT:REF -Xlinker /OPT:ICF -incremental -output-file-map CMakeFiles\cmTC_bc87d.dir\\output-file-map.json T:\x86_64-unknown-windows-msvc\FoundationMacros\CMakeFiles\CMakeScratch\TryCompile-6u6j6e\main.swift
<unknown>:0: warning: using (deprecated) legacy driver, Swift installation does not contain swift-driver at: 'C:\Users\swift-ci\jenkins\workspace\swift-PR-windows\build\5\bin\swift-driver-new.exe'
<unknown>:0: warning: option '-incremental' is only supported in swift-driver
<unknown>:0: error: module compiled with Swift 6.0.3 cannot be imported by the Swift 6.2 compiler: T:/toolchains/swift-6.0.3-RELEASE-windows10/LocalApp/Programs/Swift/Platforms/6.0.3/Windows.platform/Developer/SDKs/Windows.sdk\usr\lib\swift\windows\Swift.swiftmodule\x86_64-unknown-windows-msvc.swiftmodule
ninja: build stopped: subcommand failed.
The issue appears to be that they're trying to build the trunk Foundation macros with the freshly-built trunk 6.2 Swift compiler, but passing in a Swift 6.0.3 SDK with -sdk T:/toolchains/swift-6.0.3-RELEASE-windows10/LocalApp/Programs/Swift/Platforms/6.0.3/Windows.platform/Developer/SDKs/Windows.sdk to build it. The trunk Swift 6.2 compiler then quietly ignores that 6.0.3 Swift resource directory and likely uses the 6.2 Swift resource directory next to the toolchain instead. This pull, by enforcing that the Swift resource directory in the 6.0.3 -sdk that is passed in is used because <6.0.3-sdkRoot>/usr/lib/swift/windows/ exists, then fails because the 6.0.3 modules cannot be used with the trunk Swift 6.2 compiler.
@weliveindetail, do you know if that's merely a configuration mistake when building the Foundation and Testing macros on the Windows CI, which should be easily fixed, or something more complicated?
| @@ -2258,6 +2259,18 @@ static bool ParseSearchPathArgs(SearchPathOptions &Opts, ArgList &Args, | |||
|
|
|||
| if (const Arg *A = Args.getLastArg(OPT_resource_dir)) | |||
| Opts.RuntimeResourcePath = A->getValue(); | |||
| else if (!Triple.isOSDarwin() && Args.hasArg(OPT_sdk)) { | |||
There was a problem hiding this comment.
This code explicitly follows what the original C++ Driver has long done when looking for the Swift runtime libraries, swiftrt.o, and a few other files found in the Swift resource directory:
if (const Arg *A = args.getLastArg(options::OPT_resource_dir)) {
StringRef value = A->getValue();
resourceDirPath.append(value.begin(), value.end());
} else if (!getTriple().isOSDarwin() && args.hasArg(options::OPT_sdk)) {
StringRef value = args.getLastArg(options::OPT_sdk)->getValue();
resourceDirPath.append(value.begin(), value.end());
llvm::sys::path::append(resourceDirPath, "usr");
CompilerInvocation::appendSwiftLibDir(resourceDirPath, shared);
} else {
auto programPath = getDriver().getSwiftProgramPath();
CompilerInvocation::computeRuntimeResourcePathFromExecutablePath(
programPath, shared, resourceDirPath);
}
Note how the SDK is only looked in if a non-Darwin -sdk is explicitly specified: Saleem later tried to expand that to Darwin also in #26361, but he may have never got it to work.
That C++ Driver setup now matches this Frontend setup, because the default in both is now to look relative to the compiler, which is done first in this Frontend here, ie usr/bin/../lib/swift/. If a -resource-dir is set, that is given first priority, then a non-Darwin -sdk is given second priority, ie the C++ Driver and the Frontend now match in where they look.
This is important for two reasons:
- The new swift-driver simply queries the Frontend and uses whatever Swift resource directory it uses, so now the new swift-driver finally matches the original C++ Driver's behavior, and piecemeal workarounds like that in [Unix] Go back to only checking the runtime resource path for swiftrt.o swift-driver#1822 can now be eliminated.
- The Frontend will now look in the same Swift resource directory for stdlib/corelibs swiftmodules as the swift-driver is looking for runtime libraries and
swiftrt.o, eliminating any confusion between the two by centralizing the Swift resource directory lookup here. That already found one bug in the Windows CI, see my other code comment.
However, unlike the C++ Driver, my -sdk code below actually checks if the -sdk path contains a Swift resource directory for the platform triple and does not use the -sdk for this if not, falling back to the aforementioned default next to the compiler in that case. This is because an -sdk is not guaranteed to have a Swift resource directory and may have only a C/C++ sysroot.
We should probably tighten this up to require an explicit -sdk to have a Swift resource directory, with the only exception when an explicit -resource-dir is also specified, but I'm open to debate here. The C++ Driver doesn't even check if the non-Darwin -sdk has a Swift resource directory and simply assumes one is there, we can do a bit better than that.
|
Alright, this tiny pull and the resulting swiftlang/swift-driver#1822 cleanup now pass the linux CI, have no effect on Darwin, and found a seeming bug in the Windows CI. I'm going to test this more in comparison to the C++ Driver and examine more tests to see if I can use it to flush out more cross-compilation bugs with an explicit non-Darwin In the meantime, the basic functionality works and is ready for review. I will add tests once the few remaining smaller design choices mentioned above are hammered out. @artemcm, @etcwilde, and @compnerd, please take a look: I'm looking for feedback on both this current patch and the remaining issues and questions in my detailed comments above. |
|
swiftlang/swift-driver#1827 |
|
swiftlang/swift-driver#1827 |
finagolfin
requested review from
zoecarver,
hyp,
egorzhdan,
beccadax,
ian-twilightcoder,
Xazax-hun,
j-hui and
fahadnayyar
as code owners
finagolfin
removed request for
hyp,
Xazax-hun,
egorzhdan,
zoecarver,
ian-twilightcoder and
fahadnayyar
This needs some tests fixed and a build script update to pass the CI, plus some discussion if the warning and falling back is the best approach. I plan to get that done in the next week.
No, as noted there, "ready to go, a partial revert of swiftlang/swift-driver#1667, independent of the linked changes in #79621." That swift-driver pull alone fixes the bug you've run into with using external C/C++ SDKs with SDK bundles, you don't need this one. This pull simply tries to implement setting the location of the Swift resource path correctly, but given how buggy that has been in the past, warrants some discussion on how we want to finally approach it. |
|
@swift-ci smoke test |
|
@swift-ci smoke test linux |
|
@swift-ci smoke test |
1 similar comment
|
@swift-ci smoke test |
|
@swift-ci smoke test |
|
@swift-ci smoke test |
|
@swift-ci smoke test macos |
|
@swift-ci smoke test macos |
|
@swift-ci smoke test windows |
|
@swift-ci smoke test |
|
@compnerd, getting close to the finish line with this pull, with the macOS CI passing and the linux CI almost there, looking into those last few test failures. However, the Windows CI fails when building DynamicFoundation, because it now gets confused between the Dispatch in the prebuilt Windows SDK and the freshly built one: The failing command is with the Since a fresh Swift SDK build on the CI doesn't need an old Windows.sdk's Swift runtime resources to build fresh corelibs, should |
|
No, |
|
OK, do you need any C/C++ headers or libraries from that |
|
@swift-ci smoke test linux |
… non-Darwin platform runtime libraries and modules too as done originally in swiftlang#25990 with the legacy C++ Driver, but since lost in the new swift-driver. Only difference is this checks if a Swift resource directory exists in `-sdk` and falls back to the default if not.
|
@swift-ci test |
|
Finally passed linux CI, but one mac test now regressed and still need to fix Windows build. |
as done originally in #25990 with the legacy C++ Driver, but since lost in the new swift-driver
This is another attempt to fix swiftlang/swift-driver#1562 and avoid the various workarounds that have been put in elsewhere to spackle over this root cause.
This worked for me with the compiler validation suite running natively on an Android device, with the exception of one SourceKit test. Let's run it through all the platforms on the CI before explaining fully what it's doing and adding a test to make sure it doesn't regress.