Fix issues with variant group, versioned, and asset catalog resources #4635

efirestone · 2015-12-07T20:07:33Z

Addresses issue #1653.

Summary
Xcode represents certain resources specially in its project structure, and without that special representation they will fail to be compiled correctly. The notable special cases are:

General localized files - en.lproj/image.png and de.lproj/image.png are represented as a "variant group" named image.png with two localizations within the Xcode project.
Localized interface files (XIB, Storyboard) using Base Internationalization - Base.lproj/Main.xib and en.lproj/Main.strings are represented as a variant group named Main.xib
Core Data Models - On disk Core Data models have a parent xcdatamodeld directory, and within them have individual versions with an xcdatamodel extension. These are represented in the Xcode project as a "versioned group"
Asset Catalogs - On disk an asset catalog is a directory, but within the Xcode project they're represented as a single file reference to the top level xcassets directory.

Because of these special cases, the previous naive copying caused issues when using a recursive glob for collecting resources. Notably, you usually had two options:

Use Resources/* (non-recursive), which would copy the .lproj directories into the resources directly. Because the internals of these directories were ignored, strings, XIBs, and other resources weren't compiled.
Use Resources/**/* (recursive), which would add all the files, but use the default CocoaPods behavior of flattening everything into a single level (like the internals of xcdatamodeld directories), thus breaking these resources.

Changes

Change variant group name to match the resource rather than stripping the extension. Ex: Localizable.strings rather than just Localizable
Special case interface files to use the XIB or Storyboard name for the variant group when using Base Internationalization.
Stop naively grouping things with the same name and different extensions. Ex: no longer groups image.strings with image.png. The specific case where this is needed is Base Internationalization, which is handled specially (see previous item).
Stop adding file references to the project for anything within an xcdatamodeld or xcassets directory. Xcode only wants a reference to the root and will handle the rest.
Stop adding items within a variant group to the resources build phase. Only the variant group itself should be added.

Other Considerations

Legacy Usage - To avoid breaking legacy usage of Resources/*, lproj directories are still added directly to the project if none of their children are matched in the glob. In general, everyone should move to the more correct Resources/**/* glob, which now works correctly.
Nested Non-Localized Resources - Localized resources are handled correctly, but as the CocoaPods default behavior is to flatten everything, nested directories such as Resources/Nested/Image.png will become just Image.png in the resulting bundle. This wasn't previously a problem because recursive globs didn't work and weren't used. This is consistent with how source and header files are handled, and so I think is a separate issue. A proper fix here might be to add something like header_mappings_dir for resources.

segiddins · 2015-12-07T20:18:27Z

@efirestone this looks absolutely awesome! I'm swamped with finals now so it might take me a bit to get around to reviewing this, but thank you for opening the PR :D

efirestone · 2015-12-07T21:11:24Z

No problem at all. Let me know if there's anything I can do to help, and good luck with finals!

efirestone · 2015-12-07T21:15:03Z

lib/cocoapods/installer/file_references_installer.rb

+      #
+      # @return [Array<Pathname>] The paths which can be added to the Xcode project
+      #
+      def allowable_project_paths(paths)


This is done as a separate pass because it needs to determine which .lproj directories are only added directly (without adding any of its children), and therefore needs access to the entire paths list. This was to preserve the existing behavior of non-recursive Resources/* globs. If we don't care/stop caring about that behavior then this filtering can be added in-line in the paths.each on L181 and just remove all items ending in .lproj. I think as-implemented is the right behavior and likely always will be.

segiddins · 2015-12-07T21:15:46Z

Biggest help would be a project / podspec I can eventually turn into an integration spec (ideally just a showcase of all the different cases you've addressed)

efirestone · 2015-12-07T21:17:30Z

lib/cocoapods/installer/target_installer/pod_target_installer.rb

@@ -98,6 +98,7 @@ def add_files_to_build_phases
          resource_refs = file_accessor.resources.flatten.map do |res|
            project.reference_for_path(res)
          end
+          resource_refs.compact!


Some resource files in the glob no longer have direct corresponding file references. This includes things like files within an xcdatamodeld directory or xcassets directory.

a comment to that effect would be excellent!

Will do. I commented on the one just below this, but worth repeating here.

efirestone · 2015-12-07T21:26:20Z

For the sample project: I added all the sample cases I could think of to the BananaLib fixture. You would just need to change from s.resource = 'Resources/*' to s.resource_bundle = 'BananaResources' => 'Resources/**/*' like some of the new tests do.

segiddins · 2015-12-07T21:27:30Z

lib/cocoapods/installer/file_references_installer.rb

+          path_str = path.to_s.downcase
+
+          # We add the directory for a Core Data model, but not the items in it.
+          next false if path_str =~ /.*xcdatamodeld\/.+/


all of these next false can just be next

segiddins · 2015-12-08T18:07:14Z

Looks good! @neonichu / @manuyavuz want to have a stab at this?

neonichu · 2015-12-08T18:56:58Z

Looks good from my end. Especially like the detailed comments, great work, @efirestone!

An additional integration spec would indeed be great, though, maybe you can share the example(s) you used to test against Xcode?

efirestone · 2015-12-08T19:37:02Z

The BananaLib tarball now includes all of them, and you can see the differences here

Namely, the following cases are important:

Base Internationalization of XIB/Storyboards - If files exist named Base.lproj/<name>.xib or Base.lproj/<name>.storyboard and files named <language>.lproj/<name>.strings then: (1) A variant group should exist named <name>.xib/storyboard that contains all of these files. (2) None of those files should exist in the Xcode project outside of the variant group. (3) Only the variant group should be in the Resources build phase and none of the child files should. If done correctly Xcode will show <FileName> in Resources/(Localization).lproj in the build phase list (it literally says (Localization)).

General Localized Files - If files exist with the same name in different localization folders (like en.lproj/image.png and de.lproj/image.png then the results should look similar to the case above: single variant group, files are only within that variant group, resources build phase only includes the variant group.

Nested Localized Files - If directories exist with the same name in different localization folders (like en.lproj/images/<files> and de.lproj/images/<files> then the results should look similar to the case above, but the directory itself is the nested item and it should be a "file system reference" type item (blue folder icon in Xcode). Xcode will have no idea of the <files> within the directories and the final resource bundle should contain direct copies of the directories, still within their appropriate lproj directories.

Core Data Models with Multiple Versions - Create a Core Data model, then add a version by going to Editor > Add Model Version…. Results should be similar to the above: (1) A versioned group should exist named <ModelName>.xcdatamodeld (2) None of the version files (xcdatamodel, no d) within the directory should exist in the project outside of the versioned group. (3) Only the versioned group should be in the Source Files build phase.

Image Asset Catalog - Create a standard asset catalog. This is actually just a PBXFileReference in the xcode project and not a group. None of the files within the catalog should show up in the file tree anywhere (clicking it gives the standard Asset Catalog UI on the right). Only the asset catalog directory file reference should show up in the resources build phase.

efirestone · 2015-12-08T19:38:07Z

After actually going through that, let me check that core data models are being put into the right build phase. I believe they may be added to Copy Bundle Resources instead of Compile Sources right now.

efirestone · 2015-12-08T22:25:41Z

CoreData models are indeed going into Resources and not Compile Sources. This isn't what Xcode does via the UI, but they do get built correctly.

Thoughts @segiddins & @neonichu? I can add a special case in the "add resources" code to add these files to the compile phase instead of resources (matches Xcode), or we can just leave them as being copied into the resources phase (less code to maintain). Both work equally well.

manuyavuz · 2015-12-09T10:58:13Z

spec/unit/installer/file_references_installer_spec.rb

+        file_ref.should.be.not.nil
+      end
+
+      it "doesn't add file references for files within Asset Catalogs" do


Shouldn't this be inside 'Installation With Recursive Resources Glob' section? BananaLib adds resources non-recursively by default, also explicitly sets xcassets path. This scenario seems to not testing your aim because there will be no resource paths for Pods/BananaLib/Resources/Images.xcassets/Logo.imageset/logo.png for example. Am I missing sth?

Yup, good catch. Will move.

manuyavuz · 2015-12-09T11:15:46Z

@efirestone this is great, really liked the good documentation and test coverage!

I have one little concern that I commented on the code, other than that, one more LGTM.

efirestone · 2015-12-09T19:43:19Z

Thanks guys! Everything's updated and should be ready to merge.

There's the one outstanding question of whether we should automatically be adding Core Data models to the Compile Sources phase rather than the Copy Bundle Resources phase. As mentioned, functionally this makes no difference.

Arguments for Compile Sources:

It's what Xcode does if you add a Core Data model using the UI.

Arguments for Copy Bundle Resources:

It's one fewer edge case to maintain.
It's consistent with how XIBs, Asset Catalogs, and strings are handled. They're also compiled, but live in the resources phase.
It's slightly more straightforward for podspec creators. They're specifying this item as part of resources, so you'd expect it to show up in resources.

My slight personal inclination is to leave it in Copy Bundle Resources for simplicity sake, but I can definitely see where the weight of the "that's how Xcode does it" argument is overpowering. If we do want to move it to the Compile Sources phase, does it make sense to modify Xcodeproj's native target add_resources method, or to find all the places in CocoaPods that this is called and branch for Core Data models? My thought would be to catch all the places in CocoaPods as this seems like a strong unintended behavior for add_resources. That does of course introduce more code.

segiddins · 2015-12-09T19:49:31Z

Let's leave it for now, but add a @note comment in the relevant method with that above comment

manuyavuz · 2015-12-09T21:26:30Z

spec/unit/installer/target_installer/pod_target_installer_spec.rb

+          config.sandbox.project = @project
+
+          @spec = fixture_spec('banana-lib/BananaLib.podspec')
+          @spec.resource_bundle = { 'banana_bundle' => ['Resources/**/*'] }


Here we should also set resources attribute to nil because current state will cause resources to be added multiple times to the project, which is not the case you want to test.

Done and done.

efirestone · 2015-12-09T22:31:40Z

@segiddins Added @note for the relevant sections.

manuyavuz · 2015-12-09T22:40:25Z

Great! 🛥

efirestone · 2015-12-11T18:58:51Z

Anything left before we can hit the merge button?

segiddins · 2015-12-11T23:59:49Z

I'll do a final review in the next couple of days

efirestone · 2015-12-12T00:20:43Z

👍

segiddins · 2015-12-12T19:14:35Z

lib/cocoapods/installer/file_references_installer.rb

+          path_str = path.to_s.downcase
+
+          # We add the directory for a Core Data model, but not the items in it.
+          next if path_str =~ /.*xcdatamodeld\/.+/


.*\.xc ? (for the next line as well)

Updated, along with the xcassets regex below it.

segiddins · 2015-12-12T19:16:04Z

👍 other than that one nitpick

Fix issues with variant group, versioned, and asset catalog resources

efirestone reviewed Dec 7, 2015
View reviewed changes

efirestone force-pushed the firestone/variant-groups branch from a892c21 to 7f59208 Compare December 7, 2015 21:22

segiddins reviewed Dec 7, 2015
View reviewed changes

efirestone force-pushed the firestone/variant-groups branch 2 times, most recently from 3c6c093 to d386c79 Compare December 7, 2015 21:55

manuyavuz reviewed Dec 9, 2015
View reviewed changes

efirestone force-pushed the firestone/variant-groups branch from d386c79 to 3b85a59 Compare December 9, 2015 19:16

manuyavuz reviewed Dec 9, 2015
View reviewed changes

efirestone force-pushed the firestone/variant-groups branch from 3b85a59 to 6040dab Compare December 9, 2015 22:31

segiddins reviewed Dec 12, 2015
View reviewed changes

Fix issues with variant group, versioned, and asset catalog resources

5b6ec48

efirestone force-pushed the firestone/variant-groups branch from 6040dab to 5b6ec48 Compare December 12, 2015 21:13

segiddins added a commit that referenced this pull request Dec 13, 2015

Merge pull request #4635 from efirestone/firestone/variant-groups

2f23307

Fix issues with variant group, versioned, and asset catalog resources

segiddins merged commit 2f23307 into CocoaPods:master Dec 13, 2015

neonichu mentioned this pull request Jan 4, 2016

Unsealed contents when a pod contains a CoreData model file CocoaPods/cocoapods-packager#118

Open

This was referenced Jan 14, 2016

resource_bundle is broken in 1.0.0.beta.2 #4762

Closed

Copy resources v3 #1690

Closed

Storyboard in Resource Bundle #2597

Closed

nkibler mentioned this pull request Jan 29, 2016

Fix bug where lproj directories aren't added to project #4765

Merged

efirestone mentioned this pull request Feb 11, 2016

Question: 'Single Vector' PDF resource in a private pod #4862

Closed

jwilliams-handy mentioned this pull request Aug 1, 2019

Pods project.pbxproj gets modified after opening up Xcode 10.2 #8689

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with variant group, versioned, and asset catalog resources #4635

Fix issues with variant group, versioned, and asset catalog resources #4635

efirestone commented Dec 7, 2015

segiddins commented Dec 7, 2015

efirestone commented Dec 7, 2015

efirestone Dec 7, 2015

segiddins commented Dec 7, 2015

efirestone Dec 7, 2015

segiddins Dec 7, 2015

efirestone Dec 7, 2015

efirestone commented Dec 7, 2015

segiddins Dec 7, 2015

segiddins commented Dec 8, 2015

neonichu commented Dec 8, 2015

efirestone commented Dec 8, 2015

efirestone commented Dec 8, 2015

efirestone commented Dec 8, 2015

manuyavuz Dec 9, 2015

efirestone Dec 9, 2015

manuyavuz commented Dec 9, 2015

efirestone commented Dec 9, 2015

segiddins commented Dec 9, 2015

manuyavuz Dec 9, 2015

efirestone Dec 9, 2015

efirestone commented Dec 9, 2015

manuyavuz commented Dec 9, 2015

efirestone commented Dec 11, 2015

segiddins commented Dec 11, 2015

efirestone commented Dec 12, 2015

segiddins Dec 12, 2015

efirestone Dec 12, 2015

segiddins commented Dec 12, 2015

Fix issues with variant group, versioned, and asset catalog resources #4635

Fix issues with variant group, versioned, and asset catalog resources #4635

Conversation

efirestone commented Dec 7, 2015

segiddins commented Dec 7, 2015

efirestone commented Dec 7, 2015

Choose a reason for hiding this comment

segiddins commented Dec 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efirestone commented Dec 7, 2015

Choose a reason for hiding this comment

segiddins commented Dec 8, 2015

neonichu commented Dec 8, 2015

efirestone commented Dec 8, 2015

efirestone commented Dec 8, 2015

efirestone commented Dec 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuyavuz commented Dec 9, 2015

efirestone commented Dec 9, 2015

segiddins commented Dec 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efirestone commented Dec 9, 2015

manuyavuz commented Dec 9, 2015

efirestone commented Dec 11, 2015

segiddins commented Dec 11, 2015

efirestone commented Dec 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

segiddins commented Dec 12, 2015