[ClangImporter] Make sure the -resource-dir is checked before the -sdk, as done everywhere else in the compiler
#74814
Conversation
finagolfin
requested review from
zoecarver,
hyp,
egorzhdan,
beccadax and
ian-twilightcoder
as code owners
finagolfin
changed the title
-resource-dir is checked before the sdk, as done everywhere else in the compiler-resource-dir is checked before the -sdk, as done everywhere else in the compiler
|
@swift-ci please test |
|
This causes a breakage locally for me due to the modulemaps now not being found. |
Some more info, this is on Windows? Maybe you can paste the failing command. |
|
Yes, on Windows, with an Android SDK being used to build; I'll try to get this soon, I have a different state now to fix a few issues with swift-foundation for android. |
OK, I don't think that breakage is related to this patch, as this patch simply changes the order in which two paths are checked for the module maps, so it won't cause "the modulemaps now not being found," as it's still checking the same two paths. What might be happening is that it's using a different module map with this patch, which might break something depending on your setup. Let us know what you find. Since this passed CI, I will go ahead and add a test, to make sure it doesn't regress. |
|
@finagolfin I applied the patch to my Fedora build and I still get |
|
Thanks for trying it out, @tachoknight, but I'm unclear how you're doing so. I gave a Fedora repro scenario in the last comment of the linked issue where a 5.10.1 system toolchain package is installed and interferes with building a 6.1 stdlib with a prebuilt Swift 6.1 compiler. Since this is a compiler patch, you would have to change that repro command to apply it, and it is not clear how you are doing so. Let me know how you applied this patch and changed the Fedora repro command I gave to use it, and we can figure out if it works or not. |
|
@finagolfin I build Swift for Fedora using my repo here. I applied it to the master branch as that's always the "latest" version of Swift (e.g. 6 currently). Your patch is in the repo as If you'd like to give it a go in a container, all you have to do is create a new container (e.g. |
|
@tachoknight, thanks for the link to the detailed setup. Three questions:
I'd rather not do a full hours-long compiler build on my Fedora VPS to test this, maybe you can look into these issues and try rebuilding if you have faster hardware than me. If you're still seeing the error, I can then kick off a compiler build myself. |
Unsure what you mean here? It passed CI. I will add a test though, so this isn't ready yet. |
There are comments above from a couple of people saying that this patch is breaking their build process – let's make sure that doesn't happen before merging this patch.
Sounds good! |
|
Alright, I built the latest July 8 trunk source snapshot on Fedora 40 x86_64 using the system 5.10.1, and it failed exactly as shown in the linked issue with a prebuilt trunk compiler, but with a freshly-built trunk compiler now by running @tachoknight, I suspect this patch was not applied cleanly in your build, as it should get the Fedora build working again for you too. Let me know what you find. @compnerd, find anything yet? As I said, since this pull only switches the order in which paths are checked, it's not going to cause "the modulemaps now not being found," though it might pick up a different modulemap, depending on your setup. |
|
Rebuilt this on Fedora without the I dumped the libc mappings as I discussed in the linked issue, and found that I also tried cross-compiling the stdlib for Android and running the compiler validation suite in host test mode for Android with this pull, and most tests passed. @hyp, you've been running the compiler validation suite in host test mode for Android: can you try it with this pull and see if any more tests fail? |
|
Okay, rebuilt with this change and got the error logs from what previously would build fine: |
|
That's pretty much useless: you'll want to extract the failing command, then run it manually with |
|
@finagolfin Yes when the patch is applied properly (I got it directly from this PR), it works. Now I'm onto errors further along the build chain, unrelated to your patch. :) |
There was a problem hiding this comment.
This change LGTM. Given that it fixes a build issue a lot of people are reliably hitting on Linux, I don't think we necessarily need a compiler test for this change right now. Let's land this patch this week.
@compnerd are you still seeing a build error with this patch? If you could provide steps to reproduce, I can try reproducing this on my end to get the verbose logs.
|
Fine by me, I was going to try and add a test for this in the coming weeks, but I can always add that after this pull is in. |
|
IIRC, it was simply using the Android SDK on Windows and cross-compiling swift-firebase for Android. There has been a series of regressions in the toolchain which is currently occupying my attention, so I would appreciate it if you could look into it @egorzhdan! |
|
Ah, I missed that this build is targeting Android, I don't have the setup to test it unfortunately. However, from the error message is looks like the libc++ version that's being picked up is somewhat old, and the @compnerd is it possible for you to switch to a newer libc++ that doesn't suffer from this issue? Alternatively, we could temporarily preserve the old search paths ordering for builds targeting Android. |
|
I don't think we should stress about that Android SDK for Windows. They are experimenting with a new layout and compilation flags, unusual changes which are not done on all other Unix platforms currently. If this pull breaks that experimental SDK, it should be fine, as long as it fixes much more stable platforms like linux. |
|
@egorzhdan unfortunately, that is the version of the libc++ that is shipped in the latest Android SDK so we cannot update that. @finagolfin I strongly disagree with that - this was working before this change and breaking it because "its different" is not really acceptable. This currently does work and is not "experimental". It is actively being used by The Browser Company as we are developing Android libraries. |
It works because you added new paths and flags to your local toolchain, that are not used on other Unix platforms, as I detailed on that other pull. I am not against making those changes, as I noted in that linked comment that I actually submitted pulls to change the runtime library path for Unix platforms before that (which was unfortunately rejected by @etcwilde), but you can't expect us to break stable platforms like linux because you want to make breaking changes just for your new Android SDK on Windows. As I said before, we can try to accomodate you, but you will have to give us some info on what you're doing. Have @hyp or somebody track down the failing compilation command with this pull, add As long as you guys don't bother giving us that log and layout info, I don't see why we should support your experimental SDK layout, which is not used on linux. |
|
I am willing to do that, but am currently dealing with a number of recent regressions in the build and trying to finish some of the changes for 6.0 regarding the macros builds for foundation. I can circle back to this once that is complete. |
|
In the meantime, could we have a special case for builds targeting Android to preserve the old ordering, and apply the new ordering in all other cases? That would allow landing this patch sooner, and would unblock several usage scenarios on Linux. |
|
The issue is not Android. The issue is likely the new cross-compilation model laid out here, which they are now implementing only for their Android SDK that they're building on Windows. |
This applies #74814 for all targets except Android. This fixes a common source of build failures on Linux, where shim functions from CxxStdlibShim would not be found by the hosttools Swift compiler when building the CxxStdlib overlay. This change is keeping Android intact to keep the existing Android SDK working.
| result.append(SDKPath.begin(), SDKPath.end()); | ||
| llvm::sys::path::append(result, "usr", "lib", "swift"); | ||
| if (!Opts.RuntimeResourcePath.empty()) { | ||
| result.clear(); |
There was a problem hiding this comment.
This line needs to be in the second if-condition below. Currently the SDK check would fail because the path would be incorrectly constructed.
There was a problem hiding this comment.
Oh, I simply copy-pasted this from below, maybe that's why it was failing for them on Windows? I will update this and maybe they can try again.
There was a problem hiding this comment.
maybe that's why it was failing for them on Windows
Ah, I'm pretty sure that's indeed the reason! 😃
There was a problem hiding this comment.
Alright, rebased and made this change.
@egorzhdan, another CI run would be good.
@compnerd or @hyp, please try this updated pull with this change pointed out by Egor and let us know if it works now.
There was a problem hiding this comment.
@hjyamauchi, maybe you can trigger a Windows build with this pull, if your TBC colleagues are too busy.
There was a problem hiding this comment.
@finagolfin if you mean the TBC CIs, they are still not in a good state and we're working on fixing them, which may need a bit more time :)
There was a problem hiding this comment.
@hjyamauchi, I'm talking about whatever was used to produce the TBC error output above, which we don't see on this CI, could be CI or a local run. If your CI is broken now, maybe you can try a local run?
There was a problem hiding this comment.
I'm not actually sure what he ran there but I'll see if I can do some build
There was a problem hiding this comment.
Thanks @hjyamauchi, that was still a useful signal.
|
I put up another patch #76036 to unblock folks who are seeing compiler errors when building the Swift toolchain on Linux. |
…-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.
|
@swift-ci please test |
|
I think we'll have to land this patch now to unblock hosttools builds on Linux. |
…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.
…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.
…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.
…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.
…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.
…dir` before `-sdk`, as changed in #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.
Otherwise, these module maps can be pulled from a system SDK instead when building a fresh Swift stdlib, fixes #74696.
I saw no regressions when I built the latest June 13 trunk source snapshot natively on Android with this change and it fixed not picking up an older 5.10.1 module map that I had installed in the Termux SDK.
@egorzhdan, if you would run the CI on this, let's make sure it doesn't break anything on other platforms first.