Skip to content
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

#10111 - Update XCFramework installation to support Xcode 12 debug symbols #10122

Merged

Conversation

johntmcintosh
Copy link
Contributor

#10111 - Update XCFramework installation to support Xcode 12 debug symbols placed as siblings of the .framework inside each architecture's directory

Related integration-specs PR: CocoaPods/cocoapods-integration-specs#301

@dnkoutso
Copy link
Contributor

dnkoutso commented Oct 5, 2020

@johntmcintosh can you rebase against 1-10-stable branch so we can ship this with 1.10?

… debug symbols placed as siblings of the .framework inside each architecture's directory
@johntmcintosh johntmcintosh changed the base branch from master to 1-10-stable October 5, 2020 16:26
@johntmcintosh
Copy link
Contributor Author

@dnkoutso sure thing. This and the submodule PR are rebased and re-pointed to their respective 1-10-stable branches

# We don't want the path to the library binary, we want the dir that contains it
shell_escape(slice.path.dirname.relative_path_from(root))
end
args << shell_escape(slice.path.dirname.relative_path_from(root))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hah, glad this was already supported in one code path :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was relieved to find it 😅

elif [[ "$package_type" == "framework" ]]; then
copy_dir "$source" "$destination"
fi
copy_dir "$source/" "$destination"
Copy link
Member

@amorde amorde Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might run in to problems here if xcframeworks are vended using the new style of embedding dSYMs but users are still on Xcode 11.x. In that case, we would not want to copy those files and instead rely on the existing logic here:

def create_copy_dsyms_script
dsym_paths = PodTargetInstaller.dsym_paths(target)
bcsymbolmap_paths = PodTargetInstaller.bcsymbolmap_paths(target)
path = target.copy_dsyms_script_path
unless dsym_paths.empty? && bcsymbolmap_paths.empty?
generator = Generator::CopydSYMsScript.new(dsym_paths, bcsymbolmap_paths)
update_changed_file(generator, path)
add_file_to_support_group(path)
end
end

I believe there may be some exposed variables here that we can use to conditionally opt-in to the newer behavior, but I haven't thought this out thoroughly yet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some initial testing with that use-case today and it actually appeared to be working with this implementation using new style dSYMs and Xcode 11. My theory at the time was that the logic that's looking for dSYMs happened to be looking from new location down, rather than starting down one level. If that's the case, then we might be in the clear with this, but I can do some more focused investigation on that use-case tomorrow as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amorde while investigating, I'm getting stuck tracing something... in the generated Pods-MyApp-frameworks.sh script, there's an install_dsym() function which I think is added to that script from embed_frameworks_script.rb, but I haven't been able to find what calls that function. Is that function being used there and I'm overlooking it, or is it leftover from a previous iteration and no longer used?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnmcintosh I have seen this code while inspecting the sources recently. I think its vestigial and can be removed. You can keep it there out of abundance of caution since its mostly irrelevant to this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is when archiving, we need to make sure we don't leave the dSYMs or bcsymbolmaps in the archive (in the incorrect place) or Apple will reject the archive. I haven't testing Xcode 12's archiving using the new dSYMs to see where they end up but I'd guess they use the previous location and don't leave it inside the framework

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is when archiving, we need to make sure we don't leave the dSYMs or bcsymbolmaps in the archive (in the incorrect place) or Apple will reject the archive.

@amorde yep, definitely. To make sure I'm testing in the right direction, what is your understanding of the source location for dSYMs from Xcode 11 xcframeworks? Since Apple didn't have guidance (that I'm aware of) it was up to us where to put them. What had been your expectation originally for where the Xcode 11 xcframework dSYMs would be located?

I had originally been thinking that they would be inside each architecture's .framework next to the BCSymbolMaps directory, but using the PSPDFKit xcframework podspec as a reference right now, they were placing the dSYMs as a sibling to the xcframework in the root of the zip download, rather than nested down somewhere inside of the xcframework. In that example since they aren't part of the .frameworks, I don't think there's any "removal" from a source location that would need to happen for that Xcode 11 style.

I polished up my test project a bit today. If you can help clarify where CocoaPods was expecting xcframework debug symbols to be placed previously, then I can spend some time tomorrow working on migrating the test project I've been using into the examples tests that will run with CI so we can have validation of both expectations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amorde after a little more investigation today, this approach is still seeming correct for everything I've been able to validate. I just pushed up a branch off of this branch as xcframeworkDebugSymbolsXcode12-example. (I created a separate branch since the example compiled xcframework is checked in and there were hundreds of files in the diff so I didn't want to clutter this diff right now).

If you pull that branch, I've added a new example examples/Vendored-XCFramework12-Example which has one example project that pulls in two libraries.

  • BananaLib is setup in the "current" Xcode 11 recommended style:
    • dSYMs are placed in a .dSYMs directory as a sibling to the .xcframework Ref #10114. They do not need to be stripped from the built archive's copy of the .framework, because they were never inside of it to start.
    • BCSymbolMaps are placed per architecture at BananaLib.xcframework/<architecture>/BananaLib.framework/BCSymbolMaps/ and do need to be from the built archive's copy of the .framework.
  • CoconutLib is setup in the "new" Xcode 12 style:
    • dSYMs and BCSymbolMaps are both placed per architecture as a sibling to the architecture's .framework:
      • CoconutLib.xcframework/<architecture>/BCSymbolMaps/
      • CoconutLib.xcframework/<architecture>/dSYMs/
      • CoconutLib.xcframework/<architecture>/CoconutLib.framework/
    • Because they are not children of the .framework they also don't need to be stripped from the built archive, they just need to be copied into it at the correct location.

If you open the Vendored-XCFramework12-Example/Example.xcworkspace, there's an ArchiveAndValidate aggregate target scheme that you can compile. There's a script build phase that compiles a release archive of the app, and then validates that:

  • The expected Xcode 11 style BCSymbolMaps and dSYMs are installed from BananaLib
  • The expected Xcode 12 style BCSymbolMaps and dSYMs are installed from CoconutLib
  • The embedded BCSymbolMaps from the XCode 11 style BananaLib are stripped from the embedded BananaLib.framework

If any of those validations do not pass, the script will fail the build. This setup appears to have this test now be included when I run the example tests (bundle exec rake examples).

The test example should be in a state that it could be PR'd on its own, I just didn't want to clutter this one up. Let me know if you'd prefer I go ahead and include it and I can update this PR to include that test.

One last note, when my test runs, the result also includes three warnings along the lines of

[ CP] Vendored binary '< long-path >' contains architectures (armv7k arm64_32) none of which match the current build architectures (arm64).

which relates to #10114. @dnkoutso if helpful for that work, my test project here may also be able to be used for validating work related to only including the relevant debug symbols (when the debug symbols are being included in the source pod using the Xcode 11 style structure).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johntmcintosh thank you so much for the attention to detail here. Looking over your example, I see the source of my own confusion. I was thinking the new Xcode 12-style had the dSYMs and BCSymbolmaps inside the .framework bundle, but I see now they are parallel to the .framework in the directory structure.

The example you've setup is really great, and is a big improvement over our existing examples since it validates the structure of an archived app. This is a gap that we've had in our automated tests for a long time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @amorde ! I was glad the idea of a run script in the aggregate target was able to work with the current setup. The overall structure of how the example tests are setup is neat.

FYI - I saw you merged the integration-specs PR, so I've pushed an update to this one bumping the submodule pointer to the new, merged commit.

Let me know if you'd prefer me add the example as part of this PR, otherwise I'll plan to put up a second PR with it right after this one merges.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second PR sounds great 👍

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.

None yet

3 participants