-
Notifications
You must be signed in to change notification settings - Fork 540
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
[build] Use SharedFramework.Sdk to create workload packs #10000
Conversation
Our workload packaging logic has been updated to use more of the [Microsoft.DotNet.SharedFramework.Sdk][0] MSBuild SDK included in the dotnet/arcade repo. This SDK allows us to easily exclude the symbols files that we've been shipping directly in all of our workload packs, and instead move them into `*.symbols.nupkg`` packages that can be archived and uploaded to a symbol server separately. [0]: https://github.com/dotnet/arcade/tree/21f46f494265f11e124c47ee868fac199768a35b/src/Microsoft.DotNet.SharedFramework.Sdk
@@ -425,8 +425,6 @@ public Foo () | |||
$"{app.ProjectName}.pdb should be deployed!"); | |||
StringAssertEx.ContainsRegex ($@"NotifySync CopyFile.+{lib.ProjectName}\.pdb", appBuilder.LastBuildOutput, | |||
$"{lib.ProjectName}.pdb should be deployed!"); | |||
StringAssertEx.ContainsRegex ($@"NotifySync CopyFile.+Mono.Android\.pdb", appBuilder.LastBuildOutput, | |||
$"Mono.Android.pdb should be deployed!"); |
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.
@dellis1972 @jonathanpeppers With the changes in this PR to remove symbols from runtime packs, I've had to update a few tests that check for the existance of Mono.Android.pdb in the apk/fastdev location after install. This didn't seem to cause any issues in our other tests, but I'm wondering if this will have some impact on the debugging experience? We'll still be pushing these files to various symbol servers, but do they also need to be deployed to a target device/emulator?
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 feel like for Mono the .pdb
is required to be deployed, is that correct @dellis1972?
For CoreCLR, we don't know yet what the requirements will be.
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.
We probably need to .pdb for Mono.Android
if we want to step through the code for Mono.Android
.
It would be nice to check if the Debugger tests still work without it.
That said if the pdb is present we will get better stack traces if there are crashes inside Mono.Android
which would make for a better debugging experience.
I've never been clear on the the symbols servers really work for those on Mac/Linux. I know VS can fetch them, but Ive never seem VSCode do anything like that.
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.
Ok, so the only thing we might have to test:
- Setup a
Android.Widget.Button
click event, our template probably has one - put
throw new Exception();
inside - See if Visual Studio / VS Code can break (pause) on the line of the exception
I suspect this might not work without the Mono.Android.pdb
? @jonpryor do you have a memory of that?
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 spent some time testing this today in a couple of different dev boxes. As far as I can tell, the only noticeable difference is related to stack traces and line number/information for code within Mono.Android.dll. For example:
.NET 10 preview 2:
[AndroidRuntime] android.runtime.JavaProxyThrowable: [System.Exception]: Goodbye button
[AndroidRuntime] at AndroidApp10p2TestSym.MainActivity+<>c__DisplayClass1_0.<OnCreate>b__0(C:\Users\pecolli\source\AndroidApp10p2TestSym\MainActivity.cs:19)
[AndroidRuntime] at Android.Views.View+IOnClickListenerImplementor.OnClick(/Users/runner/work/1/s/xamarin-android/src/Mono.Android/obj/Release/net10.0/android-35/mcw/Android.Views.View.cs:2520)
[AndroidRuntime] at Android.Views.View+IOnClickListenerInvoker.n_OnClick_Landroid_view_View_(/Users/runner/work/1/s/xamarin-android/src/Mono.Android/obj/Release/net10.0/android-35/mcw/Android.Views.View.cs:2477)
[AndroidRuntime] at mono.android.view.View_OnClickListenerImplementor.n_onClick(Native Method)
[AndroidRuntime] at mono.android.view.View_OnClickListenerImplementor.onClick(View_OnClickListenerImplementor.java:29)
[AndroidRuntime] at android.view.View.performClick(View.java:8028)
[AndroidRuntime] at android.view.View.performClickInternal(View.java:8005)
This PR:
[AndroidRuntime] android.runtime.JavaProxyThrowable: [System.Exception]: Goodbye button
[AndroidRuntime] at AndroidApp10TestSym.MainActivity+<>c__DisplayClass1_0.<OnCreate>b__0(C:\Users\pecolli\source\AndroidApp10TestSym\MainActivity.cs:20)
[AndroidRuntime] at Android.Views.View+IOnClickListenerImplementor.OnClick + 0xa(Unknown Source)
[AndroidRuntime] at Android.Views.View+IOnClickListenerInvoker.n_OnClick_Landroid_view_View_ + 0xe(Unknown Source)
[AndroidRuntime] at mono.android.view.View_OnClickListenerImplementor.n_onClick(Native Method)
[AndroidRuntime] at mono.android.view.View_OnClickListenerImplementor.onClick(View_OnClickListenerImplementor.java:29)
[AndroidRuntime] at android.view.View.performClick(View.java:8028)
[AndroidRuntime] at android.view.View.performClickInternal(View.java:8005)
I also couldn't manage to get VS to load Mono.Android.pdb from a local file path while debugging, and I'm not sure if we'll be able resolve from a symbol server either or if the .pdb file needs to be deployed to the target device/emu to get this information. That being said, I'm not sure how useful these paths/line numbers for generated code are? We also don't deploy any symbols from any other BCL assemblies.
Aside from this, all of our automated debugger tests are still passing against this PR on both Windows and macOS.
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.
Spent some more time trying to step into the base activity OnCreate method and was unable to do so with both .NET 9 and .NET 10 preview 2. I think we're reasonably confident we won't be regressing the debugging experience by removing Mono.Android.pdb at this time so I'll take this out of draft for further review.
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.
We probably need to
.pdb
for Mono.Android if we want to step through the code forMono.Android
.
The problem is that there is very little code within Mono.Android.dll
which can be stepped through. If the code is generated, we cannot step into it. @pjcollins confirmed this recently with .NET 9: set a breakpoint on base.OnCreate(bundle)
, and while you can "step into" it, you won't see any source code for it.
For example, from 7b4d4b8:
% $HOME/.dotnet/tools/sourcelink print-urls src/Mono.Android/obj/Debug/net10.0/android-36/Mono.Android.pdb
bd429029b775f558c3ca50e4735d64decd034427991fe9ba9be5cf31e2588455 sha256 csharp …/src/dotnet/android/src/Mono.Android/obj/Debug/net10.0/android-36/mcw/Android.App.Activity.cs
embedded
…
which yes says "embedded", but afaict nothing is able to view that embedded data?
Testing with an official build here, I'll need to run this through the symbol archiving and VS insertion process to make sure we are still processing and retaining symbols from the new symbols.nupkg artifacts correctly - https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=11352883&view=results |
No Android related symbols issues were reported in this PR https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/626310, I believe this should be good to go from an official build perspective as well |
Conflicts: build-tools/automation/yaml-templates/build-linux.yaml
( |
Our workload packaging logic has been updated to use more of the
Microsoft.DotNet.SharedFramework.Sdk MSBuild SDK included in the
dotnet/arcade repo.
This SDK allows us to easily exclude the symbols files that we've been
shipping directly in all of our workload packs, and instead move them
into
*.symbols.nupkg
packages that can be archived and uploaded toa symbol server separately.