-
Notifications
You must be signed in to change notification settings - Fork 69
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
Local pod support and Swift lib's visibility update #107
Conversation
…ed generate_module_map to be defaulted to false.
Thanks for your contribution! Sorry for the late review, mind updating and getting CI to pass? |
@rahul-malik Updated the tests with the swift lib changes. |
I've faced with swift lib's visibility issue. This MR is fixing the issue. @rahul-malik if everything is ok would you mind to merge it? |
I also have faced with swift lib's visibility issue. |
// If using an empty filepath then no need to look for it | ||
if !firstPart.isEmpty { | ||
do { | ||
directories = try fileManager.subpathsOfDirectory(atPath: firstPart).compactMap { subpath in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also hit a few cases where this starts to fail, I'm not sure what the behavior was before - e.g. was using "" equal to using "."?
@@ -184,8 +198,8 @@ pod_repo_ = repository_rule( | |||
"install_script_tpl": attr.string(), | |||
"inhibit_warnings": attr.bool(default=False, mandatory=True), | |||
"trace": attr.bool(default=False, mandatory=True), | |||
"enable_modules": attr.bool(default=True, mandatory=True), | |||
"generate_module_map": attr.bool(default=True, mandatory=True), | |||
"enable_modules": attr.bool(default=False, mandatory=True), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should turn these off by default, it's an internal convention that we default them to on, and it's probably annoying since they don't work so well. We should update the update_pods.py
/ Vendorizing script to have these defaults as well - that could be a followup
Hello,
Currently to use local pod urls, an absolute path is required rather than one relative to the Bazel sandbox. So this PR's goal is to allow for the use of local pod urls.
The reasoning behind this is that my team is in the process of converting over to using Bazel and we are using PodToBUILD to convert over our cocoapods dependencies. We are a RN app and are using the local podspecs/source found within the node_modules folders for RN. We wanted to be able to use those local pods to maintain the same version of RN easily without having to update multiple places during version updates.
Another thing that was changed was I made "generate_module_map" defaulted to false. When compiling the Yoga pod there was errors around not being able to find standard c++ libs like . I believe it was around this changed made around April around module maps bazelbuild/bazel#7944
Along with that I fixed an issue where the Swift lib's weren't set to a public visibility and fixed a crash if an empty file path is searched for.