-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Microsoft.Extensions.Internal.Transport and System.Numerics.tensors are not stable shipping packages #41632
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
This is approved for 5.0 as it is ensuring we ship the correct bits. |
<!-- This is a preview package. Do not mark as stable. --> | ||
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion> | ||
<IsShippingAssembly>false</IsShippingAssembly> | ||
<IsShipping>false</IsShipping> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong. As System.Numerics.Tensors
is a shipping package but with a non-stable version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which means it won't make it to nuget.org anymore. I believe we want that to go to nuget.org. I asked this a while ago.
#33636 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you want to do:
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as we had it. We shouldn't change this package at all. We need IsShippingAssembly=false
so that the assembly metadata contains the non-stable version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont want to ship any rc or rtm versions of these packages. (as we did in 3.1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding <SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
will still ship the rc1 & rc2 version and we dont want that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok... why not instead exclude them from the build list in libraries-packages.proj and don't build them at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to build Microsoft.Extensions.Internal.Transport as it is necessary for aspnetcore.
For System.Numerics.Tensors we just want the smallest change to not produce the shipping package. As mentioned in the issue we'll remove this code completely in 6.0 and put it back in corefxlab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given my comment above.
@Anipik is there a master PR that adds the |
yeah it should be ported to master as well. i will do that. |
Fixes #41560