-
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
Dead Ending Microsoft.CSharp Package and Bumping the leftout assembly versions to 5.0.0.0 #2264
Conversation
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.
Looks good from my end (S.R.CS.U)
What are the implications of doing this? Also do we still produce the Microsoft.VisualBasic package. That is the VB equivalent of Microsoft.CSharp. I would be suspicious if we're in a state where we're producing one but not the other. |
No real implications other than having Microsoft.CSharp's only shipping mechanism being via the shared framework. Usually we try to keep OOBs shipping as package in case we want to add API to it after a release, but with Microsoft.CSharp we can't really do that because it is inbox in many places. The only real way to ship new API via a package for Microsoft.CSharp would be to have it target netcoreapp5.0 (since it targets netstandard2.0 today) and add the API to Netcoreapp 5.0 build only. Because we don't have any plans to do that (and even if we did, we can just do that directly on the shared framework) the best thing is just to stop building the package. M.Vb package was removed about a year ago via this PR: dotnet/corefx#34087 |
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.
@ericstj it probably doesn't matter much but should we change the configuration to netcoreapp (latest), away from netstandard2.0?
Agree with @ViktorHofer let's retarget this to netcoreapp5.0 and clean up the configurations. |
@@ -41,7 +41,7 @@ | |||
01 00 00 00 00 | |||
) // false | |||
.hash algorithm 0x00008004 | |||
.ver 4:0:5:0 | |||
.ver 5:0:0:0 |
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.
Separate issue. We should see about setting define's to flow the actual values from the build-system. That way we get real fileversion and assembly version in ILProj-built assemblies.
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.
Please re-target. While you are at it, you can clean up
runtime/src/libraries/pkg/descriptions.json
Lines 17 to 27 in 4f9ae42
{ | |
"Name": "Microsoft.CSharp", | |
"Description": "Provides support for compilation and code generation, including dynamic, using the C# language.", | |
"CommonTypes": [ | |
"Microsoft.CSharp.RuntimeBinder.Binder", | |
"Microsoft.CSharp.RuntimeBinder.RuntimeBinderException", | |
"Microsoft.CSharp.RuntimeBinder.CSharpArgumentInfo", | |
"Microsoft.CSharp.RuntimeBinder.CSharpArgumentInfoFlags", | |
"Microsoft.CSharp.RuntimeBinder.CSharpBinderFlags" | |
] | |
}, |
Can we also clean the BaselineVersion of the package index for Microsoft.CSharp:
|
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.
LGTM other than re-targetting and cleanup.
@joperezr thanks for the explanation. That all makes sense. What about my questions about Microsoft.VisualBasic though? After reading your explanation I feel more strongly that Microsoft.CSharp and Microsoft.VisualBasic should be consistent here. |
I probably shouldn't have used abbreviations for that part of my response above 😄
M.Vb I meant Microsoft.VisualBasic, so that package has been removed from our build for one year now. I'm not sure why at that time we didn't decide to also remove Microsoft.CSharp package, since I agree with you that these two should always be somewhat consistent. |
Great! This all seem good to me. |
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.
Approving the stopping of publishing Microsoft.CSharp as a package. Have not looked specifically at the changes involved here.
Test failure is due to inbox assembly version change. Requires temporary suppression here: Add IgnoredReference for System.Runtime.CompilerServices.Unsafe. |
Hello @Anipik! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
This is causing majority of PRs to fail with:
I am going to submit a revert. |
That's interesting seems like a bug in the live-live build. We should figure out what part of the stack is not running on live bits. |
…mpilerServices.Unsafe (#2354) * Dead Ending Microsoft.CSharp Package and Bumping the leftout assembly versions to 5.0.0.0 (#2264) * deadending Microsoft.Csharp Package and bumping the assembly versions * retargeting and clean up * Update Microsoft.CSharp.csproj * fixing the build * reference -> projectreference * adding ignored reference * removing package references and making R2rDump bin place to a specifc output directory * remove package reference from R2r dump as well * adding comment and targeting netstandard2.0 * adding back package references for netstandard lib build * adding back baseline version for consistency
@333fred @cston @jaredpar just to check if you guys are okay with the no longer producing the Microsoft.CSharp package.
Fixes #1918