Conversation
| // 1. Bootstrap can detect hybrid mode and keep the package graph empty | ||
| // 2. On Win10, catalog loading during DllMain can expand loadFrom env vars | ||
| #if MICROSOFT_WINDOWSAPPSDK_AUTOINITIALIZE_HYBRIDDEPLOYSETUP | ||
| global::System.Environment.SetEnvironmentVariable("MICROSOFT_WINDOWSAPPRUNTIME_HYBRID_DEPLOY", "1"); |
There was a problem hiding this comment.
env vars should be avoided if at all possible. MICROSOFT_WINDOWSAPPRUNTIME_FRAMEWORK_PATH and MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY are required for loadFrom. But the boolean MICROSOFT_WINDOWSAPPRUNTIME_HYBRID_DEPLOY could instead be an API that's explicitly set by the autoinitialization source. Or better yet, can we just infer it from the existing and differing values of MICROSOFT_WINDOWSAPPRUNTIME_FRAMEWORK_PATH and MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY?
There was a problem hiding this comment.
env vars should be avoided if at all possible
+1000
We should be seriously examining dev\UndockedRegFreeWinRT\UndockedRegFreeWinRT-AutoInitializer.cs to see how we can remove
// Set base directory env var for PublishSingleFile support (referenced by SxS redirection)
Environment.SetEnvironmentVariable("MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY", AppContext.BaseDirectory);
not lean in even more heavily on it.
Q: If we need altered Assembly.Load*() behavior why aren't we adding a Custom Load Context to .NET callers? We're already adding *autoiniitalizer*.cs to .NET projects through build magic. What's a little more code?
|
Have we tested PublishSingleFile to ensure it still works and also supports hybrid now? That was the reason MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY was originally introduced. |
|
Definitely want to get @DrusTheAxe's opinion on these changes - particularly any consequences of removing the package dependency to keep the graph empty. |
There's no description of what "Hybrid" means, why it exists or what its goals are. Makes it hard to make any meaningful analysis. What I see from the code raises multiple questions and concerns, but lacking a design spec it's hard to assess any clearer than that. The Dynamic Dependencies spec includes the bootstrapper (because the bootstrapper was conceived, related and intimately dependent on winappsdk's DynDep polyfill implementation). Perhaps it would be better to start with spec to iron out intent, goals, caveats, etc before jumping to code? |
Oh sorry I didn't paste the whole background here. I had a spec about the feature background, overall changes, and the reason for changes at https://microsoft.visualstudio.com/ProjectReunion/_git/WindowsAppSDKAggregator/pullrequest/14931000?path=/docs/hybrid-deployment-spec-v2.md&_a=files In short, this feature is to support the scenario that "An application developer may want to use a newer version of a specific component package (e.g., the latest WinUI or Foundation) while continuing to depend on the currently installed runtime package for everything else." To support this, I had 3 draft PRs in Foundation, Aggregator and WWinUI: |
DrusTheAxe
left a comment
DrusTheAxe
left a comment
There was a problem hiding this comment.
I left comments about implementation details which are just details
More importantly, I left comments and questions about the requirements, intent and design underlying 'hybrid deploy'. This seems a critical gap we need to address first.
| const MddAddPackageDependencyOptions addOptions{}; | ||
| MDD_PACKAGEDEPENDENCY_CONTEXT tempContext{}; | ||
| wil::unique_process_heap_string packageFullName; | ||
| THROW_IF_FAILED(MddCore::Win11::AddPackageDependency(packageDependencyId.get(), MDD_PACKAGE_DEPENDENCY_RANK_DEFAULT, addOptions, &tempContext, &packageFullName)); |
There was a problem hiding this comment.
MddCore::Win11::* are calls to the OS DynDep APIs
I don't understand the intent. It looks like it's playing hide'n'seek with OS DynDep and winappsdk OS-alternative polyfill hacks (SetFrameworkPathEnvironmentVariable, AddDllDirectory, etc).
This is all terribly confusing and disturbing, unless I'm missing something.
| auto& activityContext{ WindowsAppRuntime::MddBootstrap::Activity::Context::Get() }; | ||
| activityContext.SetInitializationPackageFullName(packageFullName.get()); | ||
|
|
||
| // Track our initialized state (keep packageDependencyId alive for lifetime protection) |
There was a problem hiding this comment.
Say whaaat? Track it...but we just removed it (line 487)
| // Resolve framework package install path from package full name | ||
| static std::wstring GetFrameworkPackagePath(PCWSTR packageFullName) | ||
| { | ||
| UINT32 pathLength{ 0 }; |
| if (rc != ERROR_INSUFFICIENT_BUFFER) | ||
| { | ||
| THROW_WIN32(rc); | ||
| } |
There was a problem hiding this comment.
NIT: THROW_HR_IF(HRESULT_FROM_WIN32(rc), rc != ERROR_INSUFFICIENT_BUFFER);
| } | ||
|
|
||
| // Resolve framework package install path from package full name | ||
| static std::wstring GetFrameworkPackagePath(PCWSTR packageFullName) |
There was a problem hiding this comment.
Better to just use GetInstallPath<std::wstring>(pkgfullname) from #6200
That's only 2 steps away from checkin
- Address Letao's codereview feedback
- Resolve the RS5 test failure
| SetEnvironmentVariableW(L"MICROSOFT_WINDOWSAPPRUNTIME_FRAMEWORK_PATH", frameworkPath); | ||
| } | ||
|
|
||
| // For C++ apps (no C# auto-initializer), set BASE_DIRECTORY to exe dir if not already set |
There was a problem hiding this comment.
Why is this needed?
Why is it needed by C++ but not C#?
Does Rust need it?
Does Python?
Does Node?
...
MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY was added in 1.0? as an emergency workaround. Has it ever been spec'd? 100line comment somewhere?
| static void SetBaseDirectoryEnvironmentVariableIfNotSet() | ||
| { | ||
| wchar_t existing[2]{}; | ||
| if (GetEnvironmentVariableW(L"MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY", existing, ARRAYSIZE(existing)) > 0) |
There was a problem hiding this comment.
This is very loose in behavior
SET MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY=1
SET MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY=0
SET MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY=x
SET MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY=hello
...
But hard to assess how problematic as I can't find docs or spec about MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY.
P.S. For an example how to deal with this sort of hackery in a more rigorous fashion see IsLifetimeManagerViaEnumeration() in dev\WindowsAppRuntime_BootstrapDLL\MddBootstrap.cpp
| wchar_t existing[2]{}; | ||
| if (GetEnvironmentVariableW(L"MICROSOFT_WINDOWSAPPRUNTIME_BASE_DIRECTORY", existing, ARRAYSIZE(existing)) > 0) | ||
| { | ||
| return; // Already set by C# auto-initializer |
There was a problem hiding this comment.
If the C# auto-initializer handles this for C#
why can't/shouldn't the C++ auto-initializer handle it for C++?
| { | ||
| return; // Already set by C# auto-initializer | ||
| } | ||
| wchar_t exePath[MAX_PATH]{}; |
| return; // Already set by C# auto-initializer | ||
| } | ||
| wchar_t exePath[MAX_PATH]{}; | ||
| if (GetModuleFileNameW(nullptr, exePath, ARRAYSIZE(exePath)) > 0) |
There was a problem hiding this comment.
wil::GetModuleFileNameW() beats raw GetModuleFileNameW()
You can find uses in winappsdk e.g. dev\WindowsAppRuntime_BootstrapDLL\MddBootstrap.cpp line 1274 in MddBootstrapInitialize_ShowUI_OnNoMatch()
sure, I fully agree the very first thing is we should get an agreement about the background and there is no big concern about the changes. I've added link towards the sepc and 3 draft PRs, and I also added you as review for those PRs. Looking forward for any feedback / concerns / other suggestions. Thank you @DrusTheAxe |
3 participants
A microsoft employee must use /azp run to validate using the pipelines below.
WARNING:
Comments made by azure-pipelines bot maybe inaccurate.
Please see pipeline link to verify that the build is being ran.
For status checks on the main branch, please use TransportPackage-Foundation-PR
(https://microsoft.visualstudio.com/ProjectReunion/_build?definitionId=81063&_a=summary)
and run the build against your PR branch with the default parameters.