Add IsPackagedProcess and IsSelfContained to common insights PartB fields#6233
Add IsPackagedProcess and IsSelfContained to common insights PartB fields#6233
Conversation
|
@DrusTheAxe what do you think of this? Getting some signal on selfcontained vs fwp dependent, and packages vs unpackaged is something we've wanted for a while. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Good idea, but this information retrieval happens on every TraceLoggingWrite. Not too heavy a cost? |
|
Please "Resolve conversation" as you address issues. It helps make it easy to spot what was addressed vs still open/pending |
| inline HRESULT IsSelfContained_nothrow(bool& isSelfContained) noexcept | ||
| { | ||
| isSelfContained = false; | ||
| auto module = ::GetModuleHandleW(L"Microsoft.WindowsAppRuntime.dll"); |
There was a problem hiding this comment.
Silently swallows errors. if !module it would be better to report what happened
WIL is your friend :-)
There was a problem hiding this comment.
!module is the normal case when the DLL isn't loaded yet (like during bootstrap). If we RETURN_LAST_ERROR_IF_NULL here it'd bubble up as a real error. Would something like LOG_LAST_ERROR_IF_NULL + still returning S_OK be a good middle ground?
There was a problem hiding this comment.
Ahhh, but in this case it is an error. It's the caller's decision if that's a problem for them, or if they want to ignore it as 'expected'
If this is called before the DLL is loaded (e.g. bootstrap) then would the caller hit this again later and get a different answer?
Given the TraceLogging callsite calls this on 1st use and caches the result the question is, does that 1st call happen after bootstrapping? If not how would that cached answer be revised - or would it always be wrong 'cause too early?
There was a problem hiding this comment.
I think the logic still works out: self-contained apps already have the DLL loaded (no
bootstrap), so GetModuleHandleW succeeds on first call. Framework-dependent apps use the bootstrapper, and they aren't self-contained anyway, so the cached false is correct either way?
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@DrusTheAxe ready for another review, thanks! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2 participants
Add IsPackagedProcess and IsSelfContained bools to PartB fields to get signal on self-contained vs framework-dependent and packaged vs unpackaged deployments