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

v1.6-preview breaking changes: #4645

Closed
dotMorten opened this issue Aug 14, 2024 · 12 comments
Closed

v1.6-preview breaking changes: #4645

dotMorten opened this issue Aug 14, 2024 · 12 comments
Labels
area-Composition area-Deployment Issues related to packaging, installation, runtime (e.g., SelfContained, Unpackaged) breaking change fix-released The fix has been in a release (experimental, preview, stable, or servicing). Status: Fixed
Milestone

Comments

@dotMorten
Copy link
Contributor

dotMorten commented Aug 14, 2024

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
Copy link

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
Copy link
Contributor Author

@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
Copy link

@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
Copy link
Contributor Author

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
Copy link

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.

@codendone codendone added this to the 1.6 milestone Aug 15, 2024
@codendone codendone added area-Deployment Issues related to packaging, installation, runtime (e.g., SelfContained, Unpackaged) area-Composition breaking change and removed needs-triage labels Aug 15, 2024
@hez2010
Copy link

hez2010 commented Aug 15, 2024

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
Copy link

DarranRowe commented Aug 15, 2024

@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
Copy link
Contributor Author

dotMorten commented Aug 15, 2024

@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
Copy link

DarranRowe commented Aug 16, 2024

@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
Copy link
Contributor

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
Copy link

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

@codendone codendone added the Status: Fixed internally The issue has been fixed but has not yet shipped in a release. label Aug 22, 2024
@dotMorten
Copy link
Contributor Author

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

@codendone codendone added Status: Fixed fix-released The fix has been in a release (experimental, preview, stable, or servicing). and removed needs-triage Status: Fixed internally The issue has been fixed but has not yet shipped in a release. labels Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Composition area-Deployment Issues related to packaging, installation, runtime (e.g., SelfContained, Unpackaged) breaking change fix-released The fix has been in a release (experimental, preview, stable, or servicing). Status: Fixed
Projects
None yet
Development

No branches or pull requests

5 participants