Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Don't reference the netstandard shim inside the shared framework #53023
Don't reference the netstandard shim inside the shared framework #53023
Changes from 14 commits
84a7872
0d414d3
35497b1
15f7a63
a245863
494691d
7559971
86f2e60
b1b8c5e
7526e04
5e963d9
774f5ae
2c3d9f0
f8fa9c7
fa2d97d
577c0f7
4db5ecd
f5123ff
19dd369
1b9c2db
f6175b8
a65671c
459305d
01a9ab2
2da8671
f461ee8
3855df9
120d839
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
CC @maryamariyan @eerhardt
You're adding either
NETCoreAppCurrent
ornetcoreapp3.1
configurations to a lot of libraries.What made you choose one vs another? For those that are
NETCoreAppCurrent
do you expect to add more configurations with every release?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.
In this specific case I added the
NetCoreAppCurrent
tfm to the reference project as it was already present in the source project. My strategy was:netcoreapp3.1
. In some cases that would have resulted in additional internal files being loaded that aren't necessary on > NET 5 which is why I also added aNetCoreAppCurrent
configuration.NetCoreAppCurrent
tfm to OOBs which don't target .NETCoreApp but where it's desirable to avoid the netstandard.dll shim. The desire to avoid the netstandard.dll shim is defined by where the library sits in the graph. If it's at the very top and isn't referenced by any other component it's less of a concern. Going forward it would be nice to avoid the netstandard.dll shim altogether but that's TBD and unrelated to this work.The
netcoreapp3.1
tfms will be upgraded tonet6.0
and theNetCoreAppCurrent
tfms will be replaced bynet6.0
+NetCoreAppCurrent
when the repository targets .NET 7.Related but not yet tackled, there's the discussion to add a .NETCoreApp (presumably
NetCoreAppCurrent
) tfm to every applicable library that already targets .NETStandard to benefit from linker/compiler/analyzer features. I would like to keep that discussion separate from this effort though.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.
Also to benefit from new APIs as well.
I'm all for targeting the latest TFM in our OOBs all the time. Especially in the OOBs that go inbox in other teams shared frameworks. I like that we have a decent plan so the TFMs don't continually grow and this strategy will be maintainable.
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 thought we were trying to avoid
netstandard
on .NET Core. Doesn't this go against the goal of this PR?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.
Repeated in other .csprojs.
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.
The PR only targets getting rid of the netstandard reference inside the shared framework and for some OOBs (which are in the middle of the graph) for NetCoreAppCurrent. There are still 50 libraries that don't have a .NETCoreApp configuration and all of those target .NETStandard. For referencing those we need the netstandard.dll shim as that's the piece that makes referencing a .NETStandard lib from .NETCoreApp work.
My master plan is that still during the .NET 6 timeframe we add a NetCoreAppCurrent configuration to every "applicable" library and with that avoid the necessity of the shim for NetCoreAppCurrent inside our build entirely.
When we then version the repository to .NET 7, we will have a net6.0 and a .net7.0 configuration in all our "applicable" packages and by that then be able to remove the netstandard.dll shim for .NETCoreApp entirely.
Getting rid of the netstandard.dll shim for netcoreapp3.1 would be unwise at this point as we just need to wait until we version the repo and get to net6.0 if my master plan succeeds ;)
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.
Are we sure dropping
netstandard2.1
here is correct? Looks like @maryamariyan added it during getting this project moved over to this repo. 2220437There 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.
The netstandard2.1 reference isn't needed as netstandard2.0 fully satisfies the contract. It's not entirely clear to me why that configuration was added when bringing over the project but it doesn't serve any purpose, especially now that we are adding a .NETCoreApp configuration.
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.
Here's a difference from the two projects before this - targetting
netcoreapp3.1
, but referencingSystem.Runtime
here, vsnetstandard
above.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 project doesn't reference any .NETStandard projects (via P2P) therefore the shim isn't required.
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.
Do we have this written down? It is going to be near impossible for someone to discern the rules of when to reference what.
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 was already the case before this change for reference projects. Before this PR the netstandard shim was automatically referenced by any .NETCoreApp source project. Reference always had to add the netstandard reference manually when they were referencing a netstandard project.
I do have a presentation planned (target audience Libraries Team) where I will go over the current state of libraries and their packages. Will make sure that the right documentation is in place by then.
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.
@buyaa-n @joperezr @krwq @jeffhandley can you please double check if this is right? The platform analyzer warned for these OSs when I added the .NETCoreApp configuration to System.CodeDom.
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.
if this assembly only supported on certain platforms better to add a Supported attribute for those. If there is no such known list then it is OK, let's ask mono team review @marek-safar
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 think we don't have to support this on Android either
I don't know what is supported set as the API is pretty weird and uses Process to launch unknown command line.
There is also a question about general single-file support here and the APIs could be considered as incompatible with singe-file too (/cc @agocke @vitek-karas)