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

Update xunit version on release branch #59277

Merged
merged 5 commits into from
Sep 22, 2021

Conversation

maryamariyan
Copy link
Member

Cherry pick of #59275

Resolved conflicts:
eng/Versions.props

 Conflicts:
	eng/Versions.props
@ghost
Copy link

ghost commented Sep 17, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Cherry pick of #59275

Resolved conflicts:
eng/Versions.props

Author: maryamariyan
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@ericstj
Copy link
Member

ericstj commented Sep 21, 2021

Per offline conversation, @maryamariyan identified the failures here were caused by xunit/xunit@87322e9#diff-002769cbe13c8eb7dfb43a11a837108f0c269aedb5999a7495dad878826ee013L16

That change included NETStandard.Library which increased the number of package we saw in these test project graphs which overlap with projects. NuGet assumes that our projects should be used, but those no longer support the older TFMs and thus the restore failure.

@maryamariyan will follow up with @bradwilson and @clairernovotny to see if that change was intentional and see if we want that in user projects.

We can workaround it by adding a newer NETStandard.Library package reference to these test projects, since the newer package will avoid package references on frameworks where the assembly is inbox, rather than relying on that package to apply no asset (as we did in 1.x).

@ViktorHofer
Copy link
Member

We can workaround it by adding a newer NETStandard.Library package reference to these test projects, since the newer package will avoid package references on frameworks where the assembly is inbox, rather than relying on that package to apply no asset (as we did in 1.x).

We're already doing that at least for .NETCoreApp:

<PackageReference Include="NETStandard.Library"
Version="$(NetStandardLibraryVersion)"
Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'" />

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

see comments

@bradwilson
Copy link

What's the proposed fix on the xUnit.net side?

@ericstj
Copy link
Member

ericstj commented Sep 22, 2021

What's the proposed fix on the xUnit.net side?

I think it would be to add back the empty dependency group, so that it's like this:
image

@bradwilson
Copy link

I was led astray by the tooling. 😂 Version 2.4.2-pre.11 is on MyGet with the fix.

image

@ericstj
Copy link
Member

ericstj commented Sep 22, 2021

I was led astray by the tooling.

I see. The empty dependency group is a manual optimization folks did a lot with netstandard1.x that the tooling doesn't know about. An empty dependency group is correct and beneficial because NETStandard.Library is supported inbox on that framework. We've discussed helping folks make these optimizations in package validation, but we haven't implemented it yet and that wouldn't silence the nuget warning.

Technically you can "teach" nuget about the empty dependency group by cross-compiling for net452 and not referencing any packages: we did this for a lot of our netstandard packages to reduce package graph size and avoid app-local facades (for netstandard2.0). I think applying nowarn is the smallest fix to do in servicing, thank you @bradwilson for making this fix.

@ericstj
Copy link
Member

ericstj commented Sep 22, 2021

Merging this infrastructure reliability fix as tell-mode.

@ericstj ericstj closed this Sep 22, 2021
@ericstj ericstj reopened this Sep 22, 2021
@ericstj ericstj merged commit 491ed9a into dotnet:release/6.0 Sep 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants