This repository was archived by the owner on Nov 10, 2023. It is now read-only.
Allow apple_library to be imported as a module #983
Closed
Conversation
This file contains 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
|
@nguyentruongtho updated the pull request - view changes |
|
@ryu2 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@nguyentruongtho This failure seems related, can you check? |
|
@nguyentruongtho this is great, thanks for doing. I have tested interop with this here - https://github.com/stowy/buck_swift_test/tree/moduleTests - check the readme. I have found that swift does not recognize objC files in the module, unless they are specifically included in the modules bridging header. I have also not been able to have external modules (eg a test module) reference an exported modules swift class from objc. Are these results expected? |
Yes, this is how module works, your headers need to be in exported header (public headers) for other module to use it. We are currently generate umbrella header for all exported headers. In the future, I want to add the ability to use a manually created umbrella header.
This will come in follow on PR. |
|
@nguyentruongtho updated the pull request - view changes - changes since last import |
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
|
My gos, problem with Windows again. |
|
Thanks Tho, just to clarify about the reading objC from swift though, I meant that it will not read any objC classes in swift, even if those classes are in exported headers in the generated umbrella header. They will only be recognized if the headers are included in the objc-swift bridging header of the apple-library, which is unrelated to the module umbrella header. Good news about the swift-objc interior, thanks. |
|
@nguyentruongtho updated the pull request - view changes - changes since last import |
|
@stowy sure, I'll test your sample project once I get a CI pass for this PR 😄 |
|
No problems. Not a big issue with this PR, but definitely I would like to Good luck with CI 😝
|
|
I'm still getting a failure in |
|
@ryu2 you mean facebook internal CI or on your machine? what is the error message and traces? It is passing for me locally and also passed 2 CI checks with Travis & CircleCI. I'm in blind now, could you have a look at it? |
|
@nguyentruongtho On my machine and the internal CI. I'm running Xcode 8.1, how about you? This might have something to do with it. |
|
|
ok, we are having issue with 8.1 internally, so we can't upgrade to use it yet. I'll download it later once I get to my office today. Still, it'd be helpful to know the error message that you are getting. |
|
@nguyentruongtho updated the pull request - view changes - changes since last import |
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
|
@facebook-github-bot shipit |
facebook-github-bot
added
GH Review: accepted
Import Started
and removed
GH Review: needs-revision
labels
facebook-github-bot
pushed a commit
that referenced
this pull request
Nov 11, 2016
Summary: This reverts #911, with some minor fixes after merging. The reason is here: #983 (comment) /cc ryu2 Closes #1007 Reviewed By: ryu2 fbshipit-source-id: 98e69f5
ryu2
suggested changes
Nov 12, 2016
|
any output that I can see, /cc @mzlee if you find a reproduceable steps. |
|
https://gist.github.com/ryu2/4727f513a5064244ef2711f4656c27e5 has the stack trace. It basically can't find a boost header The issue is that |
|
It's an internal app, but you can try to repro it as follows with whatever app you have (not sure of the exact conditions yet needed for this error to occur): From your repo dir (e.g. Rename Then from |
|
@nguyentruongtho updated the pull request - view changes - changes since last import |
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
added 2 commits
November 18, 2016 12:48
Either use umbrella directory or existing umbrella header
|
@nguyentruongtho updated the pull request - view changes - changes since last import |
|
@nguyentruongtho updated the pull request - view changes - changes since last import |
|
Just updated with some architecture changes for creating module map, the bug caught by your internal CI has not yet been addressed. |
daedric
added a commit
to daedric/buck
that referenced
this pull request
Feb 10, 2017
daedric
added a commit
to daedric/buck
that referenced
this pull request
Feb 19, 2017
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 13, 2017
Summary: Split out from #983 in effort of supporting clang module. We create `buck.modulemap` instead of `module.modulemap` for all public header symlink trees with header map. Those module maps will enable us to import a target as module later on. The reason for this special module map naming is that, if module is enabled for a project (`-fmodules`), all `module.modulemap` files in that project's search paths will be pre-compiled and imported implicitly (see https://clang.llvm.org/docs/Modules.html, search for `-fimplicit-module-maps`), which leads to nondeterministic compilation results if we don't handle module cache carefully. Instead of looking into how to handle module cache, we will name module map that created by buck `buck.modulemap`, so that it will not be used implicitly, and will explicitly specified them via [`-fmodule-map-file`](https://clang.llvm.org/docs/Modules.html) in compile commands when needed. Closes #1238 Reviewed By: ryu2 fbshipit-source-id: ae6a02f
|
Will be broken up into smaller PRs. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR will enable Swift target to be able to import apple_library. Quite a few things are imported from #842