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

7.0: update target framework from net461 to netcoreapp3.1 #971

Merged
merged 1 commit into from
Nov 6, 2020

Conversation

bollhals
Copy link
Contributor

@bollhals bollhals commented Nov 6, 2020

Proposed Changes

7.0 Change

  • Deprectates the net461 branch and activates netcoreapp3.1
  • Cleans up NetworkOrder(De)Serialization class and aligns read/write for double

Types of Changes

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

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • 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
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

@michaelklishin michaelklishin merged commit c4fd971 into rabbitmq:master Nov 6, 2020
@michaelklishin michaelklishin added this to the 7.0.0 milestone Nov 6, 2020
@@ -28,6 +28,7 @@
<PackageOutputPath>..\..\packages</PackageOutputPath>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<CodeAnalysisRuleSet>RabbitMQ.ruleset</CodeAnalysisRuleSet>
<LangVersion>latest</LangVersion>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not set the language version to latest. Because that allows you to use language features that might not be supported by the TFMs chosen here. If you leave it blank you get automatically the proper language version supported by the TFM intersection

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/configure-language-version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't "The compiler accepts syntax from the latest released version of the compiler" mean it will always choose the latest possible whatever framework version we selected?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it will choose the the latest version the compiler knows about, overriding the version that would have been picked by the compiler based on the TFM.

@danielmarbach is right that it should not be set to get the automatic behavior that we want here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's wrong then, sorry. I'll update it in one of my next PRs. Thanks for the review

@michaelklishin michaelklishin modified the milestones: 8.0.0, 7.0.0 Dec 24, 2020
@michaelklishin michaelklishin changed the title 7.0 -> update target framework from net461 to netcoreapp3.1 7.0: update target framework from net461 to netcoreapp3.1 Dec 24, 2020
@michaelklishin
Copy link
Member

@bollhals I failed to backport this to 7.x twice because there are too many optimizations and [the elimination of] code generation changes in master. Can you please submit a version of this based on 7.x?

@michaelklishin michaelklishin modified the milestones: 7.0.0, 8.0.0 Dec 24, 2020
@bollhals bollhals deleted the updateFramework branch March 2, 2021 20:52
@lukebakken lukebakken modified the milestones: 8.0.0, 7.0.0 Mar 8, 2022
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.

5 participants