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

#1479 Rework conditional packages in v6 branch including adding net 6 #1482

Merged

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 & net 462 TFM and not the newly added net 6.0 TFM.

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: #1479

@lukebakken lukebakken self-assigned this Jan 28, 2024
@lukebakken lukebakken added this to the 6.9.0 milestone Jan 28, 2024
@thompson-tomo thompson-tomo force-pushed the enhancement/#1479_ConditionalPackagesV6 branch from bae304e to 7c1c137 Compare February 13, 2024 12:11
@lukebakken lukebakken self-requested a review February 13, 2024 15:03
@lukebakken lukebakken force-pushed the enhancement/#1479_ConditionalPackagesV6 branch 2 times, most recently from b7c81fe to e418160 Compare February 13, 2024 21:49
Copy link
Contributor

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

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

@thompson-tomo by introducing the net6.0 target, this PR no longer builds, due to conditional code here:

https://github.com/thompson-tomo/rabbitmq-dotnet-client/blob/enhancement/%231479_ConditionalPackagesV6/projects/RabbitMQ.Client/util/NetworkOrderDeserializer.cs

However, if I change

#elif NETSTANDARD

to just #else, then the test suite fails with framing errors.

https://github.com/rabbitmq/rabbitmq-dotnet-client/actions/runs/7893177389/job/21541242245

If you run RabbitMQ locally, and the test suite, you will see RabbitMQ log errors such as these:

[error] <0.750.0> closing AMQP connection <0.750.0> (10.0.0.2:35296 -> 172.17.0.2:5672):
[error] <0.750.0> {handshake_error,starting,0,
[error] <0.750.0>                  {amqp_error,frame_error,
[error] <0.750.0>                              "type 1, all octets = <<>>: {invalid_frame_end_marker,0}",
[error] <0.750.0>                              none}}

Could you please investigate?

@lukebakken
Copy link
Contributor

@danielmarbach when you made this comment, I'm assuming the kinds of failures you were predicting are exactly what I see exposed in this PR (comment)

@thompson-tomo thompson-tomo force-pushed the enhancement/#1479_ConditionalPackagesV6 branch 2 times, most recently from 5330e26 to 12a73a3 Compare March 12, 2024 22:23
Revert NetworkOrderDeserializer

Have ported some changes from V7 + Reviewed conditional compiles
@lukebakken lukebakken force-pushed the enhancement/#1479_ConditionalPackagesV6 branch from 12a73a3 to 4b97b3a Compare March 15, 2024 19:48
@lukebakken lukebakken merged commit 08e1837 into rabbitmq:6.x Mar 15, 2024
5 checks passed
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.

None yet

2 participants