-
Notifications
You must be signed in to change notification settings - Fork 208
[Unix] Go back to only checking the runtime resource path for swiftrt.o #1822
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
base: main
Are you sure you want to change the base?
Conversation
Ready for review along with the required linked frontend pull, which passed the linux CI when tested with this pull (this pull has no effect on Darwin and Windows). Comments should mostly go there, since this is just a consequence of that pull. |
@swift-ci please test |
Alright, this is ready to go, a partial revert of #1667, independent of the linked changes in swiftlang/swift#79621. This pull alone passed CI, along with a full Windows toolchain build. I'd like to get this into 6.1 also next. Some background: the prior change checked the As for the original cross-compilation model referenced in that doc, which is still the one commonly used, this change I'm reverting often breaks builds because @artemcm, please review. @compnerd, let us know what you think. |
Ping @artemcm, mind approving this? I'd like to get it into 6.1 next and I don't think Saleem will respond. Ever since I argued against this change in the linked pull last year, I have occasionally raised the problems that this caused, eg swiftlang/swift#76381, and got no response. I think they were probably experimenting with new SDK layouts at the time, tried this hack, then moved on to other things. Let's fix this now, otherwise it is going to start breaking 6.1 SDK bundles and toolchain builds on Unix, ie the two main scenarios where an |
@etcwilde, mind reviewing this? I suspect I'm not going to get a response from those above, since they approved the original pull that broke this, and I'd like to get this fixed before it finally ships in 6.1. |
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.
swiftrt.o is associated with the SDK content though so that the sections are set up correctly for the implementation of swift_addNewDSOImage
coming from the stdlib.
During the hybrid phase, this means that we should be grabbing swiftrt from the same place that we found libswiftCore (either via the sdk or resource-dir), though for expedience I'm fine with not hooking that exact logic up today.
As a compromise, can we continue to search the SDK first, and if it's not found there, fall back on the resource-dir?
"SDK" can mean different things under the different cross-compilation models, so I try to be more specific. Most compilation does not set the
The problem with this previous Unix change for most cross-compilation SDK bundles is that they use the old model of As for searching both, the problem is it might find a |
Ping @etcwilde or @artemcm, I think this should be merged before the 6.1 release, as SDK bundles and building the Swift toolchain itself with Swift 6.1 is going to start breaking on non-Darwin Unix otherwise, as shown in swiftlang/swift#76381. I get these runtime resources from an external |
@shahmishal, could you take a look at this, as it's been a couple weeks and I haven't really been able to get a review here? I tried building the 6.1 compiler with a prebuilt 6.1 on linux to see if the change this is reverting broke anything and it didn't, as the compiler build is careful to use clang for linking Swift executables instead of swift-driver, but the moment that changes, this previous change that I'm reverting will break the compiler build. It has already broken cross-compiling with SDKs on my Android CI with the I'm asking you since you're the review manager for linux and I'd like to get this into 6.1 next. |
@al45tair, I know that this will affect the static linux SDK, do you have feedback?
This is true. But we are also moving away from the model where the SDK content is shoved in the compiler resource directory. @compnerd, you were driving the push to split the system C sysroot flag from the SDK flag. What is missing in |
Essentially nothing has been done to make that move, other than this change looking for a single file alone, which means currently under your new model, the 6.1 compiler always looks in two different directories for Swift runtime resources like
Yes, the invocation in swiftlang/swift#76381 is using the current |
Sigh, with all the delays in reviewing this, 6.1 has now shipped with this regression, so we'll have to fix it in a patch release. |
@swift-ci test |
@swift-ci test windows |
Forced another Windows CI run because the prior one was not registering with github, which means it would have been useless for the merge. |
@etcwilde and @compnerd, this previous change I'm reverting broke compiling Unix SDK bundles using external platform C/C++ SDKs with the Can we go ahead and get this in now? |
This is the final blocker for being able to have the Swift Android SDK use a completely external NDK sysroot without any gruesome hacks. Is there hope for getting this PR in? Or, at least, are there any objections or concerns that we can assuage? There were no responses from @al45tair or @compnerd to @etcwilde's inquiry about whether this might affect the existing SDKs — perhaps they could take another look and offer their thoughts? |
@swift-ci test |
This issue is breaking tests on the community Android CI:
@compnerd, if you think you need this change being reverted for the Android SDKs in your Windows toolchain, can you apply this patch and let us know what error you're seeing? @etcwilde, there is a bug in this new But by hacking this in explicitly here, he has broken the build of those who do specify both It may be possible to reconcile all this with additional info, as I just asked from Saleem, but we've gotten silence on the matter since it was merged last summer in #1667, despite my repeatedly raising the issue since then. If someone needs that change, they should be able to explain why. If they cannot or just aren't using these Android SDKs anymore, we should merge this and go back to the way things were. Pinging @shahmishal too, as this is breaking the upcoming Android SDK bundle with an external NDK, and this pull has received no real review for the last four months. |
@swift-ci test windows |
Ping @etcwilde, can we go ahead and get this in? Given the lack of response from @compnerd for months now, I doubt this matters to him, and if this pull breaks their CI, I have a proper fix queued up to go in swiftlang/swift#79621. To summarize, the way this was done breaks cross-compilation to Unix platforms where the C/C++ SDK and Swift resource directory are separate, which will be the case for all proprietary C/C++ SDKs like the Android NDK, because it requires that an explicit If applying this revert breaks some external CI- it works with all official CI- a version of swiftlang/swift#79621 is the correct way to fix that, while still maintaining the proper precedence. |
This definitely will break our CI. Note that we package up the registrar only in the SDK by design. |
@compnerd, OK, can you apply this pull in a branch and let us know what error you see? My goal is to keep your CI working while not breaking other valid cross-compilation scenarios, as the current swift-driver does. If you have one of the TBC people work with us, I think we can get a version of swiftlang/swift#79621, perhaps scoped only to Unix, to accomodate both your and others' needs. |
@compnerd We need this to move forward; it is blocking progress on the Android SDK. This pull has been languishing for 5 months now. I believe everyone agrees that @finagolfin's observations and suggested solution is correct (@etcwilde, agree/disagree?). If it truly breaks Windows CI, then isn't that something you can fix? If not, would you accept a stopgap measure where we guard this change in |
I disagree. I'm fine with windows continuing to use the right path for the cross compilation. The registrar is meant to be in the SDK, not the resource directory. We should be moving the other way - moving out of the world where we go to the resource directory. Code that is shipped to the client should reside in the SDK. |
I'll see if I can get the linked draft frontend pull swiftlang/swift#79621 to resolve both your concerns and mine, will let you know later. |
I ran a local build with this PR and tried to compile a CMake project with the resulting toolchain. This is the error I got:
Specifically, the issue is here:
This file is not there in the installation, it is in |
Thanks @Steelskin, would it be possible to apply this pull on your CI, that @compnerd says this would break, and let me know what error that shows? This error you've shown is a bit unrelated to this pull, as you'll notice that the Swift command that CMake ran did not pass a Regardless of your example, let me emphasize again that this swift-driver has a long-standing bug where it does not look in an explicit So this discussion is really all about which of Saleem's two different earlier approaches we should now use! 😄 Let me work on that frontend pull this week and once it passes the official CI, we can check if it fixes your TBC issues also. |
@finagolfin I don't have a strong objection to the frontend approach. However, I do wonder if we need the frontend to learn about this. The driver is the appropriate place to know the registrar is. This pattern also matches what clang does for the C runtime initializers. |
Great, 😄 let me implement the frontend approach fully, then we can compare. Regarding whether the frontend is the right place to put this registrar logic, this swift-driver already delegates to the frontend to calculate the correct runtime resource and other target compilation paths, centralizing that logic there: that is the main reason I'm putting this That has its own benefits, like the frontend easily being able to find platform runtime modules in an explicit |
Here is the result from our CI run with your PR. The toolchain build succeeds but the Android smoke tests fails:
|
Thanks @Steelskin, since that failing path is not passed in to your SwiftPM invocation, I assume your Swift compiler is installed at Let me clean up the frontend pull and I will let you know when it is ready to try. |
This is made possible by the swift-frontend now setting this instead in swiftlang/swift#79621. More changes incoming...