-
Notifications
You must be signed in to change notification settings - Fork 517
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
[ApiDiffs] Creating Api Diffs for the Dotnet Assemblies #12886
Conversation
✅ [PR Build] Tests passed on Build. ✅Tests passed on Build. API diff✅ API Diff from stable API & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): 🎉 All 102 tests passed 🎉Pipeline on Agent XAMBOT-1096.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.
Small comment.
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 very good so far!
Make.config
Outdated
@@ -36,8 +36,7 @@ $(TOP)/Make.config.inc: $(TOP)/Make.config $(TOP)/mk/mono.mk | |||
|
|||
include $(TOP)/Make.versions | |||
|
|||
APIDIFF_REFERENCES=https://bosstoragemirror.blob.core.windows.net/wrench/xcode12.5/ab40b131d3e87a96c3414447dcded5fec45bce36/4679032/package/bundle.zip | |||
|
|||
APIDIFF_REFERENCES=https://bosstoragemirror.blob.core.windows.net/wrench/main/cd2867d44caa9485701b6b63bb6c38f771a767c9/5267313/package/bundle.zip |
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.
Unfortunately this part becomes a bit complicated.
We can't do this, because it will make the api diffs for legacy Xamarin wrong (in fact this should be bumped to the apidiff reference from the xcode13-ios
branch now).
However, we can't bump to the bundle.zip from the xcode13-ios
branch, because it won't contain binaries for Xamarin.Mac (nor the .NET assemblies for that matter).
We really need a different bundle.zip for every platform, but for legacy Xamarin that's out of scope for this PR.
But what is possible is a middle ground: use a different url for .NET assemblies, and even better would be to make it possible to use a different url for each platform, so something like this:
APIDIFF_REFERENCES_DOTNET_iOS=...
APIDIFF_REFERENCES_DOTNET_tvOS=...
APIDIFF_REFERENCES_DOTNET_macOS=...
APIDIFF_REFERENCES_DOTNET_MacCatalyst=...
because it's entirely possible we'll end up in the same situation again at some point (different stable versions of iOS and macOS), and this way we'd have a solution since we could have different reference bundles for different platforms.
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.
Will be starting this part in another PR tomorrow!
@rolfbjarne I will be pausing work on this PR to work on creating multiple bundle links to use for the references! |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results2 tests failed, 100 tests passed.Failed tests
Pipeline on Agent XAMBOT-1096.BigSur' |
$(Q) $(MAKE) all -j8 | ||
$(Q) $(MAKE) -j8 ios-markdown tvos-markdown watchos-markdown macos-markdown maccat-markdown | ||
$(Q) $(MAKE) ios-markdown tvos-markdown watchos-markdown macos-markdown maccat-markdown dotnet-markdown dotnet-legacy-markdown dotnet-iOS-MacCatalyst-markdown |
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.
Removing the -j8
since this often causes printing mistakes to the index.html
@rolfbjarne I updated this PR to accept a separate bundle.zip url for each dotnet platform and only download that url if it has not already been downloaded by another platform. Please let me know if there is any condensing that can be done in the rules! |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable API & Generator diffℹ️ API Diff (from PR only) (please review changes) GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results1 tests failed, 101 tests passed.Failed tests
Pipeline on Agent XAMBOT-1100.BigSur' |
This looks much better now! |
This PR creates the Api Diffs for the Dotnet Assemblies for three different scenarios:
Task is found here: #10210
Next part of this is to include these new diffs into the CI!