-
Notifications
You must be signed in to change notification settings - Fork 10.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Target Info] Compute resource-directory relative to the SDK on non-Darwin targets #72409
base: main
Are you sure you want to change the base?
Conversation
@swift-ci test |
@swift-ci test macOS platform |
Anything holding this up, @artemcm? The new cross-compilation doc @compnerd wrote up in #73581 is wrong with the new |
@swift-ci test |
6612e9c
to
e260c5d
Compare
…arwin targets Since the new driver uses `-print-target-info` for the resource directory and the legacy C++ driver used the logic in `getResourceDirPath()`, mimic that logic now in the print-target-info job itself.
e260c5d
to
aa9aef4
Compare
@swift-ci test |
@swift-ci Please Build Toolchain Windows Platform |
@swift-ci test |
This doesn't work for Android, where the toolchain contains Swift resources, and SDK has the Swift runtime and swiftrt.o. With this patch, swift is unable to find the Clang builtins headers |
That doesn't make any sense: can you clarify how you are building, ie what paths have what? It sounds like you are using a highly non-standard layout.
I suppose that's possible depending on your layout, but again, we'll need more info. |
The Android SDK is packaged separately from the toolchain in the following way when distributed with our windows toolchain, here are some of our Swift standard library / runtime pieces:
This SDK is passed to our compilation using the -sdk flag (-sdk S:\Program Files\Swift\Platforms\Android.platform\Developer\SDKs\Android.sdk). We expect a file like swiftrt.o and all the SOs to be in the SDK. However, the compiler and it's resource dir, with the clang builtin headers as located in the toolchain, just like the compiler binary itself:
This layout is somewhat similar to Darwin, where the standard library is separated out and is in the SDK as opposed to the toolchain itself (except for things like pre-compiled modules). |
OK, the problem is not that, as other SDKs also have those Swift runtime files. The problem is that you're placing the shared libraries in the arch-specific directory Why are you so particular about that, when you only have a single arch in that directory? I understand if you had multiple arches, but you don't. I'm all for organizing the Swift runtime libraries for multiple arches on Unix platforms like that, as I submitted several pulls to move the Unix toolchain to that last year, #63782, but that approach was rejected, even when I said it could be a temporary measure. The second difference in your non-standard layout is that you don't place the clang builtin headers in the SDK, as I think even Darwin platforms do. I understand that @compnerd wants to separate those out and change the meaning of I think it would be easier if we moved all SDKs to this new cross-compilation model at the same time, or at least all non-Darwin SDKs. Otherwise, we will be stuck special-casing particular SDKs like this with a bunch of driver logic until we move them all over. Try something: apply this pull and remove your patch from swiftlang/swift-driver#1560. Then, move all the |
@finagolfin we only have a single directory there because it takes time to build. The installer will have all the architectures under the same directory, just like Windows already does with i686, x86_64, and aarch64. Also, it is not a single SDK that we are changing to the resource headers being organised this way. Windows has long used this layout and the Android SDK we are building and packaging for Windows uses the same layout. Right now, at least on Windows, there are only 2 SDKs - Windows and Android. I don't have a problem with the other SDKs also using the same layout though. |
OK, makes sense, but what is your plan here? When I suggested a similar organization last year, it was rejected. We shouldn't special-case only the Android SDK on Windows to work that way, but all other Unix platforms don't.
I haven't used the Windows toolchain, but I find this hard to believe based on the common driver logic. For example, when you compile natively for Windows, what is the |
It has to be a gradual roll out. As we start building more of the platform SDKs, we will start fixing that. Right now the build setup makes this complicated to do as a generic, single shot application. As work progresses towards cleaning up the builds, we should be able to do this more readily across all the platforms.
It is set to the expected path - the resource directory under the toolchain. |
IMO, a gradual roll-out will be much more complicated, but I could be wrong.
Thanks, I will try running the Windows toolchain myself to see how it does things differently. @etcwilde, it appears that Darwin, Unix, and Windows currently place all these SDK/resource files in different layouts. If we're going to put together cross-compilation SDK bundles that work on all platforms, we're going to have to come up with a way to regularize all this. Rolling it out gradually with a bunch of special-case logic like in swiftlang/swift-driver#1560, for Android in that case, seems like it's going to get messy to me. You're working on cross-compilation support, what do you think? Pinging @MaxDesiatov too. |
@@ -3552,6 +3576,9 @@ bool CompilerInvocation::parseArgs( | |||
return true; | |||
} | |||
|
|||
if (FrontendOpts.PrintTargetInfo) | |||
setRuntimeResourcePath(computeRuntimeResourcePathForTargetInfo()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just fixes the narrow problem of passing the right path for the Swift core modules back to swift-driver
, but I raised a larger concern in the issue that motivated this pull, swiftlang/swift-driver#1562, that the swift-frontend
is not looking in a passed in -sdk
for those Swift core modules.
Can this pull be broadened to fix that too, especially since @compnerd has proposed looking in -sdk
primarily for those Swift core modules?
Right now, the way it works is that swift-driver
queries swift-frontend
for what paths to use then uses those to set -sdk
and -resource-dir
flags to pass back to the swift-frontend
for compiling new Swift files. During compilation, swift-frontend
only checks -resource-dir
and adjacent to the compiler for the Swift core modules like the stdlib.
I think we should change swift-frontend
to look in -sdk
and adjacent to the compiler instead and stop passing -resource-dir
to the swift-frontend
unless one was specified to swift-driver
. I noted in the linked issue that the legacy C++ Driver worked that way, ie it did not pass -resource-dir
to the swift-frontend
unless one was passed to the Driver first.
This would be in keeping with Saleem's plan to de-emphasize -resource-dir
to look for Swift core modules and look in -sdk
instead.
I think we can move forward with this, indeed expand it as I laid out above. While it may not fix whatever additional issues Alex and Saleem have with their non-standard Android SDK, it does partially fix the Let me know what you all think. |
@compnerd, you want to chime in on what I wrote last week? This pull fixes the issue that the |
Ping @artemcm, been six months since you proposed this fix and nothing has happened for the last couple months. If you don't have time for this, would you like me to take over the fix for the linked swift-driver issue? |
Since the new driver uses
-print-target-info
for the resource directory and the legacy C++ driver used the logic ingetResourceDirPath()
, mimic that logic now in the print-target-info job itself.