[msbuild] Simplify resolving xcframeworks - #10376
Merged
Merged
Conversation
TD&LR: This PR simplifies how we refer to user frameworks and fixes both warnings and non-optimal (app) output. Much longer story: Additional testing on macOS showed some build-time warnings and an [extra (dupe) file](spouliot/xcframework@a20f8ab#diff-54fd7d9cd5deae57f30195be0a43133eace03c1132401741a317e0ae8d5e13fdR34). Logs shows that we referred to the xcframework several times, where once should have been enough. ``` /native-reference:/Users/poupou/git/spouliot/xcframework/Universal.xcframework /native-reference:/Users/poupou/git/spouliot/xcframework/Universal.xcframework/macos-arm64_x86_64/Universal.framework /native-reference:/Users/poupou/git/spouliot/xcframework/Universal.xcframework/macos-arm64_x86_64/Universal.framework/Universal ``` The first `/native-reference` line produced a warning like: ``` MMP warning MM2006: Native library 'Universal.xcframework' was referenced but could not be found. ``` which makes sense as the tools (both `mmp` and `mtouch`) are not, by design, aware of (unresolved) xcframeworks. Removing `{NativeReference}` from `Xamarin.Mac.Common.targets` (and `Xamarin.iOS.Common.targets`) as it has already been processed by `_ExpandNativeReferences` solves this. The other part of the issue (next two lines) is because `msbuild` does not track changes to directories like it does for files - and the workaround (in `_ExpandNativeReferences`) had to be copied in other places (both XI and XM `_CompileToNative`) and that was not enough (and would eventually need to be duplicated again and again). This could lead to duplicate entries (i msbuild logs) like ``` NativeReferences ../../Universal.xcframework/macos-arm64_x86_64/Universal.framework ../../Universal.xcframework/macos-arm64_x86_64/Universal.framework/Univeral ``` which maps to our extra entries. In order to simplify things we make the `_ExpandNativeReferences` resolve the full path to the library name (not the `.framework` directory) which simplifies both `_CompileToNative` and ensure a single way (at least for `msbuild`) to provide this data to the tools (`mmp` and `mtouch`). Using a file, instead of a directory, is also more consistent for the existing `-framework` option, e.g. we provide the names like: ``` --framework=CoreLocation --framework=ModelIO ``` So adding a full path that include the name is more appropriate, e.g. ``` --framework=/Users/poupou/git/master/xamarin-macios/tests/xharness/tmp-test-dir/xcframework-test760/bin/AnyCPU/Debug/bindings-xcframework-test.resources/XTest.xcframework/ios-i386_x86_64-simulator/XTest.framework/XTest ``` Finally for macOS applications it turns out we were embedding yet another copy of the framework's library inside the `MonoBundle`, which is clearly wrong, because of the last entry. ``` $ l bin/Release/xcf-mac.app/Contents/MonoBundle/Universal -rwxr-xr-x 1 poupou staff 167152 2 Dec 16:16 bin/Release/xcf-mac.app/Contents/MonoBundle/Universal ``` The tool now checks if a provided library is inside a framework (or not) which is a good validation to have anyway when it gets called directly, i.e. not thru `msbuild`.
rolfbjarne
approved these changes
Jan 11, 2021
| Outputs="$(_AppBundlePath)Contents\MacOS\$(_AppBundleName);$(DeviceSpecificOutputPath)bundler.stamp"> | ||
|
|
||
| <ItemGroup> | ||
| <_FrameworkNativeReference Include="@(_FrameworkNativeReference -> '%(Identity)/%(Filename)')" Condition="'%(Extension)' == '.framework' Or '%(Extension)' == '.xcframework'" /> |
Member
There was a problem hiding this comment.
Should the identical code in Xamarin.iOS.Common.targets be removed too?
dalexsoto
approved these changes
Jan 11, 2021
Collaborator
❌ Device tests failed on Build ❌Device tests failed on Build. Test results2 tests failed, 39 tests passed.Failed tests
Pipeline on Agent XAMBOT-1106' |
This was referenced Jan 11, 2021
Contributor
|
Test failures are unrelated: https://github.com/xamarin/maccore/issues/2357 |
mandel-macaque
approved these changes
Jan 11, 2021
Collaborator
❌ Device tests failed on Build ❌Device tests failed on Build. Test results7 tests failed, 34 tests passed.Failed tests
Pipeline on Agent XAMBOT-1099' |
rolfbjarne
approved these changes
Jan 12, 2021
Contributor
|
Build failure |
tj-devel709
approved these changes
Jan 12, 2021
Contributor
Author
|
Original failures (first build) are due to
Additional failures (2nd build) are due to |
emaf
approved these changes
Jan 12, 2021
spouliot
added a commit
to spouliot/xamarin-macios
that referenced
this pull request
Feb 4, 2021
TD&LR: This PR simplifies how we refer to user frameworks and fixes both warnings and non-optimal (app) output. Much longer story: Additional testing on macOS showed some build-time warnings and an [extra (dupe) file](spouliot/xcframework@a20f8ab#diff-54fd7d9cd5deae57f30195be0a43133eace03c1132401741a317e0ae8d5e13fdR34). Logs shows that we referred to the xcframework several times, where once should have been enough. ``` /native-reference:/Users/poupou/git/spouliot/xcframework/Universal.xcframework /native-reference:/Users/poupou/git/spouliot/xcframework/Universal.xcframework/macos-arm64_x86_64/Universal.framework /native-reference:/Users/poupou/git/spouliot/xcframework/Universal.xcframework/macos-arm64_x86_64/Universal.framework/Universal ``` The first `/native-reference` line produced a warning like: ``` MMP warning MM2006: Native library 'Universal.xcframework' was referenced but could not be found. ``` which makes sense as the tools (both `mmp` and `mtouch`) are not, by design, aware of (unresolved) xcframeworks. Removing `{NativeReference}` from `Xamarin.Mac.Common.targets` (and `Xamarin.iOS.Common.targets`) as it has already been processed by `_ExpandNativeReferences` solves this. The other part of the issue (next two lines) is because `msbuild` does not track changes to directories like it does for files - and the workaround (in `_ExpandNativeReferences`) had to be copied in other places (both XI and XM `_CompileToNative`) and that was not enough (and would eventually need to be duplicated again and again). This could lead to duplicate entries (i msbuild logs) like ``` NativeReferences ../../Universal.xcframework/macos-arm64_x86_64/Universal.framework ../../Universal.xcframework/macos-arm64_x86_64/Universal.framework/Univeral ``` which maps to our extra entries. In order to simplify things we make the `_ExpandNativeReferences` resolve the full path to the library name (not the `.framework` directory) which simplifies both `_CompileToNative` and ensure a single way (at least for `msbuild`) to provide this data to the tools (`mmp` and `mtouch`). Using a file, instead of a directory, is also more consistent for the existing `-framework` option, e.g. we provide the names like: ``` --framework=CoreLocation --framework=ModelIO ``` So adding a full path that include the name is more appropriate, e.g. ``` --framework=/Users/poupou/git/master/xamarin-macios/tests/xharness/tmp-test-dir/xcframework-test760/bin/AnyCPU/Debug/bindings-xcframework-test.resources/XTest.xcframework/ios-i386_x86_64-simulator/XTest.framework/XTest ``` Finally for macOS applications it turns out we were embedding yet another copy of the framework's library inside the `MonoBundle`, which is clearly wrong, because of the last entry. ``` $ l bin/Release/xcf-mac.app/Contents/MonoBundle/Universal -rwxr-xr-x 1 poupou staff 167152 2 Dec 16:16 bin/Release/xcf-mac.app/Contents/MonoBundle/Universal ``` The tool now checks if a provided library is inside a framework (or not) which is a good validation to have anyway when it gets called directly, i.e. not thru `msbuild`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TD&LR: This PR simplifies how we refer to user frameworks and fixes both
warnings and non-optimal (app) output.
Much longer story:
Additional testing on macOS showed some build-time warnings and an
extra (dupe) file.
Logs shows that we referred to the xcframework several times, where once
should have been enough.
The first
/native-referenceline produced a warning like:which makes sense as the tools (both
mmpandmtouch) are not, bydesign, aware of (unresolved) xcframeworks.
Removing
{NativeReference}fromXamarin.Mac.Common.targets(andXamarin.iOS.Common.targets) as it has already been processed by_ExpandNativeReferencessolves this.The other part of the issue (next two lines) is because
msbuilddoesnot track changes to directories like it does for files - and the
workaround (in
_ExpandNativeReferences) had to be copied in otherplaces (both XI and XM
_CompileToNative) and that was not enough (andwould eventually need to be duplicated again and again).
This could lead to duplicate entries (i msbuild logs) like
which maps to our extra entries.
In order to simplify things we make the
_ExpandNativeReferencesresolvethe full path to the library name (not the
.frameworkdirectory) whichsimplifies both
_CompileToNativeand ensure a single way (at least formsbuild) to provide this data to the tools (mmpandmtouch).Using a file, instead of a directory, is also more consistent for the
existing
-frameworkoption, e.g. we provide the names like:So adding a full path that include the name is more appropriate, e.g.
Finally for macOS applications it turns out we were embedding yet another
copy of the framework's library inside the
MonoBundle, which is clearlywrong, because of the last entry.
The tool now checks if a provided library is inside a framework (or not)
which is a good validation to have anyway when it gets called directly,
i.e. not thru
msbuild.