Add Dev Environments feature into main from feature branch #2346

bbonaby · 2024-03-05T00:44:56Z

Summary of the pull request

This PR moves the Dev Environment code from the feature branch to Dev Home main. The move includes the following

Hyper-V Dev Home extension
1.1. The extension includes separate projects for the DevSetupAgent service and DevSetupEngine COM server. These allow configuration to be done within a virtual machine running the Windows OS.
An environments tool page that allows users to manage their Dev Environments
A new Setup target flow that allows users to configure their environments in the machine configuration page.
DevHome.Common and the Hyper-V extension now use the new Dev SDK version.

Note: The Azure extension will house the code for the Dev Box environment and a second PR to move that code from a feature branch within the azure extension, to the azure extensions main branch will happen after this one is complete.

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

Rebuilt Dev Home and confirmed all preview and canary versions of extensions are still working. Github extension and Azure extension all still clone repositories and allow users to login through their respective flows. Core widgets are also still working as expected. Also confirmed that I can still install apps.

The DevHome Common project now uses the new published Dev Home SDK: 0.200.427

PR checklist

Closes Add VM and Cloud Environment Support to Dev Home #2166
Tests added/passed
Documentation updated

* Add initial code from private branch that will be shared between the setup flow and the Dev environments tool page. PRs: from private ADO repo: https://dev.azure.com/microsoft/Dart/_git/DevHome?version=GBDevEnv * add changes to the setup flow for dev environment configuration PRS: from private ADO repo: https://dev.azure.com/microsoft/Dart/_git/DevHome?version=GBDevEnv * Add dev environments management tool page from private ADO repo: https://dev.azure.com/microsoft/Dart/_git/DevHome?version=GBDevEnv * fix conflicts and stylecop errors due to update from merge with main --------- Co-authored-by: Huzaifa Danish <[email protected]>

…ironments feature branch (#2246) * Add Hyper-v extension to Dev Home from Private Hyper-v extension branch: PRS https://github.com/microsoft/DevHomeHyperVExtension * Merge changes from Dev Home main and fix style cop errors

…rV extension. (#2258)

…ice to a Hyper-V VM. (#2261)

…Adaptive Cards to Hyper-V Configure command. (#2289)

…d DevSetupAgent_*.zip to MSIX package. (#2325) * Add WaitForLogin and Credentials Adaptive Cards * Address review comments. * New VS solution for DevSetupAgent * Add BuildDevSetupAgentHelper script * Fix x86 DevSetupAgent to work on x64 OS. Create DevSetupAgent zip for different VM architectures. Fixed build scripts and HyperVExtension.csproj to include DevSetupAgent zip file into Dev Home MSIX package. * Remove old comment. * Fix a comment.

…m/microsoft/devhome into microsoftfeature/dev-environments

…ent. (#2321) * initial code * update messaging and adaptive cards * remove added method * update strings and names * update based on initial comments and update IsHyperVModuleLoaded with new work around that doesn't involve installing using Install-Module which installs from PsGallery * improve wording * fix merge conflicts

* update feature branch to latest sdk * update InputJson to inputJson * update with latest changes

* add updates to ui * fix crashes due to resource name mismatch * update feature branch to latest sdk * update InputJson to inputJson * update with latest changes * update * update to remove duplicate, resize shimmer, remove id from winget file since its not needed, and add comments * Re-add correct adaptive host file for correct theming, address some initial comments, update loading page to allow scrolling when there are tasks and actions in the action center. Fix error message spelling. * improve setup target loading page progress messaging * Fix git clone's dependsOn Id to match our new Ids for setup target flow

…2339)

…app sdk v1.5 to prevent build issues (#2344)

EricJohnson327

Blocking checkin until core team has a chance to look tomorrow.

HyperVExtension/DevSetupAgent.sln

extensionsdk/Microsoft.Windows.DevHome.SDK/ComputeSystemOperationData.cpp

EricJohnson327 · 2024-03-05T01:18:04Z

src/App.xaml.cs

@@ -105,6 +106,9 @@ public App()
            services.AddSingleton<IAppInstallManagerService, AppInstallManagerService>();
            services.AddSingleton<IPackageDeploymentService, PackageDeploymentService>();
            services.AddSingleton<IScreenReaderService, ScreenReaderService>();
+            services.AddSingleton<IComputeSystemService, ComputeSystemService>();
+            services.AddSingleton<IComputeSystemManager, ComputeSystemManager>();
+            services.AddSingleton<ToastNotificationService>();


@dkbennett do we have toast notifications in Dev Home core already? I just want to make sure we don't have 2 solutions now.

For what it's worth we added this here now because we needed a way to tell the user that they arent in the Hyper-V admin group when they open the environments tool page or the "Setup a target" flow in machine configuration. (You can't do any Hyper-V admin functionality unless you are in an elevated process or you're in the Hyper-v admin group). E.g the Hyper-V manager in the Windows OS actually runs elevated, we just don't see the UAC.

But we have plans to actually create an elevated process to add the user to this group, without the need to show a toast notification (work is to be done before Build), in which case we'd remove this. Though I know there are other ways to tell the user this without the need for the toast notification, it's what we ended up doing for now. So, in the end, this is only temporary. If David already has a way to send notifications, we can use that. Otherwise, just know this is temporary.

This is totally fine. I know David added notifications to the extensions, but I wanted to check if there was existing code in Dev Home to do this.

Consider adding an interface for this service for mock-ability

src/Models/ExtensionWrapper.cs

src/Package.appxmanifest

krschau · 2024-03-05T14:56:27Z

HyperVExtension/BuildDevSetupAgentHelper.ps1

+}
+
+if (-not $BypassWarning) {
+  Write-Host @"


As an FYI (I looked this up recently) powershell scripts conventionally have 4 space indents (not blocking)

Number of spaces per intent is a religion, we just need to agree what we believe in 😄. The existing Build.ps1 and BuildSDKHelper.ps1 have 2 space intents. This script follows what we already have.

We updated Build.ps1 recently to have 4, but I understand we're quite inconsistent now. This was more of a "we came to a conclusion that we'll try to follow going forward" note. (Not that everyone's happy with that conclusion 😄 )

If it's 4 now, then this one can be changed. I have no problem with any reasonable number (I worked for a company where it was 3).

3 is simply ridiculous :) I'm the one that doesn't like 4 spaces, but I concede it's become the standard.

krschau · 2024-03-05T15:11:54Z

src/Services/ExtensionService.cs

@@ -110,42 +114,34 @@ private async Task<bool> IsValidDevHomeExtension(Package package)
            if (package.Id.FullName == extension.Package.Id.FullName)
            {
                var (devHomeProvider, classId) = await GetDevHomeExtensionPropertiesAsync(extension);


Should this be classIds now?

src/Services/ExtensionService.cs

krschau · 2024-03-05T15:18:22Z

tools/Environments/DevHome.Environments/DevHome.Environments.csproj

+
+	<ItemGroup>
+		<PackageReference Include="CommunityToolkit.WinUI.Collections" Version="8.0.240109" />
+		<PackageReference Include="Microsoft.WindowsAppSDK" Version="1.5.240227000" />


Remove WinAppSDK, you get it free for including DevHome.Common (and we don't want more places to have to keep updated). You can also remove Converters, Shimmer, Animations, and Segmented.

krschau · 2024-03-05T15:21:03Z

tools/Environments/DevHome.Environments/DevHome.Environments.csproj

+
+	<ItemGroup>
+		<ProjectReference Include="..\..\..\common\DevHome.Common.csproj" />
+		<ProjectReference Include="..\..\..\logging\DevHome.Logging.csproj" />


Remove Logging, you get it for free with Common

It's not entirely free. It brings cross-project dependencies that we don’t want to have (at least not now). Part of the projects in this PR are built separately from the main solution and don’t go into Dev Home’s output directly, but rather compressed into a .zip file to be deployed on a Hyper-V VM. The additional cross-project dependencies make building those projects more difficult. Also, it’s likely we will change .NET version for those projects from .NET8 to .NET Framework to reduce output size since .NET Framework is pre-installed on Windows. That can introduce additional problems if we can change some of the code to be compatible with .NET Framework.

I suppose I shouldn't have said "free", but including DH.Common makes DH.Logging redundant. I would understand if you wanted Logging without the rest of Common, but since you already have Common, I'm not sure what Logging gets you.

I actually tried to remove it about a week ago and it created more problems with those special projects that I had time to fix.

krschau · 2024-03-05T15:37:31Z

tools/SetupFlow/DevHome.SetupFlow/DevHome.SetupFlow.csproj

+
+  <ItemGroup>
+    <Folder Include="NewFolder\" />
+  </ItemGroup>


Guessing this doesn't exist and should be removed.

krschau · 2024-03-05T15:39:30Z

tools/Environments/DevHome.Environments/DevHome.Environments.csproj

+	</ItemGroup>
+
+	<ItemGroup>
+		<Folder Include="Converters\" />


Converters, Models, and Templates all don't exist, remove.

HyperVExtension/src/DevSetupAgent/DevSetupAgent.csproj

krschau · 2024-03-05T16:17:04Z

common/Environments/CustomControls/CardBody.xaml

+        <devEnvConverters:CardStateToLocalizedTextConverter x:Key="CardStateToLocalizedTextConverter"/>
+    </UserControl.Resources>
+
+        <Grid


Indentation in this file is messed up (not blocking)

krschau · 2024-03-05T16:33:36Z

HyperVExtension/src/DevSetupAgent/DevSetupAgent.csproj

+    <ProjectReference Include="..\Telemetry\HyperVExtension.Telemetry.csproj" />
+  </ItemGroup>
+  <ItemGroup>
+    <PackageReference Update="Microsoft.CodeAnalysis.NetAnalyzers" Version="7.0.4">


Consider using the .net analyzers you get for free, that's what the rest of Dev Home does (see that Directory.Build.Props has true)

".NET compiler platform (Roslyn) analyzers inspect your C# or Visual Basic code for code quality and style issues. Starting in .NET 5, these analyzers are included with the .NET SDK and you don't need to install them separately. If your project targets .NET 5 or later, code analysis is enabled by default. If your project targets a different .NET implementation, for example, .NET Core, .NET Standard, or .NET Framework, you must manually enable code analysis by setting the EnableNETAnalyzers property to true."
https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/overview?tabs=net-8

Is there a specific reason you chose 7.0.4 and not 8.0.0?

No reason other than this project was started separately from Dev Home as an extension that would have its own GitHub repo and used whatever version of Microsoft.CodeAnalysis.NetAnalyzers existed at that time. So, it can be removed now.

krschau · 2024-03-05T16:43:29Z

HyperVExtension/src/HyperVExtensionServer/HyperVExtensionServer.csproj

+  <ItemGroup>
+    <Folder Include="Assets\" />
+  </ItemGroup>


Doesn't exist, remove.

krschau · 2024-03-05T16:44:52Z

HyperVExtension/src/HyperVExtensionServer/HyperVExtensionServer.csproj

+    </PackageReference>
+    <PackageReference Include="Microsoft.Extensions.Hosting" Version="8.0.0" />
+    <PackageReference Include="Microsoft.Toolkit.Uwp.Notifications" Version="7.1.3" />
+    <PackageReference Include="Microsoft.WindowsAppSDK" Version="1.5.240227000" />


Included in HyperVExtension.Common, remove here. Microsoft.Extensions.Hosting too.

Can you do a scan for these in all the projects under HyperVExtension? GH is really laggy with a PR this big and me trying to jump between files is not working well 😅

krschau · 2024-03-05T17:00:19Z

The only thing I'm really blocking on above is the missing null check, but if you could also remove some of the references to things that don't exist, I think that would be a pretty quick thing to do.

As an FYI, the codebase uses spaces instead of tabs (.sln files are the only ones that don't follow this) and crlf line endings instead of lf. I pushed these changes just to take it off your plate, hope I didn't step on any toes.

The additional things we would like to see here (that we've agreed will go in separately) are

Move the HyperVExtension directory under Extensions, and possibly move other things
Adding the new projects to the mermaid diagram here: https://github.com/microsoft/devhome/blob/main/docs/architecture.md

EricJohnson327

Pending the null fix Kristen mentioned and the github build pipeline break, code looks good to me. Please fix those before checking in.

AmelBawa-msft · 2024-03-05T23:11:24Z

HyperVExtension/src/DevSetupAgent/DevAgentService.cs

+{
+    private readonly IHost _host;
+
+    public DevAgentService(IHost host)


Are IHostChannel and IRequestManager singleton? If so avoid directly injecting IHost, instead directly inject IHostChannel and IRequestManager. If they are transient, consider registering a factory class/function.

AmelBawa-msft · 2024-03-05T23:31:17Z

src/App.xaml.cs

@@ -105,6 +106,9 @@ public App()
            services.AddSingleton<IAppInstallManagerService, AppInstallManagerService>();
            services.AddSingleton<IPackageDeploymentService, PackageDeploymentService>();
            services.AddSingleton<IScreenReaderService, ScreenReaderService>();
+            services.AddSingleton<IComputeSystemService, ComputeSystemService>();
+            services.AddSingleton<IComputeSystemManager, ComputeSystemManager>();
+            services.AddSingleton<ToastNotificationService>();


Consider adding an interface for this service for mock-ability

AmelBawa-msft · 2024-03-05T23:40:34Z

src/App.xaml.cs

@@ -176,6 +183,12 @@ protected async override void OnLaunched(Microsoft.UI.Xaml.LaunchActivatedEventA

    private void OnActivated(object? sender, AppActivationArguments args)
    {
+        if (args.Kind == ExtendedActivationKind.ToastNotification)


I think we can leverage IActivationHandler here by adding a new activation handler for the type ToastNotificationActivatedEventArgs.

Check ProtocolActivationHandler.cs for an example.

AmelBawa-msft · 2024-03-05T23:44:17Z

tools/SetupFlow/DevHome.SetupFlow/Models/ConfigurationUnitResult.cs

@@ -38,6 +41,23 @@ public ConfigurationUnitResult(ElevatedConfigureUnitTaskResult result)
        ErrorDescription = result.ErrorDescription;
    }

+    public ConfigurationUnitResult(SDK.ApplyConfigurationUnitResult result)


Is this a permanent SDK change?

yea for the Dev Environments project we allow environments to be configured with WinGet configure by passing a config file as a string to the extension. However from an extensions point of view, there are times where they'd need to actually translate what they get from WinGet on a remote machine e.g a local VM or over the network back to the Dev Home extension. For example this can be done through strings, but now Dev Home and the extension need a common way to understand what is going on with the configuration (interpreting this string). So, we created a subset of WinGet interfaces/runtime classes in the SDK that extensions can then translate WinGet progress and results into, then send back to Dev Home via the SDK. From there once we receive it in Dev Home. We can reuse some of the classes we're using currently in the Configuration flow.

John from the WinGet team was on board with using a subset of the winget interfaces for this purpose. I can share the api review doc with you if you like.

AmelBawa-msft · 2024-03-05T23:47:53Z

tools/SetupFlow/DevHome.SetupFlow/ViewModels/PackageViewModel.cs

@@ -75,13 +76,15 @@ public partial class PackageViewModel : ObservableObject
        IWinGetPackage package,
        IThemeSelectorService themeSelector,
        IScreenReaderService screenReaderService,
-        IHost host)
+        IHost host,


You can remove host here and just keep the orchestrator

sure, I can update this in a future PR

krschau

Removing my block

… progress reporting, and GitHub build. (#2348)

bbonaby and others added 21 commits February 8, 2024 16:44

add initial SDK changes (#2209)

970a573

Merge branch 'main' into microsoftfeature/dev-environments

482d529

Merge branch 'main' into microsoftfeature/dev-environments

ca94296

Initial implementation of IComputeSystem::ApplyConfiguration for Hype…

eec8889

…rV extension. (#2258)

Add environments to experimentation (#2260)

2f4186a

Add PowerShell script and a helper class to deploy DevSetupAgent serv…

cef8480

…ice to a Hyper-V VM. (#2261)

Hyper-V extension: Add "Ask for VM credentials" and "Wait for logon" …

caa2a92

…Adaptive Cards to Hyper-V Configure command. (#2289)

Changes to make DevSetupEngine work with .NET 8 (#2308)

1c4127c

Merge branch 'main' into microsoftfeature/dev-environments

4eed026

Merge branch 'microsoftfeature/dev-environments' of https://github.co…

d0634aa

…m/microsoft/devhome into microsoftfeature/dev-environments

update dev environments feature branch with latest idl changes (#2334)

1bb1d15

* update feature branch to latest sdk * update InputJson to inputJson * update with latest changes

Update build scripts to build DevSetupAgent in Azure official build. (#…

075af1f

…2339)

Merge branch 'main' into microsoftfeature/dev-environments

4ec9e02

Merge branch 'main' into microsoftfeature/dev-environments

532e26a

update dev home to use new SDK version and other projects to use win …

5ad9d74

…app sdk v1.5 to prevent build issues (#2344)

Fix PS DevSetupAgent deployment script. (#2345)

a72e7ea

bbonaby requested review from kanismohammed and sshilov7 March 5, 2024 00:45

Merge branch 'main' into microsoftfeature/dev-environments

3271804

bbonaby requested review from huzaifa-d and EricJohnson327 March 5, 2024 00:48

EricJohnson327 requested changes Mar 5, 2024

View reviewed changes

EricJohnson327 reviewed Mar 5, 2024

View reviewed changes

HyperVExtension/DevSetupAgent.sln Show resolved Hide resolved

EricJohnson327 reviewed Mar 5, 2024

View reviewed changes

extensionsdk/Microsoft.Windows.DevHome.SDK/ComputeSystemOperationData.cpp Outdated Show resolved Hide resolved

kanismohammed approved these changes Mar 5, 2024

View reviewed changes

EricJohnson327 reviewed Mar 5, 2024

View reviewed changes

src/Models/ExtensionWrapper.cs Show resolved Hide resolved

EricJohnson327 reviewed Mar 5, 2024

View reviewed changes

src/Package.appxmanifest Show resolved Hide resolved

bbonaby and others added 3 commits March 4, 2024 17:42

remove classes that don't need to be added

60a9139

Change all tabs to spaces

e0c0a00

Update tools directory to use crlf

94d116b

krschau force-pushed the microsoftfeature/dev-environments branch from 754bae2 to 94d116b Compare March 5, 2024 15:49

krschau added 3 commits March 5, 2024 11:05

Update HyperVExtension directory to use crlf

79baa9d

Update common/Environments directory to use crlf

7d8c2ee

Update more in common directory to use crlf

15af544

krschau requested changes Mar 5, 2024

View reviewed changes

krschau reviewed Mar 5, 2024

View reviewed changes

EricJohnson327 approved these changes Mar 5, 2024

View reviewed changes

AmelBawa-msft reviewed Mar 5, 2024

View reviewed changes

krschau approved these changes Mar 6, 2024

View reviewed changes

sshilov7 and others added 3 commits March 6, 2024 13:36

[Hyper-V extension]: Fixes for DevSetupEngine registration, Configure…

0c11642

… progress reporting, and GitHub build. (#2348)

remove s that was added from a previous commit causing build to fail

b2bb209

fix tests for build

433ebe9

sshilov7 approved these changes Mar 7, 2024

View reviewed changes

update malformed configuration string to allow arm64 to build

fe542ed

bbonaby merged commit 1200e38 into main Mar 7, 2024
4 checks passed

krschau deleted the microsoftfeature/dev-environments branch August 14, 2024 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Dev Environments feature into main from feature branch #2346

Add Dev Environments feature into main from feature branch #2346

bbonaby commented Mar 5, 2024 •

edited by krschau

Loading

EricJohnson327 left a comment

EricJohnson327 Mar 5, 2024

bbonaby Mar 5, 2024

EricJohnson327 Mar 5, 2024

AmelBawa-msft Mar 5, 2024

krschau Mar 5, 2024 •

edited

Loading

sshilov7 Mar 5, 2024

krschau Mar 5, 2024

sshilov7 Mar 5, 2024

EricJohnson327 Mar 5, 2024

krschau Mar 5, 2024

krschau Mar 5, 2024

krschau Mar 5, 2024

sshilov7 Mar 5, 2024

krschau Mar 5, 2024

sshilov7 Mar 5, 2024

krschau Mar 5, 2024

krschau Mar 5, 2024

krschau Mar 5, 2024 •

edited

Loading

krschau Mar 5, 2024

sshilov7 Mar 5, 2024

krschau Mar 5, 2024

krschau Mar 5, 2024

krschau Mar 5, 2024

krschau commented Mar 5, 2024

EricJohnson327 left a comment

AmelBawa-msft Mar 5, 2024

AmelBawa-msft Mar 5, 2024

AmelBawa-msft Mar 5, 2024

AmelBawa-msft Mar 5, 2024

bbonaby Mar 6, 2024

AmelBawa-msft Mar 5, 2024

bbonaby Mar 6, 2024

krschau left a comment

Add Dev Environments feature into main from feature branch #2346

Add Dev Environments feature into main from feature branch #2346

Conversation

bbonaby commented Mar 5, 2024 • edited by krschau Loading

Summary of the pull request

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

EricJohnson327 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krschau Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krschau Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krschau commented Mar 5, 2024

EricJohnson327 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krschau left a comment

Choose a reason for hiding this comment

bbonaby commented Mar 5, 2024 •

edited by krschau

Loading

krschau Mar 5, 2024 •

edited

Loading

krschau Mar 5, 2024 •

edited

Loading