[msbuild] Fix how UnpackLibraryResources handles mscorlib.dll (and potentially other framework assemblies) #1011
Conversation
This file contains 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
…tentially other framework assemblies) Target _UnpackLibraryResources: Task "UnpackLibraryResources" Using task UnpackLibraryResources from Xamarin.MacDev.Tasks.UnpackLibraryResources, Xamarin.MacDev.Tasks, Version=1.0.6128.15885, Culture=neutral, PublicKeyToken=null UnpackLibraryResources Task Prefix: monotouch IntermediateOutputPath: obj/iPhone/Debug/ NoOverwrite: obj/iPhone/Debug/ibtool-link/LaunchScreen.storyboardc/01J-lp-oVM-view-Ze5-6b-2t3.nib obj/iPhone/Debug/ibtool-link/LaunchScreen.storyboardc/Info.plist obj/iPhone/Debug/ibtool-link/LaunchScreen.storyboardc/UIViewController-01J-lp-oVM.nib obj/iPhone/Debug/ibtool-link/Main.storyboardc/BYZ-38-t0r-view-8bC-Xf-vdC.nib obj/iPhone/Debug/ibtool-link/Main.storyboardc/Info.plist obj/iPhone/Debug/ibtool-link/Main.storyboardc/UIViewController-BYZ-38-t0r.nib ReferencedLibraries: /Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS/System.dll /Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS/System.Xml.dll /Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS/System.Core.dll /Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS/Xamarin.iOS.dll /Users/poupou/Downloads/LinkingTest-2/RMSDKWrapper/bin/Debug//RMSDKWrapper.dll /Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS//mscorlib.dll /Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS//mscorlib.dll Skipping framework assembly: /Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS/System.dll Skipping framework assembly: /Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS/System.Xml.dll Skipping framework assembly: /Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS/System.Core.dll Skipping framework assembly: /Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS/Xamarin.iOS.dll Inspecting assembly: /Users/poupou/Downloads/LinkingTest-2/RMSDKWrapper/bin/Debug//RMSDKWrapper.dll Inspecting assembly: /Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS//mscorlib.dll Inspecting assembly: /Users/poupou/git/xamarin/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS//mscorlib.dll Done executing task "UnpackLibraryResources" Done building target "_UnpackLibraryResources" in project "/Users/poupou/Downloads/LinkingTest-2/LinkingTest/LinkingTest.csproj". The above log excerpt shows two issues: 1. mscorlib.dll is needlessly inspected as it's **not** considered a "framework" assembly. The current check was checking *how* it was resolved and not *where* it was resolved to. The later is the most important as it's possible for other assemblies to have direct paths references and we do not want to process them. This is fixed by comparing each assembly path with the (now) provided `TargetFrameworkDirectory` 2. mscorlib.dll is inspected twice That's because it's present two times in the task's input. That issue is upstream (not sure why) of the current task but it makes #1 twice as costly. The fix for #1 indirectly fix that too. Future ------ It's worth investigating to move that logic into `mtouch`. The later must already load all assemblies and is in charge of removing other embedded data (e.g. native code from bindings) from the assemblies (so they are not shipped both inside and outside the .dll in the final .app). This makes this task seems extraneous work. Considering that my current test case, `RMSDKWrapper.dll`, is 1.3GB in size it's easy to see that the extra load (which has nothing to be extracted wrt resources*) is quite visible in build time. > 3268.201 ms UnpackLibraryResources 1 calls * it has for bindings but that's already handled by mtouch
|
@radical, @jstedfast feedback welcome :) |
jstedfast
reviewed
Oct 20, 2016
| { | ||
| foreach (var dir in TargetFrameworkDirectory) { | ||
| if (asm.ItemSpec.StartsWith (dir.ItemSpec, StringComparison.Ordinal)) | ||
| return true; |
There was a problem hiding this comment.
Depending on how the TaskItem is created, the ItemSpec may be a relative path so it may be better to use the full path of each item using this approach:
var path = item.GetMetadata ("FullPath");Obviously we can do this outside the loop for asm as an optimization.
My other suggestion is that we may want to use an OrdinalIgnoreCase compare since the Mac file system is case insensitive by default.
rolfbjarne
approved these changes
Oct 21, 2016
|
build |
|
Build failure |
|
Build failure: /Users/builder/jenkins/workspace/xamarin-macios-pr-builder/_mac-build/Library/Frameworks/Mono.framework/External/xbuild/Xamarin/Mac/Xamarin.Mac.Common.targets: error : Error executing task UnpackLibraryResources: Required property 'TargetFrameworkDirectory' not set. |
|
Looks like TargetFrameworkDirectory does not get defined. Is this something that was supposed to be defined by xbuild? |
|
The build failure happened on XM - so it seems |
|
Build failure |
|
@jstedfast please double check that I understood your comment correctly, thanks! |
|
Build failure |
|
random failure due to https://bugzilla.xamarin.com/show_bug.cgi?id=50634 |
|
build |
|
Build success |
|
@spouliot yep, that's what I meant. I approve this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The above log excerpt shows two issues:
mscorlib.dll is needlessly inspected as it's not considered a
"framework" assembly.
The current check was checking how it was resolved and not where
it was resolved to. The later is the most important as it's possible
for other assemblies to have direct paths references and we do not
want to process them.
This is fixed by comparing each assembly path with the (now) provided
TargetFrameworkDirectorymscorlib.dll is inspected twice
That's because it's present two times in the task's input. That issue
is upstream (not sure why) of the current task but it makes
make installdoes not work unlessmake allis already complete #1 twiceas costly. The fix for
make installdoes not work unlessmake allis already complete #1 indirectly fix that too.Future
It's worth investigating to move that logic into
mtouch. The later mustalready load all assemblies and is in charge of removing other embedded
data (e.g. native code from bindings) from the assemblies (so they are not
shipped both inside and outside the .dll in the final .app). This makes
this task seems extraneous work.
Considering that my current test case,
RMSDKWrapper.dll, is 1.3GB insize it's easy to see that the extra load (which has nothing to be
extracted wrt resources*) is quite visible in build time.