-
Notifications
You must be signed in to change notification settings - Fork 507
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
Update AOTCompile task to handle cycles in up-to-date check #20103
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
💻 [PR Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Monterey (12) passed 💻✅ All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Ventura (13) passed 💻✅ All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
📚 [PR Build] Artifacts 📚Packages generatedView packagesPipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11) passed. Pipeline on Agent |
💻 [CI Build] Windows Integration Tests passed 💻✅ All Windows Integration Tests passed. Pipeline on Agent |
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
🚀 [CI Build] Test results 🚀Test results✅ All tests passed on VSTS: simulator tests. 🎉 All 170 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
|
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 looks great!
One question: does the algorithm handle assemblies that are not referenced by any other assembly? From the code it looks like it should work fine, but I'd like to confirm it.
A test case would also be great, but I'm not sure if it's possible to re-create this scenario with normal C# projects?
We iterate over the whole input set of assemblies in the top-most
It's rather difficult to produce a reasonable test case. Not impossible but requires quite a bit of manual / MSBuild fiddling. |
tarjan's algorithm for the win! |
Background
While cyclic assembly references are unusual they are supported by the runtime and encountered in practice. In our project we use a version of Xamarin.Forms retargeted for a modern .NET (as a stopgap solution before an update to full MAUI stack). Xamarin.Forms historically come with such an assembly cycle:
Xamarin.Forms.Core.dll
->Xamarin.Forms.Platform.dll
->Xamarin.Forms.Platform.iOS.dll
->Xamarin.Forms.Core.dll
. This is produced by first compilingXamarin.Forms.Core.dll
against a dummyXamarin.Forms.Platform.dll
(netstandard2.0
assembly). Then the Platform assemblies are built, and finally forwarder versions ofXamarin.Forms.Platform.dll
are created for each platform. This is all packaged into NuGets in such a way that each TFM gets the sameXamarin.Forms.Core.dll
but a different set ofXamarin.Forms.Platform.dll
andXamarin.Forms.Platform.<platform>.dll
assemblies.Problem
With current workloads each incremental rebuild fails with:
Solution
The PR updates the algorithm in
AOTCompile
to detect cycles using Tarjan's algorithm for finding strongly connected components in a graph. When a cycle is detect any assembly in the cycle is considered up-to-date only if all the assemblies in the cycle are up-to-date.With this change the project does incremental rebuilds correctly. I verified in the build output that the assembly cycle is considered up-to-date, and that any changes to the application assemblies still return
false
from the up-to-date check and get rebuilt.