Skip to content
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

Add diagnostic support into sample app and AppBuilders on Mono. #53361

Merged
merged 3 commits into from
May 31, 2021

Conversation

lateralusX
Copy link
Member

@lateralusX lateralusX commented May 27, 2021

PR simplifies enabling diagnostics on Android and iOS sample apps. Normal build process would first enable diagnostics_tracing runtime component using Android|AppleAppBuilder RuntimeComponents property. In order to activate diagnostics when running the app, DOTNET_DiagnosticPorts env variable needs to be set before staring up app. Since the process of setting env variables work differently between Android and iOS as well as how the app is executed on simulator/emulator/device/debugger, adding ability to put the value of DOTNET_DiagnosticPorts env variable directly into the launcher would simplify the process. The launcher is also rebuild when changing msbuild arguments making it possible to turn on/off diagnostics support for different configurations and get a rebuild of needed sources. This works similar to how logging, full aot and interpreter is handled in the sample apps.

Setting the DOTNET_DiagnosticPorts env in launcher is controlled using a Android|AppleAppBuilder property called DiagnosticPorts. Its possible to change the values directly in call to AppBuilder or use varaibles defined in existing make files. Just setting DiagnosticPorts and not set RuntimeComponents to at least include diagnostics_tracing will generate a build error.

PR also adds logic to shutdown runtime before exiting. This is needed to make sure any EventPipe sessions not close, will be correctly flushed and closed when shutting down EventPipe or produced streams will be incomplete, lacking needed rundown events.

@ghost
Copy link

ghost commented May 27, 2021

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

PR simplifies enabling diagnostics on Android and iOS sample apps. Normal build process would first enable diagnostics_tracing runtime component using Android|AppleAppBuilder RuntimeComponents property. In order to activate diagnostics when running the app, DOTNET_DiagnosticPorts env variable needs to be set before staring up app. Since the process of setting env variables work differently between Android and iOS as well as how the app is executed on simulator/emulator/device/debugger, adding ability to put the value of DOTNET_DiagnosticPorts env variable directly into the launcher would simplify the process. The launcher is also rebuild when changing msbuild arguments making it possible to turn on/off diagnostics support for different configurations and get a rebuild of needed sources. This works similar to how logging, full aot and interpreter is handled in the sample apps.

Setting the DOTNET_DiagnosticPorts env in launcher is controlled using a Android|AppleAppBuilder property called DiagnosticPorts. Its possible to change the values directly in call to AppBuilder or use varaibles defined in existing make files. Just setting DiagnosticPorts and not set RuntimeComponents to at least include diagnostics_tracing will generate a build error.

PR also adds logic to shutdown runtime before exiting. This is needed in to make sure any EventPipe sessions not close, will be correctly flushed and closed when shutting down EventPipe.

Author: lateralusX
Assignees: -
Labels:

area-Infrastructure-mono

Milestone: -

Comment on lines +7 to +10
#RUNTIME_COMPONENTS=diagnostics_tracing
#DIAGNOSTIC_PORTS=10.0.2.2:9000,nosuspend
#DIAGNOSTIC_PORTS=10.0.2.2:9000,suspend
#DIAGNOSTIC_PORTS=$(DOTNET_DiagnosticPorts)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just setting DiagnosticPorts and not set RuntimeComponents to at least include diagnostics_tracing will generate a build error.

Since they're all commented out, can it be noted in the makefiles that RUNTIME_COMPONENTS and DIAGNOSTIC_PORTS should be uncommented together if at all?

Also, out of curiosity, how were 10.0.2.2:9000 and 127.0.0.1:9000 selected (having no prior context of diagnostic ports)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add more comments into the script.

There are no dedicated port numbers, so could be anything.

10.0.2.2 is how you reach host loopback from Android emulator (without need to use adb). On iOS, 127.0.0.1 can still be used from simulator. But as above these are just examples and might need to be adjusted if you run different scenario.

/p:TargetArchitecture=$(MONO_ARCH) \
/p:UseLLVM=$(USE_LLVM) \
/p:ForceAOT=$(AOT) \
'/p:RuntimeComponents="$(RUNTIME_COMPONENTS)"' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this format
'/p:RuntimeComponents="$(RUNTIME_COMPONENTS)"' \
instead of like the previous formats
/p:RuntimeComponents=$(RUNTIME_COMPONENTS) \

Copy link
Member Author

@lateralusX lateralusX May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it includes , in the value it needs to be quoted or you will get msbuild argument parsing errors, just putting "" around value is not enough so complete property needs to be quoted as well in order to correctly go through shell parsing rules, problem is described here:

dotnet/msbuild#2999

and there are various workarounds to the problem, one of them is in this PR, but also suggested in different places, like here dotnet/msbuild#471 (comment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good to know, thanks for the explanation :)

Copy link
Member

@mdh1418 mdh1418 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@lateralusX lateralusX merged commit e41a250 into dotnet:main May 31, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Jun 1, 2021
…asm_debugger_and_use_debugger_agent

* upstream/main: (597 commits)
  Fix42292 (dotnet#52463)
  [mono] Remove some obsolete emscripten flags. (dotnet#53486)
  Fixed path to projects (dotnet#53435)
  support ServerCertificateContext in quic (dotnet#53175)
  Socket: delete unix local endpoint filename on Close (dotnet#52103)
  [mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
  Refactor MsQuic's native IP address types. (dotnet#53461)
  Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
  Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
  Fix issues with virtuals and mdarrays (dotnet#53400)
  Split Variant marshalling tests (dotnet#53035)
  Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
  remove WSL checks in tests (dotnet#53475)
  Always spawn message loop thread for SystemEvents (dotnet#53467)
  add AcceptAsync cancellation overloads (dotnet#53340)
  Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
  Add CookieContainer.GetAllCookies (dotnet#53441)
  Remove DynamicallyAccessedMembers on JsonSerializer  (dotnet#53235)
  [wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
  [libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Jun 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants