Correct VFS libc modulemap injection condition. #60265
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @egorzhdan |
finagolfin
approved these changes
Jul 28, 2022
|
@swift-ci please smoke test |
|
@swift-ci please smoke test macOS |
|
Heh, looks like this pull broke two PS4 tests: 🤣 |
3405691582
force-pushed
the
CorrectVFSExclusion
branch
3 times, most recently
from
488e7a6 to
e55300c
Compare
|
Alright, this should pass the CI now. |
|
Yep, that was a rather astonishing test failure. |
|
Apparently Saleem added some PS4 support in #3221 a little more than six years ago: I had no idea that pull existed. |
|
Ping. |
egorzhdan
reviewed
Aug 8, 2022
There was a problem hiding this comment.
Sorry for the delay!
I think the condition should remain in the form of "what OSes we want to apply this to" and not "what OSes do we skip the injection", otherwise someone porting Swift to a new platform in the future will encounter a very surprising behavior.
Can you do something like triple.isGlibc() || triple.isOpenBSD() || triple.isFreeBSD()?
3405691582
force-pushed
the
CorrectVFSExclusion
branch
from
e55300c to
9457f40
Compare
|
I have factored this a little more for clarity with some of your suggestion. If this LGTY as-is, then good, but I'm still thinking about the problem where a porter for a new platform comes across missing modulemap errors during compilation, but aren't immediately directed to this point in the code and to the decision they need to make. However, the way to solve this is to force an |
|
@swift-ci please smoke test |
finagolfin
reviewed
Aug 17, 2022
| @@ -171,11 +171,15 @@ createClangArgs(const ASTContext &ctx, clang::driver::Driver &clangDriver) { | |||
| return clangDriverArgs; | |||
| } | |||
|
|
|||
| static bool shouldInjectGlibcModulemap(const llvm::Triple &triple) | |||
| { | |||
| return triple.isOSGlibc() || triple.isOSOpenBSD() || triple.isOSFreeBSD(); | |||
There was a problem hiding this comment.
This will break the native Android build, please add Android here and maybe a comment explaining why we're adding these non-glibc platforms.
There was a problem hiding this comment.
Done; can you pick and retest?
3405691582
force-pushed
the
CorrectVFSExclusion
branch
from
9457f40 to
7bb61cc
Compare
egorzhdan
reviewed
Aug 17, 2022
We should only be skipping VFS libc modulemap injection on Darwin and Windows. Unfortunately, we cannot just use the isOSGlibc Triple method because LLVM's sense of Glibc (does the platform _actually use_ glibc) differs from Swift's sense of Glibc (does the platform use libc). There may be other non-libc platforms that we should skip VFS injection on but let's correct the conditional for now and other prs should add those platforms as necessary.
3405691582
force-pushed
the
CorrectVFSExclusion
branch
from
7bb61cc to
6a55161
Compare
|
@swift-ci please smoke test |
finagolfin
approved these changes
Aug 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We should only be skipping VFS libc modulemap injection on Darwin and
Windows. Unfortunately, we cannot use the isOSGlibc Triple method because
LLVM's sense of Glibc (does the platform actually use glibc) differs
from Swift's sense of Glibc (does the platform use libc).
There may be other non-libc platforms that we should skip VFS injection
on but let's correct the conditional for now and other prs should add
those platforms as necessary.