microsoft / WindowsAppSDK

The Windows App SDK empowers all Windows desktop apps with modern Windows UI, APIs, and platform features, including back-compat support, shipped via NuGet.
https://docs.microsoft.com/windows/apps/windows-app-sdk/
MIT License
3.85k stars 323 forks source link

v1.6-preview breaking changes: #4645

Closed dotMorten closed 2 months ago

dotMorten commented 3 months ago

Describe the bug

I noticed a few breaking changes in v1.6-p1: image image

I get changing the parameter names to use proper casing or better convey some concept, but in this case the changes here are so subtle and non-consequential that I don't think it's worth breaking people's code.

Example of code that no longer compiles in 1.6:

`manager.IsPackageResgistrationPending(userSecurityId: "id", packageFamilyName: "packageName") // won't compile`

NuGet package version

Windows App SDK 1.6 Preview 1: 1.6.240807006-preview1

DarranRowe commented 3 months ago

For the change between packageFamilyName to packageFullName, there is a pretty big and functional difference between a package's family name and full name. To give an example, using the Windows App Runtime itself. Package family name: Microsoft.WindowsAppRuntime.1.5_8wekyb3d8bbwe. Package full name: Microsoft.WindowsAppRuntime.1.5_5001.178.1908.0_x64__8wekyb3d8bbwe For the Windows App Runtime, the package full name identifies an exact package, where the family name identifies a set of packages that can differ by platform and version.

dotMorten commented 3 months ago

@DarranRowe So are you saying this method is not only changing parameter name, but also changing what parameter we're meant to send in? If so that's even worse.

DarranRowe commented 3 months ago

@dotMorten If IsPackageRegistrationPending was expecting a package family name, then it was outright wrong. A package family name works if the package is an application, since Windows will only allow you to install one version. But for framework packages, like the Windows App Runtime, WinUI 2 and more, then you can have multiple versions installed. As a worst case scenario of an ARM64 version of Windows 11 22H2, there can be 4 versions of a framework package installed. So if you call IsPackageRegistrationPending with a family name, and there are 3 packages with that family name, which status is the correct result? Even with the x64 version of Windows 11, it isn't unreasonable to assume that there can be two instances of a framework package installed due to WoW64. I agree that it is questionable that this mistake got as far as the experimental version. But it is also a good thing that they caught this with the preview version.

dotMorten commented 3 months ago

I agree that it is questionable that this mistake got as far as the experimental version

I'm not following. This is Preview (not experimental), and is a comparison to v1.5 where it was packageFamilyName. Hence the breaking change.

DarranRowe commented 3 months ago

My mistake, I completely forgot that this was in 1.5 and thought that this was a comparison to 1.6 experimental. But yes, it is worse.

hez2010 commented 3 months ago

In .NET we don't consider changing the name for a parameter as source breaking change (while actually it is, but named parameters are too subtle to account for). Also this change happens in the winmd so that C++ users won't be affected at all. I can totally accept such change for better consistency and understanding as it's not an ABI breaking change so that existing apps will continue to work. Generally we always have some breaking changes in a WASDK update, I don't see why this cannot be part of them here.

DarranRowe commented 3 months ago

@hez2010 I think the biggest issue here is the implication that the meaning of the IsPackageRegistrationPending parameter changed. If, in 1.5, it took a package family name, then code would fail in 1.6 if it provided a package family name. The package full name includes version and platform information. The package family name doesn't include this.

dotMorten commented 3 months ago

@hez2010 That's not true. It's a breaking change from the .NET Runtime team's perspective: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-changes.md#bucket-1-public-contract

Generally we always have some breaking changes in a WASDK update

I sure hope not. When did this happen last?

I could maybe, and that's a BIG maybe, see the parameter name being changed in a major release, but not in a minor one.

DarranRowe commented 3 months ago

@hez2010

Generally we always have some breaking changes in a WASDK update, I don't see why this cannot be part of them here.

There is also a doubly problematic issue here.

If you look into the documentation for COM interfaces, it states:

COM interfaces are immutable. You cannot define a new version of an old interface and give it the same identifier. Adding or removing methods of an interface or changing semantics creates a new interface, not a new version of an old interface. Therefore, a new interface cannot conflict with an old interface. However, objects can support multiple interfaces simultaneously and can expose interfaces that are successive revisions of an interface, with different identifiers. Thus, each interface is a separate contract, and systemwide objects need not be concerned about whether the version of the interface they are calling is the one they expect. The interface ID (IID) defines the interface contract explicitly and uniquely.

I emphasised the important points here. This is relevant because under the surface, these are WinRT components which are, in turn, COM components. Both 1.5 and 1.6 preview 1 use the same IID for the main interface:

[contract(Microsoft.Windows.Management.Deployment.PackageDeploymentContract, 1.0)]
[exclusiveto(Microsoft.Windows.Management.Deployment.PackageDeploymentManager)]
[uuid(F41717D8-5AB2-57AC-83CD-D0C48CC784CD)]
interface IPackageDeploymentManager : IInspectable
{

You can see this in the metadata if you use ildisasm, but I used winmdidl to generate idl files from the .winmd files. But the important thing here is, if the parameter semantics changed along with the parameter name, then this is a pretty bad violation of the interface rules.

As an interesting side note, Win2D did this too.

codendone commented 3 months ago

It looks like potential parameter behavior change for PackageDeploymentManager::IsPackageRegistrationPending() and PackageDeploymentManager::IsPackageRegistrationPendingForUser() is not very interesting, given the implementation of these functions in 1.5:

    bool PackageDeploymentManager::IsPackageRegistrationPending(hstring const& packageFamilyName)
    {
        return IsPackageRegistrationPendingForUser(hstring{}, packageFamilyName);
    }
    bool PackageDeploymentManager::IsPackageRegistrationPendingForUser(hstring const& userSecurityId, hstring const& packageFamilyName)
    {
        //TODO Awaiting FrameworkUdk update with PackageManagement_IsPackageRegistrationPending()
        throw hresult_not_implemented();
    }

In short, these always threw a not-implemented exception in 1.5.

@DrusTheAxe for any further comments on these functions (added as part of #4453).

Internal bugs for the two API breaking changes: ContentIsland.Create and PackageDeploymentManager.IsPackageRegistrationPending*

jarno9981 commented 3 months ago

same for the CommunityToolkit.WinUI.Controls.SettingsControls all ui controls are broken in app sdk 1.6 preview 1 only experimantel works

dotMorten commented 2 months ago

Confirmed fixed: https://omds.xaml.dev/WinAppSDK_v1.5_v1.6-p2.html