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

DeploymentManager spec update for the new Repair API #2869

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sachintaMSFT
Copy link
Contributor

@sachintaMSFT sachintaMSFT commented Aug 18, 2022

Deployment Manager Spec update to add the new Repair API for 1.3 release. Hence, incrementing the DeploymentContract to version 4.

@ghost ghost added the needs-triage label Aug 18, 2022
specs/Deployment/DeploymentAPI.md Outdated Show resolved Hide resolved
specs/Deployment/DeploymentAPI.md Outdated Show resolved Hide resolved
specs/Deployment/DeploymentAPI.md Outdated Show resolved Hide resolved
specs/Deployment/DeploymentAPI.md Outdated Show resolved Hide resolved
specs/Deployment/DeploymentAPI.md Outdated Show resolved Hide resolved
specs/Deployment/DeploymentAPI.md Outdated Show resolved Hide resolved
specs/Deployment/DeploymentAPI.md Outdated Show resolved Hide resolved
specs/Deployment/DeploymentAPI.md Show resolved Hide resolved
specs/Deployment/DeploymentAPI.md Show resolved Hide resolved
specs/Deployment/DeploymentAPI.md Show resolved Hide resolved
@sachintaMSFT sachintaMSFT added area-Deployment Issues related to packaging, installation, runtime (e.g., SelfContained, Unpackaged) feature and removed needs-triage labels Aug 19, 2022
@sachintaMSFT sachintaMSFT force-pushed the user/sachinta/DeploymentRepairAPISpec branch 2 times, most recently from 65cfd5a to ac61f62 Compare September 20, 2022 21:50
Copy link
Member

@DrusTheAxe DrusTheAxe left a comment

Choose a reason for hiding this comment

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

DON'T CHECKIN until API Review

specs/Deployment/DeploymentAPI.md Show resolved Hide resolved
architecture needed are derived from the current WinAppSDK Framework package.

Since both the Main _and_ the Singleton packages will be repaired, it is possible that Main package
repair has failed and Initialize returns with PackageRepairFailed status (OR) it is possible that
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "(OR)" mean here? "or"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is just "or". I should probably just use "or" to avoid such confusion.

repair has failed and Initialize returns with PackageRepairFailed status (OR) it is possible that
Main package is repaired successfully but the Singleton package repair has failed and Initialize
returns with repair has failed and Initialize returns with PackageRepairFailed status (OR) it is
possible that status. In both cases, the returned WindowsAppRuntimeStatus will contain the error of
Copy link
Contributor

Choose a reason for hiding this comment

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

both cases

I think it just listed 3 cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually just 2 cases only.

Copy link
Member

Choose a reason for hiding this comment

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

I see three cases in the text (separated by "OR"):

  1. it is possible that Main package repair has failed and Initialize returns with PackageRepairFailed status
  2. it is possible that Main package is repaired successfully but the Singleton package repair has failed and Initialize returns with repair has failed and Initialize returns with PackageRepairFailed status
  3. it is possible that status

Not sure what 3 is trying to say.

repair has failed and Initialize returns with PackageRepairFailed status (OR) it is possible that
Main package is repaired successfully but the Singleton package repair has failed and Initialize
returns with repair has failed and Initialize returns with PackageRepairFailed status (OR) it is
possible that status. In both cases, the returned WindowsAppRuntimeStatus will contain the error of
Copy link
Contributor

Choose a reason for hiding this comment

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

possible that status

Looks like there was more sentence intended here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I need to rectify that. Not sure how I ended up with that incomplete sentence there. But I wanted to mean "Main's repair could fail" or "Main repair succeeded but Singleton repair could fail".

specs/Deployment/DeploymentAPI.md Show resolved Hide resolved
@@ -143,18 +161,16 @@ by using DeploymentInitializeOptions object and setting ForceDeployment option b
this API. This option when set will shut down the application(s) associated with WinAppSDK packages,
update the WinAppSDK packages and restart the application(s).
Copy link
Contributor

Choose a reason for hiding this comment

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

If I don't set ForceDeployment true, the deploy will fail? And then the app should interact with the to say "on next reboot" or some such?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the packaged app doesn't call Initialize() or Repair() with ForceDeployment set and if the current install is actually an update because an older package version is already installed, then yes it does fail with an error that reads "Package in Use". There is no further story to it. If the intention of the developer is to use ForceDeployment eventually, then there is no reason to wait for the first call of Initialize to fail before calling it again with ForceDeployment set this time. The developer could as well have passed it in the first call of Initialize or Repair itself.

Explained what happens when ForceDeployment is set in your next feedback comment in detail.

framework package and all dependent packages that are currently in use will be re-installed, after
they shut down, to refer to the updated framework package.
Framework package and all dependent packages that are currently in use will be re-installed, after
they shut down, to refer to the updated Framework package.
Copy link
Contributor

Choose a reason for hiding this comment

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

after they shut down

Does this mean it will force the apps currently using the package to shut down, or does this mean that the Initialize() call could block indefinitely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will only force the Main, Singleton and DDLM packages to shut down in order to update Main, Singleton and DDLM packages (ex: PushNotifications would be shutdown as it's part of Singleton package) and the shutdown packages are restarted automatically by the OS platform after their updates are complete (and in cases such as PushNotifictions which would need explicit reboot as it's a com server, we do reboot it after the update in Initialize()/Repair()).

It doesn't shutdown the WinAppSDK framework package dependent packaged apps and installs the higher version of the framework package side by side without uninstalling the older version. It does update the WinAppSDK framework package dependent packaged apps (in their package graph) to point to higher version of the framework package, once they are shutdown by the user. Once all references to older version of the framework package are updated to refer to the higher version of it, the older version is then completely uninstalled by the OS platform.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if an app is currently using Main/Singleton/DDLM package (and I set Force)? What happens to the app?

}
```

Here's the previous example extended with the `Show UI on error` option:
Copy link
Contributor

Choose a reason for hiding this comment

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

This example doesn't match the above logic; the logic shows UI, the example doesn't. If the UI built in to the API and explains to the user what the problem is? Or if the logic is correct and the app needs to show UI, it's awkward because they can't use WinAppSDK to show the UI

Copy link
Member

@DrusTheAxe DrusTheAxe Oct 4, 2022

Choose a reason for hiding this comment

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

Yes, the UI shown is built into the API explaining the problem to the user (currently via MessageBox())

/// WindowsAppSDK main and singleton packages will be shut down forcibly if they are
/// currently in use, when registering the WinAppSDK packages.
/// WinAppSDK Main and Singleton packages will be shut down forcibly if they are
/// currently in use, when initializing the WindowsAppRuntime.
Copy link
Contributor

Choose a reason for hiding this comment

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

So when you start my app, you might see some other apps disappear? Is there any kind of interaction with the user first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Please see one of the above comments for detailed explanation on ForceDeployment. In short, ForceDeployment only shuts down WinAppSDK packages that are dependent on it's framework packages (ex: Main, Singleton and DDLM). It doesn't shut down any non-WinAppSDK packages that declared dependency on WinAppSDK framework package.

they shut down, to refer to the updated Framework package.

```C#
if (DeploymentManager.GetStatus().Status == DeploymentStatus.PackageInstallRequired)

Choose a reason for hiding this comment

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

This is in the DeploymentRepairOptions section, so this should be checking for == DeploymentStatus.PackageRepairRequired right?

Copy link
Contributor Author

@sachintaMSFT sachintaMSFT Oct 4, 2022

Choose a reason for hiding this comment

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

Yes. Need to check for == DeploymentStatus.PackageRepairRequired. Thanks for pointing that out.

the system?
3. How do I repair the already installed WinAppSDK Main and Singleton packages, if needed ?

Choose a reason for hiding this comment

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

Why do we need Repair to be a distinct action than Install/Initialize, instead of having developers [re]install the packages whenever they're "broken". This is something of a breaking change since we'd previously told developers to Initialize for any status other than .OK, right?

The manual method to repair by "reinstalling from msdn downloads page" is identical for install and repair. Why is the full-trust-app method different?

Copy link
Contributor Author

@sachintaMSFT sachintaMSFT Oct 6, 2022

Choose a reason for hiding this comment

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

Intialize installs/re-installs WinAppSDK Main and Singleton packages only when needed (i.e. when one of the packages is not installed or if one of them is in identified bad state). Thus, avoiding the unnecessary perf overhead of the unconditional re-install in every launch of the WinAppSDK dependent Packaged app.

Repair, on the other hand, re-installs those packages unconditionally. This API is important because there are many bad states a package can be in but only a few of them could be identified by Initialize and fix. Hence, we need Developers to just able to call Repair in cases where the WinAppSDK features aren't working due to a bad state that Initialize couldn't identify and fix it.

This is not a breaking change. Because the current usage of Initialize as detailed in the code sample in this spec prior to the current PR would still work for Initialize API (i.e. up until 1.2 WinAppSDK release). But the changes to code samples in this PR show a better recommended method in calling Initialize and Repair API conditionally, that developers can adopt with 1.3+ WinAppSDK release.

Manual method of using WindowsAppRuntimeInstall.exe can now, starting with 1.2 WinAppSDK release, use repair option explicitly. Once again, this is due to the desire to avoid unnecessary re-installs in default install path (ex: as complained by PowerToys team).

current WinAppSDK Framework package.

Since both the Main _and_ the Singleton packages may need to be installed, it is possible that Main
package install has failed and Initialize returns with PackageInstallFailed state (OR) it is

Choose a reason for hiding this comment

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

What happens if I try to Repair() a package that's in PackageInstallFailed state? That seems like a natural inclination ("I did Initialize() and it's in some error state now, I guess that means it's broken so I should fix it. Fix -> repair. Call Repair()"). Are we providing DeploymentResult properties that tell the developer they called the wrong method?

Copy link
Contributor Author

@sachintaMSFT sachintaMSFT Oct 6, 2022

Choose a reason for hiding this comment

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

Repair() essentially provides an unconditional core functionality of Initialize() - which is to install WindowsAppSDK Main and Singleton licenses and packages. So, if Repair() is tried when Initialize() fails, then it is effectively a retry of Initialize(). If the underlying issue for PackageInstallFailed is transient, then Repair() may succeed. Otherwise, it will continue to be PackageInstallFailed.

No, DeploymentResult doesn't have any property that calls out wrong method usage. Because calling Repair() when Initialize() fails is not a wrong method.

architecture needed are derived from the current WinAppSDK Framework package.

Since both the Main _and_ the Singleton packages will be repaired, it is possible that Main package
repair has failed and Initialize returns with PackageRepairFailed status (OR) it is possible that
Copy link
Contributor Author

@sachintaMSFT sachintaMSFT Oct 6, 2022

Choose a reason for hiding this comment

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

In this paragraph here, "Initialize returns with PackageRepairFailed status" should actually be "Repair returns with PackageRepairFailed status" at 2 instances.

// for the Status.
}
else if (result.Status == DeploymentStatus.PackageRepairRequired ||
result.Status == DeploymentStatus.Unknown)

Choose a reason for hiding this comment

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

what if the status is PackageInstallFailed ? Shouldn't I want to Repair() it in that case? (Since repair is unconditional core functionality of Initialize, is PackageInstallFailed a special-case or subset of PackageRepairRequired?)

// WindowsAppRuntime. result.ExtendedError has the error code that has the reason
// for the Status.
// WindowsAppRuntime is either not installed or not in a good state (can't be both at same time).
if (result.Status == DeploymentStatus.PackageInstallRequired)
Copy link
Member

Choose a reason for hiding this comment

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

What happens if an app developer upgrades to WinAppSDK 1.3 but forgets to update this part of their code? In other words, what happens if the app just calls Initialize for all non-OK deployment statuses instead of calling Repair sometimes?

I hope the answer is "Everything still works, though maybe less efficiently than if you had used Repair."

// for the Status.
}
else if (result.Status == DeploymentStatus.PackageRepairRequired ||
result.Status == DeploymentStatus.Unknown)
Copy link
Member

Choose a reason for hiding this comment

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

Suppose in WinAppSDK version 1.9 we add a new DeploymentStatus. This code will ignore all new DeploymentStatus error codes. Should it treat all unknown DeploymentStatus values the same as Unknown?

As written, it means that adding a new DeploymentStatus is a breaking change.

again.
Once the Main and Singleton packages have been deployed, they generally do not need to be deployed
again. If a user were to remove the Main or Singleton package, the API can be used to reinstall them
again. If there is anything wrong with installed Main and Singleton packages, use Repair API to try
Copy link
Member

Choose a reason for hiding this comment

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

What does "anything wrong" mean? Does it mean "Any DeploymentStatus other than DeploymentStatus.Ok"?

Because that's not what the sample code does. It calls Repair only for selected DeploymentStatus values, not for "anything wrong".


```C#
if (DeploymentManager.GetStatus().Status != DeploymentStatus.Ok)
if (DeploymentManager.GetStatus().Status == DeploymentStatus.PackageInstallRequired)
Copy link
Member

Choose a reason for hiding this comment

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

This does not do what the previous text recommends. Previous text says, "If there is anything wrong with installed Main and Singleton packages, use Repair API to try and repair them." But in this code, we don't call Repair for "anything wrong". In fact, we're not calling Repair at all!

@@ -167,12 +183,13 @@ they shut down, to refer to the updated framework package.
// These should be run on a separate thread so as not to hang your app while the
// packages deploy.
var initializeTask = Task.Run(() => DeploymentManager.Initialize(deploymentInitializeOptions));
//...do other work while the repair is running...
Copy link
Member

Choose a reason for hiding this comment

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

The comment says "repair", but the method we called was "Initialize".

Either the code should be "Repair", to match the comment; or the comment should say "initialize" to match the code.

if (DeploymentManager.GetStatus().Status == DeploymentStatus.PackageRepairRequired ||
DeploymentManager.GetStatus().Status == DeploymentStatus.Unknown)
{
// Repair will always attempt to repair WindowsAppRuntime regardless of it's state.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Repair will always attempt to repair WindowsAppRuntime regardless of it's state.
// Repair will always attempt to repair WindowsAppRuntime regardless of its state.


```C#
if (DeploymentManager.GetStatus().Status == DeploymentStatus.PackageRepairRequired ||
DeploymentManager.GetStatus().Status == DeploymentStatus.Unknown)
Copy link
Member

Choose a reason for hiding this comment

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

Is it right to call GetStatus() twice? Could the status change between the two calls?

When the API is updating Framework package and ForceDeployment option is set, then all dependent
packages that are NOT currently in use will be immediately re-installed to refer to the updated
Framework package and all dependent packages that are currently in use will be re-installed, after
they shut down, to refer to the updated Framework package.
Copy link
Member

Choose a reason for hiding this comment

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

Wait, I thought the first paragraph in this section said that if you force deployment, then we don't want for the users to exit the apps; we shut down the apps immediately. But here, we wait for the apps to shut down naturally.

Or is "after they shut down" saying "after we shut them down" (rather than the app shutting down on its own)? Could use some clarity about whether the shutdown is immediate or eventual.

var deploymentRepairOptions = new DeploymentRepairOptions();
deploymentRepairOptions.ForceDeployment = true;

// Repair will always attempt to repair WindowsAppRuntime regardless of it's state.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Repair will always attempt to repair WindowsAppRuntime regardless of it's state.
// Repair will always attempt to repair WindowsAppRuntime regardless of its state.

DeploymentManager::Repair(...)
{
// Only needed supported in packaged processes
IF IsPackagedProcess()
Copy link
Member

Choose a reason for hiding this comment

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

Test seems backwards.

@sachintaMSFT sachintaMSFT removed the request for review from cwruss January 26, 2023 14:00
@sachintaMSFT sachintaMSFT changed the base branch from main to develop January 26, 2023 14:00
@bpulliam bpulliam added the api-design Updates to Project Reunion API surfaces label Mar 27, 2023
@mominshaikhdevs
Copy link

@sachintaMSFT Is this stale or what's the latest update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-design Updates to Project Reunion API surfaces area-Deployment Issues related to packaging, installation, runtime (e.g., SelfContained, Unpackaged) feature proposal status-no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants