Skip to content
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

Look for runtime library modules in the SDK, too #23175

Merged
merged 2 commits into from Mar 13, 2019

Conversation

Projects
None yet
3 participants
@brentdax
Copy link
Collaborator

brentdax commented Mar 8, 2019

Once module stability arrives, we'll want to load modules (and particularly parsable interfaces) from the SDK instead of the toolchain. This PR starts to stage that change in.

Resolves rdar://problem/46132288.

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Mar 8, 2019

The WIP commit doesn't have tests yet, but as far as I know it will work.

@brentdax brentdax force-pushed the brentdax:theres-a-path-for-everyone branch from e68c834 to 019f8ba Mar 9, 2019

@brentdax brentdax marked this pull request as ready for review Mar 9, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Mar 9, 2019

@swift-ci please test

@swift-ci

This comment was marked as resolved.

Copy link
Contributor

swift-ci commented Mar 9, 2019

Build failed
Swift Test OS X Platform
Git Sha - e68c834

@swift-ci

This comment was marked as resolved.

Copy link
Contributor

swift-ci commented Mar 9, 2019

Build failed
Swift Test Linux Platform
Git Sha - e68c834

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Mar 9, 2019

This will require a matching change in lldb:

18:24:38 /home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/lldb/source/Symbol/SwiftASTContext.cpp:4762:40: error: no member named 'RuntimeLibraryImportPath' in 'swift::SearchPathOptions'; did you mean 'RuntimeLibraryPath'?
18:24:38       m_ast_context_ap->SearchPathOpts.RuntimeLibraryImportPath.c_str());
18:24:38                                        ^~~~~~~~~~~~~~~~~~~~~~~~
18:24:38                                        RuntimeLibraryPath

brentdax pushed a commit to brentdax/swift-lldb that referenced this pull request Mar 11, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Mar 11, 2019

Please test with following pull request:
apple/swift-lldb#1365

@swift-ci test

@brentdax brentdax requested a review from jrose-apple Mar 11, 2019

@jrose-apple
Copy link
Member

jrose-apple left a comment

Looks pretty good, only minor comments.

Show resolved Hide resolved lib/Frontend/CompilerInvocation.cpp
Show resolved Hide resolved lib/Serialization/SerializedModuleLoader.cpp
Show resolved Hide resolved test/Serialization/runtime-import-from-sdk.swift Outdated
Brent Royal-Gordon
[NFC] Support many runtime library import paths
Replaces SearchPathOptions::RuntimeLibraryImportPath with an equivalent std::vector of paths. Also reimplements SearchPathOptions::SkipRuntimeLibraryImportPaths to cause the list of runtime library import paths to be empty, rather than exiting early from SerializedModuleLoader::findModule().

@brentdax brentdax force-pushed the brentdax:theres-a-path-for-everyone branch from 019f8ba to 09f87d3 Mar 12, 2019

@brentdax brentdax requested a review from jrose-apple Mar 12, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Mar 12, 2019

Please test with following pull request:
apple/swift-lldb#1365

@swift-ci test

@apple apple deleted a comment from swift-ci Mar 12, 2019

@apple apple deleted a comment from swift-ci Mar 12, 2019

@brentdax brentdax force-pushed the brentdax:theres-a-path-for-everyone branch from 09f87d3 to ba09af0 Mar 12, 2019

@brentdax

This comment was marked as outdated.

Copy link
Collaborator Author

brentdax commented Mar 12, 2019

Please test with following pull request:
apple/swift-lldb#1365

@swift-ci please smoke test and merge

@brentdax

This comment was marked as outdated.

Copy link
Collaborator Author

brentdax commented Mar 12, 2019

Please test with following pull request:
apple/swift-lldb#1365

@swift-ci please smoke test and merge

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Mar 12, 2019

Please test with following pull request:
apple/swift-lldb#1365

@swift-ci please smoke test

@brentdax brentdax force-pushed the brentdax:theres-a-path-for-everyone branch from ba09af0 to 9b56c6c Mar 13, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Mar 13, 2019

The new test caused a different error message on Linux than it did on macOS. Tweaked the test to use an empty SDK instead of an SDK with an unloadable standard library; that ought to give the same message on both platforms.

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Mar 13, 2019

Please test with following pull request:
apple/swift-lldb#1365

@swift-ci please smoke test

@jrose-apple

This comment has been minimized.

Copy link
Member

jrose-apple commented Mar 13, 2019

Tweaked the test to use an empty SDK instead of an SDK with an unloadable standard library; that ought to give the same message on both platforms.

Er, but that's no longer testing that we prefer the SDK over the toolchain.

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Mar 13, 2019

Why doesn't Github have 🤦‍♂️ as a reaction?

Brent Royal-Gordon
Load runtime library modules from SDK
With this change, swiftc will still look for standard library and overlay modules in ../lib/swift (relative to the compiler), but if it doesn’t find them there, it will now look in usr/lib/swift in the SDK.

@brentdax brentdax force-pushed the brentdax:theres-a-path-for-everyone branch from 9b56c6c to c177674 Mar 13, 2019

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Mar 13, 2019

@jrose-apple I don't think there's a way to distinguish between "standard library is invalid" and "standard library is missing" on non-Darwin platforms, so I'll go back to the old test design and make it conditional on objc_interop. I filed SR-10097 to improve the errors.

I need to get this tested and merged so it can be cherry-picked, but if you're not happy with where this is at, I can do some follow-up work in another PR.

@brentdax

This comment has been minimized.

Copy link
Collaborator Author

brentdax commented Mar 13, 2019

Please test with following pull request:
apple/swift-lldb#1365

@swift-ci please smoke test

@brentdax brentdax merged commit 21b96d4 into apple:master Mar 13, 2019

2 checks passed

Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details

brentdax added a commit to brentdax/swift that referenced this pull request Mar 13, 2019

Merge pull request apple#23175 from brentdax/theres-a-path-for-everyone
Look for runtime library modules in the SDK, too
@jrose-apple

This comment has been minimized.

Copy link
Member

jrose-apple commented Mar 13, 2019

I don't think there's a way to distinguish between "standard library is invalid" and "standard library is missing" on non-Darwin platforms, so I'll go back to the old test design and make it conditional on objc_interop.

The folder doesn't do that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.