App-configurable compatibility options spec: CompatibilityOptions

[codendone]

Spec for app-configurable compatibility options, which can be set via CompatibilityOptions APIs or via project properties.

[DrusTheAxe]

RuntimeCompatibilityOptions?

AppConfigurableCompatibility?

(Naming, hard :-()

[Scottj1s]

Agreed - even though it's in a namespace, I'd prefer a more specific name.

[codendone]

RuntimeCompatibilityOptions sounds good to me. I'll wait for other feedback from API review. (If we go with that, then CompatibilityChange would similarly change to RuntimeCompatibilityChange.)

[DrusTheAxe]

'App-configurable compatibility' - seems to be the catchy term for this feature. API naming should match?

[codendone]

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 CompatibilityOptions was a better name for the API called by apps.

[DrusTheAxe]

Is HypotheticalChange an actual thing or it's representative of an actual value? If the latter please use a more real example #Resolved

[codendone]

There's nothing real I can use instead here. I could put something like TextBoxDestructionCrash or RegisterPackageRequiresUri, to name two fixes which were in 1.6.3, but then we're likely to debate those names here.

[Scottj1s]

Agreed that a generic symbol is appropriate, but 'hypothetical' raises my eyebrows - maybe SampleChange?

[DrusTheAxe]

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 CompatibilityChange.BlockTopSpin or...?

[DrusTheAxe]

initializing the Windows App Runtime

What does this mean? When does this occur? There is no WindowsAppRuntimeInitialize() function (intentionally)

Do you mean the bootstrapper APIs? Other APIs?

[codendone]

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.

[Scottj1s]

I think "early in the process" is sufficient, given that an error would occur otherwise (the developer will learn how early is enough)

[DrusTheAxe]

Is that a way to 'Reset` or 'Unapply' the configuration? OR no, 'till process death do you part?

[codendone]

There is no way to reset or unapply (unless WinAppSDK somehow gets entirely unloaded from the process). The intent is indeed 'til process death.

[DrusTheAxe]

Missing API contract

SUGGESTION:

namespace Microsoft.Windows.ApplicationModel.WindowsAppRuntime
{
    [contractversion(1)]
    apicontract AppConfigurableCompatibilityContract{};

    /// The set of servicing changes that can be disabled.
    [contract(AppConfigurableCompatibilityContract, 1)]
    enum CompatibilityChange
...

[codendone]

Is that typically in the API spec? The actual code has that, which I think should be sufficient.

[DrusTheAxe]

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...

[codendone]

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.

[DrusTheAxe]

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...?

[contract(AppConfigurableCompatibilityContract, 1)]
runtimeclass CurrentCompatibilityOptions
{
    static CurrentCompatibilityOptions GetDefault();

    WindowsAppRuntimeVersion EffectiveVersion { get; }

    IVector<CompatibilityChange> DisabledChanges{ get; };
}

[codendone]

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.

[DrusTheAxe]

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.

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 IsSupported API e.g.

if (Microsoft.Stuff.Thingamajig.IsSupported("foo"))
{
    ...ComapatibilityChange.Foo is enabled...
}
else
{
    ...ComapatibilityChange.Foo is disabled or doesn't exist yet (e.g. checking in 1.7.2 when Foo was introduced in 1.7.3)...
}

If the feature doesn't already have an IsFooSupported() or IsSupported(parameter_foo) API they'll be required to add one in a patch servicing release, or if an IsSupported(parm) exists they'll need to invent a new parameter in a patch release ("easy" if parm=string, less so if enum).

This is a broad tax. It's not isolated to one feature, team or release.

A query API simplifies this problem.

[codendone]

We can add that if/when there is a need.

[Scottj1s]

Suggested change

	<WindowsAppSDKRuntimePatchMode>1.7.3</WindowsAppSDKRuntimePatchMode>
	<WindowsAppSDKRuntimePatchMode1>1.7.3</WindowsAppSDKRuntimePatchMode1>

[Scottj1s]

numeric suffixes should be consistent between xml and api

[Scottj1s]

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?

[codendone]

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.

[Scottj1s]

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?

[codendone]

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.)

[Scottj1s]

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.

[DrusTheAxe]

Why would WinUI matter here? APIs shouldn't be calling Apply() only the app (the app/dev owns process context decisions).

E_ILLEGAL_STATE_CHANGE for 2nd+ Apply() call that would produce different outcomes (and S_OK if no different) seems righteous . Most apps will only call Apply once (and more often, nonce) but if devs make multiple calls the error seems appropriate

[Scottj1s]

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.

[codendone]

WinUI doesn't support it yet, but why should we make an app's attempt to ensure it is applied be an error?

[codendone]

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 Apply API using the mechanism described here. Allowing apps to simply Apply without a check is easier and safer for apps.

[DrusTheAxe]

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 PatchMode1 and PatchMode2 with IVector<WindowsAppRuntimeVersion> PatchModes{ get; }; Then the object supports an extensible list of versions, much like it supports an extensible list of DisabledChanges.

For example, the sample on line 55 changes from

void ApplyCompatibilityOptions()
{
    var compatibilityOptions = new CompatibilityOptions();
    compatibilityOptions.PatchMode1 = new WindowsAppRuntimeVersion(1,7,3);
    compatibilityOptions.PatchMode2 = new WindowsAppRuntimeVersion(1,8,2);
    compatibilityOptions.Apply();
}

to

void ApplyCompatibilityOptions()
{
    var compatibilityOptions = new CompatibilityOptions();
    compatibilityOptions.Add(new WindowsAppRuntimeVersion(1,7,3));
    compatibilityOptions.Add(new WindowsAppRuntimeVersion(1,8,2));
    compatibilityOptions.Apply();
}

[Scottj1s]

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).

[DrusTheAxe]

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?

[Scottj1s]

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.

[DrusTheAxe]

Hard limit - sure. My point is, if "max 2 versions" needs >2 in the future the API shape would need impactful changes. Plus PatchMode1 and PatchMode2 property names are kinda weird. If instead it's a list it wouldn't pose future API shape issues (and not be 'weird'), and there's no significant difference to the developer experience.

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

[Scottj1s]

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).

[codendone]

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.

[DrusTheAxe]

if the options differ from the locked configuration

The options, or the effective options?

If an app winds up calling

var co = new CompatibilityOptions();
co.PatchMode1 = new WindowsAppRuntimeVersion(1,7,3);
co.PatchMode2 = new WindowsAppRuntimeVersion(1,8,2);
co.Apply();
...
var co2 = new CompatibilityOptions();
co2.PatchMode1 = new WindowsAppRuntimeVersion(1,7,3);
co2.Apply();

and it's running with 1.7 the options to Apply differ but the effective options are identical.

Is the check for identical values for all properties of the object, or identical outcomes of the runtime decision?

[Scottj1s]

yeah, seems like all kinds of interesting edge cases are possible. I vote for a strict single Apply call pattern.

[DrusTheAxe]

strict single Apply call pattern

As in, any 2nd+ Apply() errors (regardless the 2nd+ values)?

Seems reasonable. You've got my axe :-)

[codendone]

The second Apply will fail if it doesn't exactly match the first one. WinUI has long wanted to support an app being able to uninitialize and reinitialize, and this ability to call Apply again with the identical values enables that scenario. I don't see a need to prevent the scenario and make initialization potentially being more difficult for apps which manually initialize WinAppSDK.

[codendone]

Oops, I forgot to submit my comments from last week.

[codendone]

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.

[codendone]

Correct, see the paragraph at line 84.

[codendone]

There will be a compile error if an unknown enum name is used.

[codendone]

See previous comment.

[codendone]

That is the intent, yes. We still need users to report issues to help us understand why a change needed to be disabled.

[codendone]

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 DisabledChanges are temporary.)

[codendone]

I think whitespace-only would be less easy to read. Think of the contents of this property like specifying the contents of an array.

[DrusTheAxe]

LGTM