Microsoft.NET.Sdk.IL change from PackageVersion => Version suffix broke WinForms usage

[dagood]

dotnet/winforms#2533 (comment)

@dagood the PR is still failing to build due to an unresolved dependency

##[error]src\Accessibility\src\Accessibility.ilproj(0,0): error NU1102: Unable to find package runtime.win-x64.microsoft.netcore.ilasm with version (>= 5.0.0)
  - Found 1178 version(s) in dotnet-core [ Nearest version: 5.0.0-alpha.1.20054.2 ]
  - Found 792 version(s) in dotnet-coreclr [ Nearest version: 5.0.0-alpha1.19564.1 ]
  - Found 9 version(s) in nuget.org [ Nearest version: 2.0.8 ]
  - Found 0 version(s) in arcade
  - Found 0 version(s) in dotnet-eng

This is because WinForms uses MicrosoftNETCoreILAsmPackageVersion, but dotnet/coreclr#27606 changed the IL SDK to use MicrosoftNETCoreILAsmVersion.

runtime/src/coreclr/src/.nuget/Microsoft.NET.Sdk.IL/targets/Microsoft.NET.Sdk.IL.targets

Line 30 in 6425b2d

<MicrosoftNETCoreILAsmVersion Condition="'$(MicrosoftNETCoreILAsmVersion)' == ''">5.0.0</MicrosoftNETCoreILAsmVersion>

I think it would be reasonable to accept both forms in the IL SDK. However, if it's better to just close this as "won't fix" and have WinForms change the property name instead, I think it will be useful to have had this issue open to track this breaking change.

[safern]

I think we should stick with the current property name as we've been transition Versions.props to not contain PackageVersion but Version as a suffix only. It is cleaner and easier to read, and also PackageVersion is redundant. The reason why PackageVersion was supported at the beginning was because DARC had a bug where it only matched the property if the suffix was PackageVersion instead of Version

[dagood]

we've been transition Versions.props to not contain PackageVersion but Version as a suffix only.

Are all downstream users of Microsoft.NET.Sdk.IL also making this change? Is it ok to force this, and not have a clear error message?

[safern]

I don't know which are the users, but when we were still in corefx we made this change. This was done 2 months ago, so I would've expected other people to be broken by this already:

cdc3339

I think we can error out if PackageVersion is set and Version is not or at least emit a warning.

[dagood]

Oh huh. This breaking change was absorbed in the past by dotnet/winforms#2278, but the fix was reverted by dotnet/winforms#2497 to go down to a version before this change happened. /cc @RussKie

Anyway, taking the breaking change again passed winforms CI: dotnet/winforms#2647. Just waiting for a winforms reviewer.

Having a reasonable error message would have made this really easy to resolve and had WinForms deps flowing 9 days ago, FWIW. But now that it's over with, I don't know if it'll be hit again in the future.

[safern]

I guess we can close then? Or are you planning on putting up a PR to error/warn?

[dagood]

It's up to whoever makes decisions about Microsoft.NET.Sdk.IL to decide if the package should have these warnings/errors or not, IMO. I don't know enough about how/where this package is used to know for sure.

[RussKie]

It would good be make a note of this for the future - add better error messages for breaking changes.

@dagood fixed us now (thank you!), so no further actions are required from our side.

[safern]

cc: @jkoritzinsky I don't know who is the owner for Microsoft.NET.Sdk.IL

[jkoritzinsky]

I think @tannergooding is the owner for the SDK, but I'm not positive.

[tannergooding]

I did the initial implementation work for Microsoft.NET.Sdk.IL but I'm not sure if I'm the "owner". Most of the changes since the initial work have been done by those working on infrastructure.

[ViktorHofer]

Just found this issue. That change was done by me when we consolidated the repositories. As I believed that dotnet/runtime is the only consumer of Microsoft.NET.Sdk.IL I didn't warn in beforehand or add a fallback mechanism... Sorry for that. To learn from this, next time I'll:

1. Gather potential consumers by scanning GitHub and devdiv/dnceng.
2. Open an issue to discuss the potential breaking change and loop in (ideally) all consumers.
3. Warn in beforehand / add a fallback mechanism (in this case it would have been as simple as accepting both properties).

Again sorry for the disruption here. For dotnet/runtime we will use #33821 going forward, in addition to the other mentioned steps.

I think there's no further action necessary, hence closing.