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.

Sign up for GitHub

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

Jump to bottom

Use MSBuildLocator for tests #57613

Merged
merged 16 commits into from
Nov 22, 2021
Merged

Use MSBuildLocator for tests #57613

merged 16 commits into from
Nov 22, 2021

Conversation

jmarolf
Copy link
Contributor

@jmarolf jmarolf commented Nov 5, 2021

builds on top of @JoeRobich 's excellent work in this PR: #52064

@jmarolf jmarolf requested review from a team as code owners November 5, 2021 23:02
JoeRobich
JoeRobich reviewed Nov 5, 2021
View reviewed changes
src/Workspaces/MSBuildTest/Resources/NetCoreApp2/Project.csproj
@@ -2,7 +2,7 @@

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>netcoreapp2.1</TargetFramework>
<TargetFramework>netcoreapp3.1</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change the folder name to match? or maybe remove the version from the folder?

eng/Versions.props Outdated Show resolved Hide resolved
@jmarolf jmarolf force-pushed the infra/update-msbuildworkspace-testings branch 2 times, most recently from 3a674da to 1da68bf Compare November 8, 2021 09:25
@runfoapp runfoapp bot mentioned this pull request Nov 8, 2021
Closed
jaredpar
jaredpar reviewed Nov 8, 2021
View reviewed changes
eng/Versions.props Outdated Show resolved Hide resolved
src/Compilers/Test/Core/DotNetCoreSdk.cs Outdated
@@ -22,6 +22,7 @@ public override string SkipReason

static DotNetCoreSdk()
{
#if NETCOREAPP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change necessary here? Trying to understand how this PR could have caused an error here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really make sense to use the .NET SDK from .NET Framework. essentially if we are running tests from .NET Framework we do not want SDK-based tests to run.

src/Workspaces/MSBuildTest/DotNetSdkMSBuildInstalled.cs Outdated
.OrderByDescending(instances => instances.Version)
.FirstOrDefault();

if (s_instance != null && !MSBuildLocator.IsRegistered)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can s_instance be non-null here?

src/Workspaces/MSBuildTest/MSBuildInstalled.cs Outdated Show resolved Hide resolved
src/Workspaces/MSBuildTest/MSBuildInstalled.cs Outdated
: base(minimumVsVersion: new Version(16, 9), minimumSdkVersion: new Version(5, 0, 201))
{
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having a type per common version why not just have the ctor of MSBuildInstalled take a string constant that can be parsed into a version? Then we can export constants for all the versions that matter. Seems less heavy handed than inheritance.

src/Workspaces/MSBuildTest/Resources/CircularProjectReferences/CircularCSharpProject1.csproj Outdated Show resolved Hide resolved
@jmarolf jmarolf force-pushed the infra/update-msbuildworkspace-testings branch from da3d724 to acba73b Compare November 9, 2021 21:11
@runfoapp runfoapp bot mentioned this pull request Nov 9, 2021
Open
jaredpar
jaredpar requested changes Nov 10, 2021
View reviewed changes
Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of NET6_0 define in the change is brittle. Would like to understand why we can't use one of the more flexible defines or barring that how we make this code resilient to future target framework changes.

src/Workspaces/MSBuildTest/DotNetSdkMSBuildInstalled.cs Outdated
@@ -25,26 +25,33 @@ static DotNetSdkMSBuildInstalled()
}
}

#if NET6_0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This #if pattern is fragile in the face of changing target frameworks. Consider either using NET6_0_OR_GREATER or NETCOREAPP. if this must change when we change the target framework then please use the following pattern

#if NET6_0
...
#elif NETCOREAPP
#error bad target framework
#endif

jaredpar
jaredpar approved these changes Nov 10, 2021
View reviewed changes
src/Workspaces/MSBuildTest/DotNetSdkMSBuildInstalled.cs Outdated
using Microsoft.Build.Locator;
using Roslyn.Test.Utilities;

namespace Microsoft.CodeAnalysis.MSBuild.UnitTests
{
internal partial class DotNetSdkMSBuildInstalled : ExecutionCondition
{
#if NETCOREAPP3_1_OR_GREATER
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious: is there a reason you prefer this over #if NETCOREAPP? don't think we use earlier than netcoreapp3.1 anymore.

This was referenced Nov 10, 2021
Closed
Closed
Closed
Closed
@jmarolf jmarolf force-pushed the infra/update-msbuildworkspace-testings branch from c404d64 to 29fba84 Compare November 17, 2021 07:11
@jmarolf jmarolf force-pushed the infra/update-msbuildworkspace-testings branch 2 times, most recently from baaf703 to 9109b93 Compare November 17, 2021 07:22
JoeRobich
JoeRobich approved these changes Nov 19, 2021
View reviewed changes
Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@JoeRobich JoeRobich mentioned this pull request Nov 19, 2021
Closed
@jmarolf jmarolf merged commit f289db2 into dotnet:main Nov 22, 2021
@jmarolf jmarolf deleted the infra/update-msbuildworkspace-testings branch November 22, 2021 18:53
@ghost ghost added this to the Next milestone Nov 22, 2021
allisonchou added a commit to allisonchou/roslyn that referenced this pull request Nov 23, 2021
@allisonchou
Revert "Merge pull request dotnet#57613 from jmarolf/infra/update-msb…
dd5dc8a 
…uildworkspace-testings"

This reverts commit f289db2, reversing
changes made to d1a34b0.
@allisonchou allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoeRobich JoeRobich JoeRobich approved these changes

@jaredpar jaredpar jaredpar approved these changes

Assignees
No one assigned
Labels
Area-Infrastructure
Projects
None yet
Milestone
17.1.P2
Development

Successfully merging this pull request may close these issues.
4 participants
@jmarolf @jaredpar @JoeRobich @allisonchou