-
Notifications
You must be signed in to change notification settings - Fork 335
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
App-configurable compatibility options spec: CompatibilityOptions #4966
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,226 @@ | ||||||
App-configurable compatibility options | ||||||
=== | ||||||
|
||||||
# Background | ||||||
|
||||||
_This spec adds a CompatibilityOptions class to control behavior of servicing changes._ | ||||||
|
||||||
Apps using Windows App SDK have two choices today: use Windows App SDK as an | ||||||
automatically-updating framework package or use Windows App SDK in self-contained mode with no | ||||||
automatic updates. Apps which use the framework package automatically get fixes, which is great, | ||||||
codendone marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
but they have a chance of being broken by a bug or subtle behavior change in a servicing update. | ||||||
Some apps choose self-contained mode to avoid that risk of an automatic servicing update breaking | ||||||
them. But even when apps choose self-contained mode, they may find that they can't update to a | ||||||
newer servicing release due to a bug or behavior change. Also, self-contained apps can't use | ||||||
features which require shared services, such as some Notification features delivered via the | ||||||
Singleton package. | ||||||
|
||||||
App-configurable compatibility is intended to prevent these problems, enabling apps to choose to | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 'App-configurable compatibility' - seems to be the catchy term for this feature. API naming should match? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think 'App-configurable compatibility' is useful way for us to succinctly understand the functionality of this new feature, but I thought some form of |
||||||
use the framework package with confidence they won't be broken. Also, apps which find a | ||||||
compatibility problem in a servicing release will still be able to move forward by temporarily | ||||||
disabling problematic changes. Even apps using self-contained can update to a new package with | ||||||
confidence that app-configurable compatibility will ensure a successful update. | ||||||
|
||||||
A new `CompatibilityOptions` class has APIs to control the behavior of servicing changes. | ||||||
There are also properties which can be set in the app's project file to automatically use the new | ||||||
APIs with the specified values. | ||||||
|
||||||
# API Pages | ||||||
|
||||||
## CompatibilityOptions class | ||||||
|
||||||
Configures which changes in Windows App SDK servicing releases are enabled. By default, all | ||||||
changes are enabled, but `CompatibilityOptions` can be used to lock the runtime behavior to a | ||||||
specified patch version or to disable specific changes: | ||||||
|
||||||
1. **Choose the patch version:** You can specify which servicing patch version's behavior you | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
API and XML refer to "patch mode". Different phrasing for same thing or there's a meaningful difference in the lingo? If same then please use consistent terminology. If not then please define "patch version" and "patch mode" so it's clear how they differ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my mind, this is consistent. The app developer chooses which WinAppSDK patch version they want to snap to, and then they set that as the patch mode to run in. Whether There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree that a unified term would be clearer. To me, "mode" doesn't quite capture it - I think of a mode as orthogonal to a given release/version/patch (like self-contained mode). How about PatchLevel? |
||||||
want to use. For example, your app can specify that it wants the 1.7.2 version behavior, which | ||||||
will have Windows App SDK run in that mode even if 1.7.3 or later is installed. This capability | ||||||
allows you to control when your app gets new fixes or behavior changes, even when not using | ||||||
self-contained mode. | ||||||
2. **Temporarily disable specific changes:** If your app encounters a problem with a specific | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
These changes are the developer's request or hint that, as you note, may not be disableable in time. But isn't that the same for patch version/mode? If I specify patch version = 1.7.2 and eventually update to use 1.8 isn't my previous 1.7 request irrelevant? The intent is "disable" means "disable...the things allowed to be disabled", which could change on each build (formerly disableable now aren't)? Hence the 'temporary'? Or no, once a 'disable' knob is present in a major.minor it will be supported disable-able forever in that major.minor? SUGGESTION: Remove 'Temporarily' There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The difference here is intentional. PatchMode doesn't necessarily mean there is a change the app wants to disable, it could just be the maximum version the app has tested against and they don't want to test against higher. The DisabledChanges list is used for identified compatibility issues an app hits. These should either be reported to the WinAppSDK team so it can be fixed if it is a bug, or should be fixed in the app, if appropriate, since otherwise this will be a known compatibility issue when trying to move to the next significant release of WinAppSDK (such as from 1.7.2 to 1.8.x). Because of all this, the DisabledChanges list should be considered temporary, one way or the other. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the term "temporarily" as it conveys the temporary nature of the disable-ability (per major.minor). But I do think it's a bit confusing that while this status only applies to a specific major.minor version, it's application here is 'global' (not scoped to a specific patch version). That could be a sharp edge - maybe we should scope change disabling to a patch version. We could then enforce applicability and potentially generate errors (which we can't do for the global scoped change disabling). |
||||||
change in a servicing update, you can disable just that change while still benefiting from the | ||||||
other changes or features in that update. (All changes are enabled by default for the patch | ||||||
version in use.) Disabling a change is a temporary measure which gives time for a fix to be | ||||||
released in a future Windows App SDK update or for you to implement an update in your app. | ||||||
|
||||||
Here's an example of using `CompatibilityOptions` to specify a patch version and disable a | ||||||
specific change: | ||||||
|
||||||
```c# | ||||||
void ApplyCompatibilityOptions() | ||||||
{ | ||||||
var compatibilityOptions = new CompatibilityOptions(); | ||||||
compatibilityOptions.PatchMode1 = new WindowsAppRuntimeVersion(1,7,3); | ||||||
codendone marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
compatibilityOptions.PatchMode2 = new WindowsAppRuntimeVersion(1,8,2); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if I'm running with 1.7.3 and only specified 1.8.anything? That's equivalent to specifying nothing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, see the paragraph at line 84. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you would omit PatchMode2 altogether if you were happy with 1.8.anything |
||||||
compatibilityOptions.DisabledChanges.Add(CompatibilityChange.HypotheticalChange); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's nothing real I can use instead here. I could put something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed that a generic symbol is appropriate, but 'hypothetical' raises my eyebrows - maybe SampleChange? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fabrikam is a fake company amd Contosso is a fake product. Do we have additional fake terms we could use? E.g. if 'top spin' is a fake feature then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if I specify an unknown change? Silently ignored? Error? Debug message? Other? CONSIDER: Option(s) for developers to be aware of unknown inputs e.g.
or
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There will be a compile error if an unknown enum name is used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where scoping disabled changes to a patch level would make sense - error detection and reporting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the enum and a compile error if trying to use something not in the enum is sufficient. We also would not want to provide a runtime warning if DisabledChanges specified for 1.7 are still passed when the app is testing running against 1.8. |
||||||
compatibilityOptions.Apply(); | ||||||
} | ||||||
``` | ||||||
|
||||||
CompatibilityOptions must be applied early in the process before any other Windows App | ||||||
SDK APIs are called, or right after initializing the Windows App Runtime. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What does this mean? When does this occur? There is no Do you mean the bootstrapper APIs? Other APIs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The bootstrapper or other initialization code, whether from WinAppSDK auto-initializers for from explicit app calls, needs to happen first. This should happen right after that. I didn't have a better way to describe that timing that what I wrote here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think "early in the process" is sufficient, given that an error would occur otherwise (the developer will learn how early is enough) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Does that include before any WinAppSDK 'auto-initializer' code is executed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See previous comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Michael unified all the auto-initializer code to ensure that compatibility options are correctly sequenced. |
||||||
|
||||||
`PatchMode1` and `PatchMode2` are simply two fields to set relevant patch modes. These needn't | ||||||
match any specific version of the Windows App Runtime nor be in a specific order, so it is valid | ||||||
to set `PatchMode1` to 1.8.2 and `PatchMode2` to 1.7.3, for example. And, in the above example, | ||||||
when updating the app to 1.9, you may choose to simply update `PatchMode1` to 1.9.3 and leave | ||||||
`PatchMode2` as 1.8.2. | ||||||
|
||||||
### Specifying CompatibilityOptions in the app's project file | ||||||
|
||||||
You can also specify the patch version and disabled changes in the app's project file instead of | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "patch version" but XML and API (above) say "patch mode" Please use consistent terminology There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comment from line 36. |
||||||
directly calling the CompatibilityOptions APIs. This approach has the advantage of ensuring the | ||||||
options are applied early at the proper timing. Here's an example of how to specify the patch | ||||||
version and disabled changes in the project file (such as `.csproj` or `.vcxproj`): | ||||||
|
||||||
```xml | ||||||
<PropertyGroup> | ||||||
<WindowsAppSDKRuntimePatchMode>1.7.3</WindowsAppSDKRuntimePatchMode> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. numeric suffixes should be consistent between xml and api |
||||||
<WindowsAppSDKRuntimePatchMode2>1.8.2</WindowsAppSDKRuntimePatchMode2> | ||||||
codendone marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
<WindowsAppSDKDisabledChanges>HypotheticalChange, OtherHypotheticalChange</WindowsAppSDKDisabledChanges> | ||||||
</PropertyGroup> | ||||||
``` | ||||||
|
||||||
The `WindowsAppSDKDisabledChanges` property is a comma-separated list of `CompatibilityChange` | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why not multiple elements? e.g.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is an arbitrary choice either way, and I prefer comma-separated here to keep this succinct, especially since the goal is this is very rarely used and should be even rarer to have multiple changes disabled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure the flexible syntax warrants the parsing complexity. presumably, most users would only ever apply a handful of changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comma-separated? So semi-colons or other characters are errors?
Whitespace is signficant? Are any of these errors or all equivalent?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, other characters will produce a compile error. Whitespace is ignored. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or even whitespace-only-delimited? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think whitespace-only would be less easy to read. Think of the contents of this property like specifying the contents of an array. |
||||||
values to disable. | ||||||
|
||||||
### Behavior with no PatchMode specified | ||||||
|
||||||
If no `PatchMode1` or `PatchMode2` is specified, or if neither value matches the major.minor | ||||||
version of the runtime being used, then the runtime will use the latest patch version. In other | ||||||
words, the runtime will run with all servicing changes enabled (just like how Windows App SDK | ||||||
worked before this API existed). | ||||||
|
||||||
## CompatibilityOptions.PatchMode1 | ||||||
|
||||||
Optional patch mode to use if the runtime version matches the major.minor version. If the runtime | ||||||
version does not match the specified major.minor version, this value is ignored. | ||||||
|
||||||
Instead of directly using this API, this patch mode could be specified in the app's project file, | ||||||
like this: | ||||||
|
||||||
```xml | ||||||
<PropertyGroup> | ||||||
<WindowsAppSDKRuntimePatchMode>1.7.3</WindowsAppSDKRuntimePatchMode> | ||||||
</PropertyGroup> | ||||||
``` | ||||||
|
||||||
If your app is not in the process of transitioning to a new version of the Windows App SDK, you | ||||||
should only set this one patch mode. | ||||||
|
||||||
## CompatibilityOptions.PatchMode2 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
or in code via similar e.g. in *.vcxproj
and in code
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having a second patch mode simpler for an app to simply hard-code the two relevant values they are using. Also, two patch modes need to be specified if an app is doing version selection at runtime (such as having most users still running on 1.7 but some set of users testing the same app running against 1.8). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I think Michael had in mind interactions with A/B Forwarding here, where the runtime version is selected dynamically (not tied to the SDK version built against). So in that case, explicit properties are necessary. |
||||||
|
||||||
Optional patch mode to use if the runtime version matches the major.minor version. If the runtime | ||||||
version does not match the specified major.minor version, this value is ignored. | ||||||
|
||||||
This property enables setting a second patch mode to help apps transitioning to a new version of | ||||||
the Windows App SDK. This is a convenience to allow the patch modes for both the old and new | ||||||
version to be specified during the transition. Apps not in the process of transitioning should | ||||||
only set the one patch mode they want to use. | ||||||
|
||||||
Setting both patch modes for the same major.minor version, such as 1.7.3 and 1.7.4, is not allowed | ||||||
and will generate an error during `Apply()`. | ||||||
|
||||||
Instead of directly using this API, this patch mode could be specified in the app's project file, | ||||||
like this: | ||||||
|
||||||
```xml | ||||||
<PropertyGroup> | ||||||
<WindowsAppSDKRuntimePatchMode2>1.8.2</WindowsAppSDKRuntimePatchMode2> | ||||||
</PropertyGroup> | ||||||
``` | ||||||
|
||||||
## CompatibilityOptions.DisabledChanges | ||||||
|
||||||
Optional list of specific servicing changes to disable. | ||||||
|
||||||
If you encounter a problem with a specific change in a servicing update, you can disable just that | ||||||
change by adding it to the `DisabledChanges` list before calling `Apply`, or by specifying it in | ||||||
your app's project file: | ||||||
```xml | ||||||
<PropertyGroup> | ||||||
<WindowsAppSDKDisabledChanges>HypotheticalChange, OtherHypotheticalChange</WindowsAppSDKDisabledChanges> | ||||||
</PropertyGroup> | ||||||
``` | ||||||
|
||||||
The [release notes](https://learn.microsoft.com/windows/apps/windows-app-sdk/stable-channel) for | ||||||
the Windows App SDK will list the name of each change that can be disabled. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can a list be provided in the SDK for build-time reference? e.g. Add
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The list will be provided via the CompatibilityChange enum. Developers could look in the WindowsAppSDK repo for the release to inspect it. We may just not have much in the way of description for each enum value there, so I think the release notes should include the enum value name to provide a connection. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This too would be difficult to support with A/B Forward, where two separate SDKs (hence, change lists) could apply |
||||||
|
||||||
Note that disabling a change is a temporary measure which gives time for a fix to be released in a | ||||||
future Windows App SDK update or for you to implement an update in your app. You should report | ||||||
changes you disable if you believe it to be a bug in the Windows App SDK. This will help the | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are disabled changes reported via insights? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is the intent, yes. We still need users to report issues to help us understand why a change needed to be disabled. |
||||||
Windows App SDK team to understand the impact of the change and prioritize a fix. You can report | ||||||
issues on the [WindowsAppSDK GitHub repo](https://github.com/microsoft/WindowsAppSDK/issues/new?template=bug-report.yaml). | ||||||
|
||||||
The capability to disable a change is **not** available across new Windows App SDK stable | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have automation in place (or planned), both to enforce containment of fixes, and to reset the containment "counter" for a new major.minor release? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CR and shiproom approval is the only verification we (already) have today. I think that should be sufficient. There is no need to reset for each new major.minor release: the containment is never in the main branch and therefore never carries over to the next release. |
||||||
releases (such as 1.7.x to 1.8.x), so this does not allow you to permanently disable an | ||||||
intentional behavior change. Also, new stable releases, such as 1.8.0, start with no changes | ||||||
available to disable -- this capability only applies to disabling targeted servicing changes | ||||||
added in servicing releases. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implies Then shouldn't these be named This gets more to policy how we name these (Hypothetical)Changes but strong guidance from the start is best. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to prefix the version, since if the app compiles against 1.8 then none of the 1.7 enum values will exist. (This gets back to the comment on why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, change disabling should be patch-specific |
||||||
|
||||||
## CompatibilityOptions.Apply | ||||||
|
||||||
Apply the set compatibility options to the runtime. | ||||||
|
||||||
CompatibilityOptions must be applied early in the process before any other Windows App | ||||||
SDK APIs are called, or right after initializing the Windows App Runtime. The options must be set | ||||||
early before the configuration is locked for the lifetime of the process. Since the Windows App | ||||||
Runtime needs to run with a consistent configuration, it will lock the configuration when it needs | ||||||
to ensure the configuration will no longer change. Calling `Apply` will also lock the | ||||||
configuration. | ||||||
|
||||||
If the configuration has already been locked, calling `Apply` will throw an `E_ILLEGAL_STATE_CHANGE` | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that a way to 'Reset` or 'Unapply' the configuration? OR no, 'till process death do you part? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no way to reset or unapply (unless WinAppSDK somehow gets entirely unloaded from the process). The intent is indeed 'til process death. |
||||||
exception if the options differ from the locked configuration. It is okay to call `Apply` multiple | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The options, or the effective options? If an app winds up calling
and it's running with 1.7 the options to Is the check for identical values for all properties of the object, or identical outcomes of the runtime decision? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, seems like all kinds of interesting edge cases are possible. I vote for a strict single Apply call pattern. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As in, any 2nd+ Seems reasonable. You've got my axe :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The second |
||||||
times with the same configuration, such as if the process had initialized and used the Windows | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a real scenario? Is WinUI finally robust enough to support it? If not, I'd just omit this sentence and allow that multiple calls are idempotent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would WinUI matter here? APIs shouldn't be calling
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WinUI has not supported reinitialization well (TLS cleanup issues), so better not to suggest this. I have no problem with any subsequent call (whether identical or not) producing an E_ILLEGAL_STATE_CHANGE. The app ought to be able to manage that requirement and it frees us from complex comparisons. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WinUI doesn't support it yet, but why should we make an app's attempt to ensure it is applied be an error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One other scenario we've had with apps using XAML Islands is sometimes they are initializing multiple thread simultaneously. We could require apps to add a lock to ensure they only initialize once, which some apps will forget to do, or we can handle this in the |
||||||
App Runtime earlier and is reinitializing for additional use. | ||||||
|
||||||
# API Details | ||||||
|
||||||
```c# (but really MIDL3) | ||||||
namespace Microsoft.Windows.ApplicationModel.WindowsAppRuntime | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing API contract SUGGESTION:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that typically in the API spec? The actual code has that, which I think should be sufficient. |
||||||
{ | ||||||
/// The set of servicing changes that can be disabled. | ||||||
enum CompatibilityChange | ||||||
{ | ||||||
None = 0, /// do not use this value | ||||||
|
||||||
// Add real changes here: | ||||||
|
||||||
// 1.7.1 | ||||||
// HypotheticalChange, | ||||||
// OtherHypotheticalChange, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do we see multiple changelists being applied across patch versions, given the app only sees one version of the SDK? i.e., where would the // 1.8.3 changes be discovered? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Across patch versions within a single major.minor release? Nothing special is required for that scenario. If you mean specifying DisabledChanges, as an example, for both the 1.7.x version and 1.8.x version the app might use, that is not possible with the proposed design. The CompatibilityChange enum will only be defined with the values for the major.minor version the app compiles against. This adds another reason DisabledChanges should be temporary. (Technically, a developer could copy the numeric value of the CompatibilityChange from the other major.minor SDK version and directly use that value. The intent is for that to not happen, or at least be so rare that the hack can be used if needed.) |
||||||
// ... | ||||||
}; | ||||||
|
||||||
/// Represents a version of the Windows App Runtime. | ||||||
struct WindowsAppRuntimeVersion | ||||||
{ | ||||||
UInt32 Major; | ||||||
UInt32 Minor; | ||||||
UInt32 Patch; | ||||||
}; | ||||||
|
||||||
/// This object is used by the app to configure any desired compatibility options | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the "app" or "process"? Can OOP WinRT servers use this API? Are OOP WinRT servers apps? SUGGESTION: This object is used by the process... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I consider the servers to be apps as well. In this place, I prefer saying "app" as something which I believe will resonate better with developers. |
||||||
/// for Windows App Runtime behavior of changes added in servicing updates. This | ||||||
/// object is only used to set the runtime behavior and can't be used to query the | ||||||
/// applied options. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why no query API? Let me guess: An app specifying this knows what it's doing so it doesn't have to query at runtime. Question: What are class libraries supposed to do? Lacking a query API creates issues for library authors. How does a library know current process' behavior? They don't control the process/environment they run in (not the app). Would that amount to...?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Class libraries don't get to choose. Yes, the app should know what it is doing, but shouldn't have any conditional code. The intent here is that no app or class library code conditions on whether the implementation of WinAppSDK code has a fix enabled or disabled. We could later add a query API if a good reason and need for one is found. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Nice in theory but as they say, the only difference between theory and practice is in theory there's no difference whereas in practice... Some behavioral changes can't be detected (w/o dipping into version checks). Lacking a query API leaves class library developers at a loss unable to know if they need to jump left or right. This could be solved by dictating all CompatibilityChanges MUST be detectable via an
If the feature doesn't already have an This is a broad tax. It's not isolated to one feature, team or release. A query API simplifies this problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can add that if/when there is a need. |
||||||
runtimeclass CompatibilityOptions | ||||||
{ | ||||||
CompatibilityOptions(); | ||||||
|
||||||
/// An optional patch mode to use if the runtime version matches the major.minor version. | ||||||
WindowsAppRuntimeVersion PatchMode1 { get; set; }; | ||||||
|
||||||
/// An optional patch mode to use if the runtime version matches the major.minor version. | ||||||
WindowsAppRuntimeVersion PatchMode2 { get; set; }; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The API is hardwired presuming an app will only work with 2 versions. That doesn't scale into the future if e.g. an app works with 1.7, 1.8 and 1.9. Or when WinAppSDK has LTSC releases for an app to work with e.g. LTSC 1.7, LTSC 1.10, current 1.11 and vNext 1.12. Any change to support >2 patch modes will weird API shape changes (PatchMode3, PatchMode4... or more break-y changes) RECOMMEND: Replace For example, the sample on line 55 changes from
to
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had suggested the list before in the code review. But it's not necessary to generalize beyond 2 - this is necessary to cover A/B Forward (otherwise one level would be sufficient). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You assume "run with product version X and test version Y". That an app will only need 2 versions, but apps can be designed to work with any of multiple versions (especially when using WinAppSDK/MSIX and not SelfContained). CompatibilityOptions is available to all WinAppSDK users, right? Not just if WindowsAppSdkSelfContained=true? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not an assumption - a hard limit. A/B Forward will only support building with major.minor and running on that and major.minor-1. CompatibilityOptions is designed primarily for FWP-dependent apps, not WindowsAppSdkSelfContained. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hard limit - sure. My point is, if "max 2 versions" needs >2 in the future the API shape would need impactful changes. Plus P.S. "designed primarily for" == SHOULD, not MUST. That makes it less the law than the Jack Sparrow Rule (There's what a man can do, and what a man can't do...). But as John Wick taught us, a pencil can be used for more than designed primarily for... :-) I didn't see any overt block disallowing or disabling this API if WinAppSDKSelfContained=true. Which is a good thing. Even if we expect more WinAppSDK/MSIX apps using this than WinAppSDK/SelfContained I can see value for the latter, albeit possibly less often. For instance, apps wanting some perf or functional improvements in, say, 1.42.87 but the updated Fubar feature griefs them, so use 1.42.87 but disable CompatibleOption.Fubar until MS gets a fix out in 1.42.88(+). Works equally well for SelfContained as MSIX. Flexibility good. Flexibility without downside even better :P There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It certainly feels like a better API design to use a list and enforce an element count of [0..2] (throwing an exception on element 3). I can get behind that. I do not understand your movie analogies. But that's ok. I say designed primarily for FWP-dependent, but not exclusively (per Michael's intro text). Yes, WindowsAppSdkSelfContained could certainly also benefit - I assume for punching holes via disabled changes, without using the patch level feature (irrelevant). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I started with a list for this property and I didn't like how it looked. It adds unnecessary complexity to the project properties and the runtime code. As Scott said, our A/B design is specifically between two versions. |
||||||
|
||||||
/// An optional list of specific servicing changes to disable. | ||||||
IVector<CompatibilityChange> DisabledChanges{ get; }; | ||||||
|
||||||
/// Apply the compatibility options to the runtime. | ||||||
void Apply(); | ||||||
} | ||||||
} | ||||||
``` | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RuntimeCompatibilityOptions
?AppConfigurableCompatibility
?(Naming, hard :-()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - even though it's in a namespace, I'd prefer a more specific name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RuntimeCompatibilityOptions
sounds good to me. I'll wait for other feedback from API review. (If we go with that, thenCompatibilityChange
would similarly change toRuntimeCompatibilityChange
.)