search

cloudevents / sdk-csharp

CSharp SDK for CloudEvents
Apache License 2.0
278 stars 81 forks source link

Dependency MQTTnet out of date and has breaking changes. #269

Closed Clockwork-Muse closed 2 months ago

Clockwork-Muse commented 12 months ago

The dependency for MQTTnet is now at version 4.3.1.873, which includes some breaking changes (or at least some binary incompatibilities).

Attempting a project like so:

<Project Sdk="Microsoft.NET.Sdk.BlazorWebAssembly">

  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="CloudNative.CloudEvents" Version="2.7.1" />
    <PackageReference Include="CloudNative.CloudEvents.Mqtt" Version="2.7.1" />
    <PackageReference Include="CloudNative.CloudEvents.SystemTextJson" Version="2.7.1" />
    <PackageReference Include="Microsoft.AspNetCore.Components.WebAssembly" Version="6.0.18" />
    <PackageReference Include="Microsoft.AspNetCore.Components.WebAssembly.DevServer" Version="6.0.18" PrivateAssets="all" />
    <PackageReference Include="MQTTnet" Version="3.1.2" />
  </ItemGroup>

</Project>

... ends up yielding

dotnet build
MSBuild version 17.3.2+561848881 for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
/workspaces/event-client/Pages/FetchData.razor(64,30): error CS0012: The type 'MqttApplicationMessage' is defined in an assembly that is not referenced. You must add a reference to assembly 'MQTTnet, Version=3.0.15.0, Culture=neutral, PublicKeyToken=b69712f52770c0a7'. [/workspaces/event-client/event-client.csproj]
/workspaces/event-client/Pages/FetchData.razor(65,37): warning CS8602: Dereference of a possibly null reference. [/workspaces/event-client/event-client.csproj]
/workspaces/event-client/Pages/FetchData.razor(65,26): warning CS8604: Possible null reference argument for parameter 'item' in 'void List<string>.Add(string item)'. [/workspaces/event-client/event-client.csproj]

Build FAILED.

/workspaces/event-client/Pages/FetchData.razor(65,37): warning CS8602: Dereference of a possibly null reference. [/workspaces/event-client/event-client.csproj]
/workspaces/event-client/Pages/FetchData.razor(65,26): warning CS8604: Possible null reference argument for parameter 'item' in 'void List<string>.Add(string item)'. [/workspaces/event-client/event-client.csproj]
/workspaces/event-client/Pages/FetchData.razor(64,30): error CS0012: The type 'MqttApplicationMessage' is defined in an assembly that is not referenced. You must add a reference to assembly 'MQTTnet, Version=3.0.15.0, Culture=neutral, PublicKeyToken=b69712f52770c0a7'. [/workspaces/event-client/event-client.csproj]
    2 Warning(s)
    1 Error(s)

Time Elapsed 00:00:00.73
jskeet commented 12 months ago

Thanks - we'll need to have a think about what to do here. We'd need a new major version (at least for the MQTT package) if we just bump the dependency - and at that point, anyone still using v3 would have problems.

I wonder whether the right approach is to create a new package, CloudNative.CloudEvents.Mqtt4. What do you think?

Clockwork-Muse commented 12 months ago

On a personal level I get annoyed with versioning in the package/namespace name. I understand why people sometimes do it though.

jskeet commented 12 months ago

On a personal level I get annoyed with versioning in the package/namespace name. I understand why people sometimes do it though.

So what would your proposal be as an alternative? How would you suggest we support v3 users and v4 users? Or just not support v3 users any more?

Clockwork-Muse commented 12 months ago

Take the breaking change, drop v3 support (which hasn't been updated in > 18 mos). If v3 support is still needed for some reason, release/maintenance branch.

jskeet commented 12 months ago

Right. And presumably release this as a new major version of just the MQTT CE package. (We don't want to disrupt other users, but we do want to signal the breaking change to MQTT users.) This is something I've thought we'll probably end up needing for a while - this is just the forcing function. I think "release everything all at the same time" is probably still appropriate, and we can keep a common minor/patch version, but vary major version by package...

antiblue commented 2 months ago

Hi there, any update on increasing the version to MQTTnet 4.3.6?

jskeet commented 2 months ago

I haven't had time to look recently - I'll see what I can do.

jskeet commented 2 months ago

I think I'll go for the "update the major, keep minor and patch" the same. We'll probably keep a release tag based on the "core" package version number.

@captainsafia does that sound appropriate to you?

captainsafia commented 2 months ago

@jskeet Just to make sure I understand the path here, the idea is that if somebody wanted to consume a new major version of the MQTT transitive dependency they would make the following jump:

- <PackageReference Include="CloudNative.CloudEvents.Mqtt" Version="2.7.1" />
<PackageReference Include="CloudNative.CloudEvents.Mqtt" Version="3.7.1" />

With the notion that the the minor and patch versions will be shared with other CloudNative.CloudEvents.* dependencies in your application:

<PackageReference Include="CloudNative.CloudEvents.Mqtt" Version="3.7.1" />
<PackageReference Include="CloudNative.CloudEvents" Version="2.7.1" />

If later one, we end up making a minor (non-breaking change) to the core library, the user would need to rev as follows:

- <PackageReference Include="CloudNative.CloudEvents.Mqtt" Version="3.7.1" />
- <PackageReference Include="CloudNative.CloudEvents" Version="2.7.1" />
+ <PackageReference Include="CloudNative.CloudEvents.Mqtt" Version="3.8.1" />
+ <PackageReference Include="CloudNative.CloudEvents" Version="2.8.1" />
jskeet commented 2 months ago

I'm actually anticipating that the first release would be 3.8.0 / 2.8.0, but that's broadly right.

But users wouldn't actually need to specify CloudNative.CloudEvents at all, as they can just use the indirect reference from CloudNative.CloudEvents.Mqtt.

captainsafia commented 2 months ago

But users wouldn't actually need to specify CloudNative.CloudEvents at all, as they can just use the indirect reference from CloudNative.CloudEvents.Mqtt.

Yep, fair point. I was moreso trying to sort out what it would look like if multiple CE packages were used. The "bump major version, same minor/patch version" seems sufficient to me.

jskeet commented 1 month ago

This is now pushed as CloudNative.CloudEvents.Mqtt version 3.8.0