[6.0][ClangImporter] Make sure the -resource-dir is checked before the -sdk, as done everywhere else in the compiler
#76199
Conversation
…-sdk`, as done everywhere else in the compiler Otherwise, these module maps can be pulled from a system SDK instead when building a fresh Swift stdlib, fixes swiftlang#74696.
…dir` before `-sdk`, as changed in swiftlang#74814 Also, dump the module map paths when `-Xfrontend -dump-clang-diagnostics` is passed in, both so we can check that manually and in these tests, and fix another Frontend test to be more specific now that this other output trips it up.
Such as?
Ideally, we'd get it into Swift 6.0.0 also, once it's accepted into this branch. |
I think the answer is that we don't know, but past experience has shown that changing search paths can break configurations that we hadn't heard about. This does look like the right set of changes to make, but I'm somewhat concerned that the issue @compnerd hit with #74814 isn't known to be resolved. |
OK, I looked into it and Saleem added this first SDK lookup five years ago in 370aa9d, precisely to look for these module maps in full SDKs that are causing the problems now. I think this was to paper over the frontend not looking for Swift modules in full SDKs altogether, as I reported earlier this year in swiftlang/swift-driver#1562, and @artemcm's Frontend pull for that is how it should've been fixed instead. I don't think most compilation scenarios set the
That was with an earlier version of this patch, but since this merged version was made available a couple weeks ago, their CI has been broken or they have been traveling, so we have not received any notice on this patch. I suspect this patch works fine, but as I said above, I'm still waiting on them to verify. |
|
@shahmishal, we're going on three weeks with no response here, can we get this in? I suspect the reason Saleem had an issue with an earlier version of this patch is that I made a mistake initially, which Egor then found and we corrected. This has been in trunk for a month now without any reported issues and distros like Fedora have to pull it in for their 6.0 build. |
|
@swift-ci please test |
|
Ping @shahmishal or @DougGregor, this passed the 6.0 CI, just need a decision now. |
|
Ping, this has been in trunk for four months now and in the downstream 6.0 release of Fedora for the last month: I think we have a pretty good body of evidence that it is unlikely to break anything. |
|
Ping @DougGregor, with the next patch release coming up, can you make a decision on this? I consider this a small fix for more advanced build scenarios, but I leave it up to you release managers if it's worth backporting. |
This bug caused me a lot of hours to be able to make a build for NixOS |
Explanation: The frontend currently looks in
-sdkfor a C/C++ sysroot and-resource-dirfor Swift modules, with the former implicitly set to/and the latter always explicitly passed in byswift-driver. However, a "full SDK" contains both C/C++ headers and Swift modules, so in that case and if-sdkis explicitly passed in, thisClangImporterwas looking in the full SDK first and wrongly using those Swift module maps, rather than the ones in-resource-dir.Scope: When does that actually happen? Suppose you have Swift 5.10 installed in the system and are trying to build Swift 6.0 from source. In that scenario, the CMake config explicitly passes
-sdk /and the Swift 6 compiler wrongly uses the old Swift 5.10 C and C++ module maps and fails. This could also affect some SDK bundles, say if one linux distro triple's Swift modules are placed in the common SDK itself, while others are put in separate-resource-dirpaths, since compiling SDK bundles passes in an explicit-sdk.It does not affect simply building Swift packages with a local SwiftPM 6.0 while the implicit system SDK path of
/has a system Swift 5.10 installed, as no explicit-sdkis passed to build those packages.Issue: #74696
Original PRs: #74814 and #76119
Risk: Low, since this only affects the few listed scenarios where the
-sdkflag is explicitly passed to the compilerTesting: Passed all CI on trunk and my and others' testing locally. @compnerd did report an issue with his new Android SDK with an earlier version of this patch, so I'll wait on his further testing and approval before asking to merge this.
Reviewer: @egorzhdan