-
Notifications
You must be signed in to change notification settings - Fork 518
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
[msbuild] Add support for .xcframework
#10046
Conversation
This is done early so we can resolve the inner framework, inside the xcframework, and let the existing framework support do most of the work. Xamarin.Mac builds are disabled. Support for XM will come in a later pull-request, possibly on `xcode12.2`. There's more code for handling _normal_ frameworks inside `mtouch` and XI msbuild than in XM so it might require more work. The resolving code has unit tests. A sample `xcframework` with tests projects is also available [here](https://github.com/spouliot/xcframework). **Work in progress** More, integrated project-level, tests are coming
Build failure Test results1 tests failed, 66 tests passed.Failed tests
|
|
||
// existing (as of this commit) IDE versions will add a `.xcframework` directorty as a `Static` native reference | ||
if ((kind == NativeReferenceKind.Static) && (Path.GetExtension (libName) == ".xcframework")) |
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.
Thinking about dotnet support, I think it would be easier if the xcframework -> framework resolution was put into a new MSBuild target that ran before the _CompileToNative target, and rewrote the NativeReference item groups to be Frameworks pointing to the right path instead. This new target could then be re-used as-is in the dotnet code.
The end result is that there would be no changes to MTouchTaskBase (nor MmpTaskBase) classes, they would work as-is.
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.
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.
@dalexsoto bad example as it is not the normal structure. That's how Xcode do them: https://github.com/spouliot/xcframework#tree
and PSPDFKit tweaks them https://pspdfkit.com/blog/2020/supporting-xcframeworks/#bcsymbolmaps-and-dsyms
The idea is to make them them (xcframework) transparent to the down-level tooling. It might be easy to deal with custom dSYM or not, but it's not a top priority.
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.
Interesting, I guess you need to enable it on demand since Xcode 12.0 release notes state the following
XCFrameworks can now include .dSYM and .bcsymbolmap debugging symbol files in your library bundle with the -debug-symbols flag. Run the command xcodebuild -create-xcframework -help for additional usage information. (64910707)
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.
interesting, I'll update my repo-sample to add the flag and see how it affects the files inside the xcframework
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.
added spouliot/xcframework@9fe2052
it does make it clear / well-defined (inside the .plist
) where the dSYM can be found
This is based on Rolf's earlier/partial implementation. rolfbjarne@xcframework Thing to note: Do not rename a framework (like XTest) to use it in an xcframework (like XCTest). That will fail at codesign but won't give anything useful. You might think signing the framework (instead of the inner binary) would solve it. It does, as it codesign, but then the app crash at startup. At some point you realize some symbols are still using XTest (not XCTest) and then you can delete several other weird workarounds (like for `ld`) because all of it was cause by this never identified rename.
Build success |
Build failure Test results3 tests failed, 71 tests passed.Failed tests
|
Build failure Test results4 tests failed, 70 tests passed.Failed tests
|
Build failure Test results3 tests failed, 71 tests passed.Failed tests
|
Build failure Test results3 tests failed, 88 tests passed.Failed tests
|
Build failure Test results26 tests failed, 65 tests passed.Failed tests
|
build |
Build failure Test results1 tests failed, 90 tests passed.Failed tests
|
Build failure Test results1 tests failed, 90 tests passed.Failed tests
|
Build failure Test results1 tests failed, 90 tests passed.Failed tests
|
Build failure Test results2 tests failed, 90 tests passed.Failed tests
|
Build failure ✅ Provisioning succeeded |
Build failure Test results1 tests failed, 92 tests passed.Failed tests
|
tests/test-libraries/Makefile
Outdated
@@ -191,6 +191,39 @@ $(eval $(call FatFrameworkTemplate,ios-fat,iphoneos,iphonesimulator)) | |||
$(eval $(call FatFrameworkTemplate,tvos-fat,tvos,tvsimulator)) | |||
$(eval $(call FatFrameworkTemplate,watchos-fat,watchos,watchsimulator)) | |||
|
|||
define XCTemplate | |||
.libs/by-platform/$(1)/XTest.framework/XTest: $(foreach arch,$(3),.libs/$(2)/libtest$(4).$(arch).dylib) | .libs/by-platform/$(1)/XTest.framework |
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.
Any reason you can't use the existing XTest.framework
s?
$ ls -lad tests/test-libraries/.libs/*/XTest.framework
drwxr-xr-x 4 rolf wheel 128 Nov 24 08:20 tests/test-libraries/.libs/ios-fat/XTest.framework
drwxr-xr-x 4 rolf wheel 128 Nov 24 08:20 tests/test-libraries/.libs/iphoneos/XTest.framework
drwxr-xr-x 4 rolf wheel 128 Nov 24 08:20 tests/test-libraries/.libs/iphonesimulator/XTest.framework
drwxr-xr-x 4 rolf wheel 128 Nov 24 08:20 tests/test-libraries/.libs/macos/XTest.framework
drwxr-xr-x 4 rolf wheel 128 Nov 24 08:20 tests/test-libraries/.libs/tvos-fat/XTest.framework
drwxr-xr-x 4 rolf wheel 128 Nov 24 08:20 tests/test-libraries/.libs/tvos/XTest.framework
drwxr-xr-x 4 rolf wheel 128 Nov 24 08:20 tests/test-libraries/.libs/tvsimulator/XTest.framework
drwxr-xr-x 4 rolf wheel 128 Nov 24 08:20 tests/test-libraries/.libs/watchos-fat/XTest.framework
drwxr-xr-x 4 rolf wheel 128 Nov 24 08:20 tests/test-libraries/.libs/watchos/XTest.framework
drwxr-xr-x 4 rolf wheel 128 Nov 24 08:20 tests/test-libraries/.libs/watchsimulator/XTest.framework
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.
That should work, it came from rolfbjarne@xcframework#diff-c0db661233e8bd837a40c8de3bdbb802b63d5edb88fa262a82d9799ee39e50ebR155 , but then I renamed it XTest since XCTest is the Apple framework for Xcode unit tests and something (linking or codesigning) did not appreciate the renaming.
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.
Oh I see what happened here. My point was that this code is more complicated than it needs to be; in any case I fixed it and pushed the fix.
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.
thanks, even simpler of the changes I made locally
@@ -133,6 +133,7 @@ Copyright (C) 2020 Microsoft. All rights reserved. | |||
<!-- The default architecture for Xamarin.Mac is x86_64 --> | |||
<TargetArchitectures Condition="'$(TargetArchitectures)' == '' And '$(_PlatformName)' == 'macOS'">x86_64</TargetArchitectures> | |||
<!-- There should always be an MtouchArch value in newer projects, but for older projects default to old values --> | |||
<TargetArchitectures Condition="'$(TargetArchitectures)' == '' And '$(_PlatformName)' == 'tvOS' And '$(ComputedPlatform)' == 'iPhoneSimulator'">x86_64</TargetArchitectures> |
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 shouldn't be necessary, all tvOS projects should have an MtouchArch value.
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'll try again but I'm pretty sure it did not work. However it might just be the xharness created/cloned projects that are missing the value.
Build failure |
|
Build success |
There may have been some sort of regression here, as even @spouliot's test project referenced in the PR fails to build with the newest version of Xamarin. |
I hv downloaded the same project but when I tried to run I get this error === Visual Studio Community 2019 for Mac === |
This is done early so we can resolve the inner framework, inside the
xcframework, and let the existing framework support do most of the
work.
The resolving code has unit tests. Custom projects for "NoEmbedding"
exists for all supported platforms and executed by xharness.
A sample
xcframework
with tests projects is also available here.Work in progress