-
Notifications
You must be signed in to change notification settings - Fork 323
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
Replace netstandard1.0 and netstandard1.3 with netstandard2.0 #3921
Conversation
src/Microsoft.TestPlatform.AdapterUtilities/PublicAPI/PublicAPI.Shipped.txt
Show resolved
Hide resolved
@@ -11,28 +11,23 @@ namespace Microsoft.VisualStudio.TestPlatform.ObjectModel; | |||
/// <summary> | |||
/// Class that holds collection of traits | |||
/// </summary> | |||
#if NETFRAMEWORK | |||
#if NETFRAMEWORK // REVIEW ME: This could be enabled for netcore and netstandard |
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.
Not for this PR but is there any reason not to enable this for all tfms? The ones we have all support it.
@@ -19,14 +19,6 @@ | |||
branch="$BranchName$" | |||
commit="$CommitId$" /> | |||
<dependencies> | |||
<group targetFramework="uap10.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.
Shall we add netstandard2.0
in replacement of uap10.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.
don't we have netstandard2.0 in the layout?
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 have just checked and we have only net462
, netcoreapp3.1
and before uap10.0
. That's why I think adding netstandard2.0
would be useful.
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.
@nohwnd Any guidance here?
<dependency id="System.Security.Principal.Windows" version="[4.0.1, )" /> | ||
<dependency id="System.Collections.Specialized" version="[4.0.1, )" /> | ||
<group targetFramework="netstandard2.0"> | ||
<!-- REVIEW ME: Why don't we have dependency to Newtonsoft.Json here? --> |
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.
Other TFMs are declaring dependency on Newtonsoft.Json
. Why are we not doing it for netstandard2.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.
That looks like an error.
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 will fix this one as a follow-up PR so things don't get mixed up.
@@ -25,12 +25,6 @@ | |||
<dependency id="Newtonsoft.Json" version="$JsonNetVersion$"/> | |||
</group> | |||
|
|||
<group targetFramework="uap10.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.
I don't think it makes sense to have netstandard2.0 for testhost but then I am not sure what to ship for uap?
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.
Why it does not make sense to have netstandard2.0 for testhost?
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.
As we don't target netcoreapp, I thought this package is meant for specific scenario and so netstandard2.0
seemed too generic.
@nohwnd there's UWP discussion open I think, what to ship in the nuget. |
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.
Withdrawing my approval, so we can finish the uwp discussion.
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.
IMO, we should keep netstandard1.3
too.
If we change our baseline to netstandard2.0
we will lose UAP support for Windows 10 before RS3 (1709), the issue here is Windows 10 RS1 LTSC is supported until October 2026, and Windows 10 TH1 LTSC is supported until October 2025.
Said versions are already retired in general access branches, so I am OK either way.
So vote
- 👍 for removal of
netstandard1.3
too. - 👎 for keeping
netstandard1.3
, and removingnetstandard1.0
only.
For the record, the problem wouldn't be on TP side as it's not injected onto UWP but problem will happen for Test Frameworks. For example, we won't be able to ship new versions of MSTest still supporting these old versions of UWP as we will no longer have the correct netstandard for ObjectModel and related objects. @Haplois to correct me if I misunderstood your concern. |
what does it mean?Can you elaborate? |
They are already retired in consumer and enterprise editions, only supported channel is LTSC. |
We discussed the issue in a team meeting and decided to move forward with this change. @Haplois @MarcoRossignoli @nohwnd I'd like to have 3 approvals on this one if you have some time. |
Description
Target .netstandard2.0 instead of netstandard1.0/netstandard1.3
Related issue
Fixes AB#1585589