-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Allow app specs to be used to upload to the app store #8275
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
Conversation
lib/cocoapods/installer/xcode/pods_project_generator/pod_target_installer.rb
Show resolved
Hide resolved
Here's an example of your CHANGELOG entry: * Allow app specs to be used to upload to the app store
[segiddins](https://github.com/segiddins)
[#issue_number](https://github.com/CocoaPods/CocoaPods/issues/issue_number) note: There are two invisible spaces after the entry's text. Generated by 🚫 Danger |
lib/cocoapods/installer/xcode/pods_project_generator/pod_target_installer.rb
Show resolved
Hide resolved
2fbe50a
to
37799cd
Compare
Would love a test case that catches the typos and undefined methods from app specs Would also love a test case in which we exercise |
cc @dostrander |
37799cd
to
ff9784e
Compare
41050c2
to
c54b258
Compare
Does this supersede #8432 and we just need to add |
No, I don't think it supersedes #8432 |
@@ -431,7 +484,7 @@ def add_app_targets | |||
# @param [Array<Sandbox::FileAccessor>] file_accessors | |||
# the file accessors list to generate resource bundles for. | |||
# | |||
# @return [Array<PBXNativeTarget] the resource bundle native targets created. | |||
# @return [Array<PBXNativeTarget>] the resource bundle native targets created. |
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.
this was actually wrong. It should be Hash{String => Array<PBXNativeTarget>}]
@@ -473,7 +520,7 @@ def add_resources_bundle_targets(file_accessors) | |||
|
|||
# Set the `SWIFT_VERSION` build setting for resource bundles that could have resources that get | |||
# compiled such as an `xcdatamodeld` file which has 'Swift' as its code generation language. | |||
if contains_compile_phase_refs && target.uses_swift? | |||
if contains_compile_phase_refs && file_accessors.any? { |fa| target.uses_swift_for_spec?(fa.spec) } |
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.
Why would this be any target and not the spec of the file_accessor
we are looping in? Isnt that where the resource bundle is under?
Alternatively I was under the impression from our offline chat that we would always set this without caring whether Swift is needed or not.
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.
Why would this be any target and not the spec of the
file_accessor
we are looping in? Isnt that where the resource bundle is under?
I changed it to do just that
Alternatively I was under the impression from our offline chat that we would always set this without caring whether Swift is needed or not.
Decided not to change that here, on the off chance it would accidentally break something
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.
LGTM. @amorde woud love your approval here as well.
@@ -385,6 +417,7 @@ def add_app_targets | |||
app_native_target = AppHostInstaller.new(sandbox, project, platform, subspec_name, spec_name, | |||
app_target_label, :add_main => false).install! | |||
|
|||
app_native_target.product_reference.name = app_target_label |
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.
wonder if this could move into AppHostInstaller
but meh.
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.
it seems like it should be in AppHostInstaller
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 was just mirroring the way we do it for test specs
@@ -109,7 +109,7 @@ def install! | |||
end | |||
unless skip_pch?(target.app_specs) | |||
target.app_specs.each do |app_spec| | |||
path = target.prefix_header_path_for_pec(app_spec) | |||
path = target.prefix_header_path_for_spec(app_spec) |
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.
🙈
@@ -360,6 +360,7 @@ def resource_paths | |||
@resource_paths ||= begin | |||
file_accessors.each_with_object({}) do |file_accessor, hash| | |||
resource_paths = file_accessor.resources.map { |res| "${PODS_ROOT}/#{res.relative_path_from(sandbox.project_path.dirname)}" } | |||
resource_paths = [] if file_accessor.spec.app_specification? |
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.
why wouldn't we want the resource paths of app specifications?
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.
because they now get added directly to the app target, rather than being integrated via the copy resources script
@@ -385,6 +417,7 @@ def add_app_targets | |||
app_native_target = AppHostInstaller.new(sandbox, project, platform, subspec_name, spec_name, | |||
app_target_label, :add_main => false).install! | |||
|
|||
app_native_target.product_reference.name = app_target_label |
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.
it seems like it should be in AppHostInstaller
Rather than using the copy resources script for them
To make testing app specs together with test specs easier, since their behavior should be very similiar
Since they’re now added directly to the app target
c54b258
to
3b31892
Compare
Rather than using the copy resources script for them