8000 Allow app specs to be used to upload to the app store by segiddins · Pull Request #8275 · CocoaPods/CocoaPods · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 17 commits into from
Feb 8, 2019

Conversation

segiddins
Copy link
Member
  • [PodTargetInstall] Add app spec resources directly to the app target
    Rather than using the copy resources script for them
  • [PodTargetInstaller] Remove target build settings that are in the pod target xcconfig
  • [BuildSettings] Allow {pod,user}_target_xcconfig to override singular settings
  • [BuildSettings] Add missing plural settings
  • [PodTargetInstaller] Use the correct methods for accessing app spec paths
  • [PodTarget] Store non-library spec build settings

@segiddins segiddins requested a review from dnkoutso November 14, 2018 09:35
@CocoaPodsBarista
Copy link
CocoaPodsBarista commented Nov 14, 2018
1 Warning
⚠️ Please include a CHANGELOG entry to credit yourself!
You can find it at CHANGELOG.md.

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

@segiddins segiddins force-pushed the segiddins/app-spec-fixes branch from 2fbe50a to 37799cd Compare November 14, 2018 09:39
@dnkoutso dnkoutso added this to the 1.7.0 milestone Nov 14, 2018
@dnkoutso
Copy link
Contributor

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 app_spec.resources as well as one that tests overriding certain settings like PRODUCT_BUNDLE_IDENTIFIER

@segiddins
Copy link
Member Author

cc @dostrander

@segiddins segiddins force-pushed the segiddins/app-spec-fixes branch from 37799cd to ff9784e Compare December 4, 2018 17:53
@segiddins segiddins force-pushed the segiddins/app-spec-fixes branch 2 times, most recently from 41050c2 to c54b258 Compare February 6, 2019 06:22
@segiddins segiddins changed the title [WIP] Allow app specs to be used to upload to the app store Allow app specs to be used to upload to the app store Feb 6, 2019
@segiddins segiddins requested review from amorde and dnkoutso February 6, 2019 06:30
@dnkoutso
Copy link
Contributor
dnkoutso commented Feb 6, 2019

Does this supersede #8432 and we just need to add INFOPLIST_FILE into the build settings list?

@segiddins
Copy link
Member Author

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.
Copy link
Contributor
@dnkoutso dnkoutso Feb 6, 2019

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) }
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Contributor
@dnkoutso dnkoutso left a 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
Copy link
Contributor

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.

Copy link
Member
@amorde amorde Feb 7, 2019

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

Copy link
Member Author

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)
Copy link
Contributor

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?
Copy link
Contributor

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?

Copy link
Member Author

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
Copy link
Member
@amorde amorde Feb 7, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
2910
0