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

#1480 Use conditional packages for v7 release #1481

Conversation

thompson-tomo
Copy link
Contributor

@thompson-tomo thompson-tomo commented Jan 27, 2024

Proposed Changes

With this change the polyfill package references have a condition placed on them so that they are only added for the net standard 2.0 TFM and not net 6.0.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in related repositories

Further Comments

Closes: #1480

@danielmarbach
Copy link
Collaborator

Generally I would suggest grouping the private dependencies separately

  <ItemGroup Label="Private dependencies">
   ...
  </ItemGroup>

when it comes to having a TFM specific group I'd agree it makes sense under specific circumstances. The problem is though once you start moving things into TFM specific groups you can only use the API that is exposed for that version for that TFM. So for System.Threading.Channel you'd be seeing the API surface for the NET6 version and for Netstandard the API surface for the 8.0 version. That can lead to suttle differences that you might end up having to conditionally compile. For example System.Text.Json introduces default properties in the newer versions while the previous once don't have it. If you'd want to use such an API you have different API surface between different TFMs.

Maybe the important question is whether we want to benefit for those reference libraries from potential API surface improvements that we can leverage within the client that would otherwise not be available.

Given the age of NET6 I'd also argue that probably we'd want to bump to NET8 since this is for the main branch which isn't yet released.

@thompson-tomo
Copy link
Contributor Author

thompson-tomo commented Feb 1, 2024

The comment from @danielmarbach ties into the review thread I closed. I would propose a discussion topic is started to discuss when to up dependencies as currently a number of people have differing opinions.

In relation to the points raised I totally agree that we should try to avoid framework specific code where ever possible. After thinking about this a little bit more, perhaps we should tie dependencies to the Min version which can be provided by the framework ie 6.x and if functionality in a newer version is needed we add an additional TFM and make nuget fetch the library on older TFM.

But this PR focuses on what was mentioned in the issue which is to only add packages if not provided by the framework.

@lukebakken
Copy link
Contributor

@thompson-tomo please see the following - #1482 (review)

It appears that the concern @danielmarbach expressed has been uncovered in that PR.

@lukebakken lukebakken force-pushed the enhancement/#1480_ConditionalPackagesForV7 branch from e4a8f3a to ee9b9a4 Compare March 11, 2024 19:33
@thompson-tomo thompson-tomo force-pushed the enhancement/#1480_ConditionalPackagesForV7 branch from ee9b9a4 to 1983490 Compare March 12, 2024 22:01
@thompson-tomo
Copy link
Contributor Author

thompson-tomo commented Mar 14, 2024

Tests are now working, so have rebased

…m.IO.Pipelines, System.Text.Json and System.Net.Http.Json to minimum versions necessary

Bump Erlang version, add comment
@lukebakken lukebakken force-pushed the enhancement/#1480_ConditionalPackagesForV7 branch from 8157a4e to 94bcfe7 Compare March 15, 2024 22:52
@lukebakken lukebakken merged commit 6f8191b into rabbitmq:main Mar 16, 2024
11 checks passed
@lukebakken
Copy link
Contributor

Thank you @thompson-tomo!

@thompson-tomo thompson-tomo deleted the enhancement/#1480_ConditionalPackagesForV7 branch March 16, 2024 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimise dependencies needed for v7 release
6 participants