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

Replace netstandard1.0 and netstandard1.3 with netstandard2.0 #3921

Merged
merged 2 commits into from
Aug 24, 2022

Conversation

Evangelink
Copy link
Member

@Evangelink Evangelink commented Aug 2, 2022

Description

Target .netstandard2.0 instead of netstandard1.0/netstandard1.3

Related issue

Fixes AB#1585589

@@ -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
Copy link
Member Author

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">
Copy link
Member Author

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?

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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? -->
Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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">
Copy link
Member Author

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?

Copy link
Member

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?

Copy link
Member Author

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.

@MarcoRossignoli
Copy link
Contributor

@nohwnd there's UWP discussion open I think, what to ship in the nuget.

Copy link
Member

@nohwnd nohwnd left a 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.

Copy link
Contributor

@Haplois Haplois left a 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 removing netstandard1.0 only.

@Evangelink
Copy link
Member Author

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.

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Aug 22, 2022

Said versions are already retired in general access branches

what does it mean?Can you elaborate?

@Haplois
Copy link
Contributor

Haplois commented Aug 22, 2022

Said versions are already retired in general access branches

what does it mean?Can you elaborate?

They are already retired in consumer and enterprise editions, only supported channel is LTSC.

@Evangelink
Copy link
Member Author

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.

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

4 participants