[test] Check that the C/C++ module maps are looked for in -resource-dir before -sdk
#76119
Conversation
finagolfin
requested review from
zoecarver,
hyp,
egorzhdan,
beccadax and
ian-twilightcoder
as code owners
| // RUN: %swift %s -typecheck -parse-stdlib -dump-clang-diagnostics -target armv7-unknown-linux-gnueabihf -sdk %t/sdk -resource-dir %t/resources 2>&1 | %FileCheck -check-prefix=CHECK-LINUX %s | ||
|
|
||
| // RUN: cp %S/../../stdlib/public/Cxx/{cxxshim/libcxxshim.modulemap,libstdcxx/libstdcxx.h,libstdcxx/libstdcxx.modulemap} %t/resources/linux | ||
| // RUN: if [ %target-os == "linux-gnu" ]; then %swift %s -typecheck -parse-stdlib -dump-clang-diagnostics -resource-dir %t/resources -cxx-interoperability-mode=default 2>&1 | %FileCheck -check-prefix=CHECK-LINUX-CXX %s; fi |
There was a problem hiding this comment.
I tried to make this linux C++ Interop test run natively on other platforms, including mocking up some C++ headers, but it apparently requires some separate gcc installation also, and I didn't want to mock that too. Simply passing in include paths with -Xcc without that gcc directory, which is actually unused for includes but seemingly required, wouldn't work, no matter what I tried.
So I have this test only run on linux with %target-os and check that it's correctly redirecting these module maps to the system default -sdk / instead. Technically, it requires that the compiler is able to access the C++ headers, so it really requires the host OS to be linux, but since nobody is likely running the linux tests on other platforms, it should work the same. If wanted, I can split it off into a separate linux-only test file instead.
| // CHECK-LINUX-CXX: Currently libstdc++ does not have a module map. To work around | ||
| // CHECK-LINUX-CXX-NEXT: this, Swift provides its own module map for libstdc++. | ||
| // CHECK-LINUX-CXX: header "libstdcxx.h" | ||
| // CHECK-LINUX-CXX: clang importer driver args: {{.*}}'-fmodule-map-file={{.*}}resources/linux/libcxxshim.modulemap' |
There was a problem hiding this comment.
I was surprised this was used with libstdc++, but this libcxxshim.modulemap isn't in the static linux Musl SDK that ships libc++.
@al45tair, is this needed for all C++ stdlibs, and if so, why doesn't your Musl SDK ship it?
There was a problem hiding this comment.
libc++ has its own module maps already; it shouldn't need shimming.
There was a problem hiding this comment.
Ah sorry I missed this comment. These shims are used with all C++ stdlibs that we support, the lack of them can cause compiler crashes.
There was a problem hiding this comment.
Ah, interesting. In which case we probably need libcxxshim.modulemap after all.
|
@swift-ci please test |
|
Updated to fix another test that was failing on the linux CI because of the new diagnostic output, and rebased. |
|
@finagolfin this is not an issue I would have encountered, but since I am set up for Android on Windows builds I will try to repro today and see how far I get. |
No luck-- I just run into other build issues (same as we're facing in our CI right now). @hyp do you have a working setup to build swift-firebase for an Android target on Windows? |
|
Updated the test to use Windows path separators too, as I now see other tests doing, and rebased again. This should pass the CI now, ready for review. |
|
@swift-ci please test |
…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.
|
Sigh, bash logic doesn't work on the Windows CI: Looked at other tests and found a better way to make that test linux-only: have it check for two prefixes, one that works on all platforms and the other using Linux and Mac CI passed, this last change should get this passing on Windows too finally. |
|
@swift-ci please test |
|
Passed CI, ready for review and merge. |
|
@compnerd, get a chance yet to try that updated module map pull, which was failing with your Android SDK before? This pull tests that and makes sure it's working. |
|
@hyp, I notice Saleem hasn't been as active on github this month: can you fill us in on where you guys at TBC are with your CI and trying the module map pull this is testing with your Android SDK? |
|
@egorzhdan, I think you can go ahead and merge: the BC people don't seem too active on github lately and we can always revert both the underlying pull and this test if needed. |
|
Ping @egorzhdan, been a month, don't think you're getting a response, might as well get this in, as it's easy to revert if needed. |
|
Thanks Egor, been meaning to add those extra diagnostics for awhile, should come in handy when debugging C/C++ issues. |
as changed in #74814
Also, dump the module map paths when
-Xfrontend -dump-clang-diagnosticsis 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.Currently, the
ClangImportersilently adds most of these module maps to the build and there's no way to check that on the commandline.@egorzhdan, the test I've been saying I'd write, please review.
@andrurogerz, I see you're building Android on Windows for TBC: has the linked pull caused any issues for you, as reported in those comments before?