-
Notifications
You must be signed in to change notification settings - Fork 514
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
[dotnet] Figure out where to publish files in the app bundle. Fixes #12572. #13591
[dotnet] Figure out where to publish files in the app bundle. Fixes #12572. #13591
Conversation
This comment has been minimized.
This comment has been minimized.
0af7340
to
9876e95
Compare
This comment has been minimized.
This comment has been minimized.
9876e95
to
6f9bcce
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
982a2cd
to
618a279
Compare
This comment has been minimized.
This comment has been minimized.
618a279
to
5b0a8c2
Compare
This comment has been minimized.
This comment has been minimized.
Use the right template variable in the template expansion to avoid duplicating variable names.
…rty to pass to 'ditto'.
…ToPublish to a separate property
…rstand *.xcframework paths. Previously we'd only support passing in a fake reference to a file inside the *.xcframework. This makes some other code simpler later on. Best viewed by ignoring whitespace, since this is mostly whitespace changes.
…function. Best viewed by ignoring whitespace, since this is mostly whitespace changes.
…ly to the ResolveNativeReferences task.
So that the tables can be used in more places.
…more work is being done there
…esToBundle We're soon going to use this task to copy other types of directories (such as plugins) as well, and in that case the old target name would be misleading.
… building a (publishable) app
…t's easier to share
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.
Epic PR. ❤️ the significant test coverage.
@@ -55,6 +56,20 @@ public void BuildNativeReferenceFlags (TaskLoggingHelper Log, ITaskItem[] Native | |||
if (Path.GetExtension (path) != ".framework") | |||
path = Path.GetDirectoryName (path); | |||
Frameworks.Add (path); | |||
} else if (kind == NativeReferenceKind.Dynamic) { |
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.
Should this be behind a platform being targeted check, so people don't use it on iOS and then later fail/complain on submission, or is letting them bounce on submission fine?
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 think that letting them bounce on submission is fine (after all dylibs work fine during development, when it can be useful).
$(Q) $(MAKE) build BUILD_ARGUMENTS="/p:RuntimeIdentifier=ios-arm64 /p:MtouchUseLlvm=true /p:MtouchLink=SdkOnly /p:Configuration=Release-llvm /bl:@.binlog" | ||
|
||
norm: | ||
$(Q) $(MAKE) build BUILD_ARGUMENTS="/p:RuntimeIdentifier=ios-arm64 /p:MtouchLink=SdkOnly /p:Configuration=Release-norm /bl:@.binlog" |
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.
Super nit - any reason for the whitespace here?
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.
Just to line arguments up in the text editor (makes it easier to see the difference between them), but in any case these targets are not needed, so I'll just remove them.
Predicate<string?> predicate = (v) => { | ||
var fn = Path.GetFileName (v!); | ||
|
||
switch (fn) { |
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.
If/when the runtime adds more files to the bundle this test pat looks to break. Maybe add a comment here explaining to future us when to add more files to this list?
AddExpectedPlugInFiles (platform, expectedFiles, "PlugInA", isSigned); // PlugIns | ||
AddExpectedPlugInFiles (platform, expectedFiles, "CompressedPlugInB", isSigned); // CompressedPlugIns | ||
} | ||
// UnknownI.bin: Unknown -- this should show a warning |
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.
Another weird spacing issue?
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 comment has been minimized.
This comment has been minimized.
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View dotnet API diffView dotnet legacy API diffAPI Current PR diffℹ️ API Diff (from PR only) (please review changes) View dotnet API diffView dotnet legacy API diffGenerator diff✅ Generator Diff (no change) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results1 tests failed, 216 tests passed.Failed tests
Pipeline on Agent XAMBOT-1101.BigSur' |
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.
Some minor comments that could make the code simpler to understand.
var items = entry.Value; | ||
var item = new TaskItem (entry.Key); |
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.
You can expand tuples inside the foreach.
if (filename.EndsWith (".dll", StringComparison.OrdinalIgnoreCase)) { | ||
return PublishFolderType.Assembly; | ||
} else if (filename.EndsWith (".exe", StringComparison.OrdinalIgnoreCase)) { | ||
return PublishFolderType.Assembly; | ||
} else if (filename.EndsWith (".pdb", StringComparison.OrdinalIgnoreCase)) { | ||
return PublishFolderType.Assembly; | ||
} else if (filename.EndsWith (".dll.mdb", StringComparison.OrdinalIgnoreCase) || filename.EndsWith (".exe.mdb", StringComparison.OrdinalIgnoreCase)) { | ||
return PublishFolderType.Assembly; | ||
} else if (filename.EndsWith (".config", StringComparison.OrdinalIgnoreCase)) { | ||
return PublishFolderType.Assembly; | ||
} |
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.
The all return 'PublishFolderType.Assembly', could we have done this with a loop through he known extensions:
if (filename.EndsWith (".dll", StringComparison.OrdinalIgnoreCase)) { | |
return PublishFolderType.Assembly; | |
} else if (filename.EndsWith (".exe", StringComparison.OrdinalIgnoreCase)) { | |
return PublishFolderType.Assembly; | |
} else if (filename.EndsWith (".pdb", StringComparison.OrdinalIgnoreCase)) { | |
return PublishFolderType.Assembly; | |
} else if (filename.EndsWith (".dll.mdb", StringComparison.OrdinalIgnoreCase) || filename.EndsWith (".exe.mdb", StringComparison.OrdinalIgnoreCase)) { | |
return PublishFolderType.Assembly; | |
} else if (filename.EndsWith (".config", StringComparison.OrdinalIgnoreCase)) { | |
return PublishFolderType.Assembly; | |
} | |
foreach(var extension in new string [] {".dll", ".exe",".pdb", "dll.mdb", ".exe.mdb", ".config"}) { | |
if (filename.EndsWith (extension, StringComparison.OrdinalIgnoreCase)) | |
return PublishFolderType.Assembly; | |
} |
Or you can create a helper function that returns if it is an assembly checking the extension and do a simple if.
// resources (png, jpg, ...?) | ||
if (filename.EndsWith (".jpg", StringComparison.OrdinalIgnoreCase)) { | ||
return PublishFolderType.Resource; | ||
} else if (filename.EndsWith (".png", StringComparison.OrdinalIgnoreCase)) { | ||
return PublishFolderType.Resource; | ||
} |
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.
why not using a single if and a ||
if (filename.EndsWith (".framework.zip", StringComparison.OrdinalIgnoreCase)) { | ||
return PublishFolderType.CompressedAppleFramework; | ||
} else if (filename.EndsWith (".xcframework.zip", StringComparison.OrdinalIgnoreCase)) { | ||
return PublishFolderType.CompressedAppleFramework; | ||
} |
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.
Same
if (filename.EndsWith (".a", StringComparison.OrdinalIgnoreCase)) { | ||
return PublishFolderType.StaticLibrary; | ||
} else if (filename.EndsWith (".dylib", StringComparison.OrdinalIgnoreCase)) { | ||
return PublishFolderType.DynamicLibrary; | ||
} |
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.
Same.
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 returns different values, so a single if and || won't work.
Log.LogWarning (MSBStrings.E7089 /* The file '{0}' does not specify a 'PublishFolderType' metadata, and a default value could not be calculated. The file will not be copied to the app bundle. */, item.ItemSpec); | ||
|
||
return PublishFolderType.None; | ||
} |
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 winder if we could simplify all this with a switch and a return using patterns matching with the strings, should make the method smaller and easier to understand.
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.
The logic is a bit more complex in a few cases than a simple string matching a pattern.
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View dotnet API diffView dotnet legacy API diffAPI Current PR diffℹ️ API Diff (from PR only) (please review changes) View dotnet API diffView dotnet legacy API diffGenerator diff✅ Generator Diff (no change) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results2 tests failed, 215 tests passed.Failed tests
Pipeline on Agent XAMBOT-1101.BigSur |
The symlink might point to a file that has been updated, and if that file needs more processing and the logic checks the symlink to check if the file is up-to-date, then we need to reflect that in the symlink (for instance if a framework binary is updated (which is a symlink on some platforms), it needs to be resigned, even if the symlink itself didn't change).
…an app bundle. This fixes the following problem: * App with framework is built and signed. * App is rebuilt, and the framework is copied in again. * This time, the framework's executable's timestamp will be earlier than the timestamp when it was last signed, and as such it won't be signed again. Fix this by touching all the copied files when copying a directory to the app bundle.
…de the zip files in two different tasks. Split decompressing zip files and figuring out what was inside the zip files in two different tasks, so that we do the second part even if the first part isn't done (it could have been done in a previous build). This is required for rebuilds to work correctly.
This comment has been minimized.
This comment has been minimized.
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View dotnet API diffView dotnet legacy API diffAPI Current PR diffℹ️ API Diff (from PR only) (please review changes) View dotnet API diffView dotnet legacy API diffGenerator diff✅ Generator Diff (no change) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results2 tests failed, 215 tests passed.Failed tests
Pipeline on Agent XAMBOT-1099.BigSur' |
In .NET, all files that should be published (put into the final .app bundle) are put into the @(ResolvedFileToPublish) item group, and at the end of the build process, .NET will publish all the files in that item group. Which files are in this item group, and how they're put in there, is out of our control (it's just how the build process works in .NET), so we'll have to cope.
Additionally, publishing an app for Apple platforms is different than publishing other .NET apps, because we can't just put all the files in the a directory like .NET usually does, we have a fairly strict directory structure we need to follow, and it also differs between platforms (the structure is different between macOS and iOS for instance).
This means that for every file in the
ResolvedFileToPublish
item group, we have to figure out:This PR implements these changes. The exact details are explained in a document in the PR, but the general logic is:
Fixes #12572.
This PR is best reviewed commit-by-commit.