-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix publishing .NET Standard facades #19731
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,8 +50,9 @@ public void It_solves_conflicts_between_package_and_implicit_references() | |
var files = getValuesCommand.GetValues() | ||
.Where(file => file.Contains(reference)); | ||
files.Count().Should().Be(1); | ||
// We should choose the file from system.runtime.interopservices.runtimeinformation package version | ||
files.FirstOrDefault().Contains(@"system.runtime.interopservices.runtimeinformation\4.3.0").Should().BeTrue(); | ||
|
||
// We should choose the system.runtime.interopservices.runtimeinformation file from Microsoft.NET.Build.Extensions as it has a higher AssemblyVersion (4.0.2.0 compared to 4.0.1.0) | ||
files.FirstOrDefault().Contains(@"Microsoft.NET.Build.Extensions\net461\lib\System.Runtime.InteropServices.RuntimeInformation.dll").Should().BeTrue(); | ||
Comment on lines
+54
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @joperezr @ericstj I had to change this test to expect the file from Microsoft.NET.Build.Extensions to win instead of the one from the package. This seems legit, here's the message from the binlog:
I'm not sure why the test originally expected the other file to win, maybe it was always a bug. Does this seem OK? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How was the test even passing before this change? I'm seeing the same behavior locally as the test after your change as well, and also for the copyLocal item file:
Given this, I'm vine with fixing the test as well as I agree that it must have been a bug before unless this was written before we serviced the Microsoft.NET.Build.Extensions facades a long time ago and we were actually expecting the package to win. |
||
} | ||
|
||
[Theory] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
// Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
// | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.IO; | ||
using System.Linq; | ||
using System.Text; | ||
using FluentAssertions; | ||
using Microsoft.DotNet.Cli.Utils; | ||
using Microsoft.NET.TestFramework; | ||
using Microsoft.NET.TestFramework.Assertions; | ||
using Microsoft.NET.TestFramework.Commands; | ||
using Microsoft.NET.TestFramework.ProjectConstruction; | ||
using Xunit; | ||
using Xunit.Abstractions; | ||
|
||
namespace Microsoft.NET.Publish.Tests | ||
{ | ||
public class PublishNetFrameworkApp : SdkTest | ||
{ | ||
public PublishNetFrameworkApp(ITestOutputHelper log) : base(log) | ||
{ | ||
} | ||
|
||
[Fact] | ||
public void NetStandardFacadesArePublished() | ||
{ | ||
var netStandardProject = new TestProject() | ||
{ | ||
Name = "NetStandardProject", | ||
TargetFrameworks = "netstandard2.0" | ||
}; | ||
|
||
var testProject = new TestProject() | ||
{ | ||
TargetFrameworks = "net461", | ||
IsExe = true | ||
}; | ||
testProject.ReferencedProjects.Add(netStandardProject); | ||
|
||
var testAsset = _testAssetsManager.CreateTestProject(testProject); | ||
|
||
var publishCommand = new PublishCommand(testAsset); | ||
|
||
publishCommand.Execute() | ||
.Should() | ||
.Pass(); | ||
|
||
// There are close to 100 facades that should be copied, just check for a few of them here | ||
publishCommand.GetOutputDirectory(testProject.TargetFrameworks) | ||
.Should() | ||
.HaveFiles(new[] | ||
{ | ||
"netstandard.dll", | ||
"System.IO.dll", | ||
"System.Runtime.dll" | ||
}) | ||
.And | ||
.NotHaveFile("netfx.force.conflicts.dll"); | ||
} | ||
} | ||
} |
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.
Perhaps we should also update the comment above since it still says that Private is set to false?
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.
Private is still set to false on the
Reference
items which are created in theAddFacadesToReferences
task. That's what the comment is supposed to be referring to, as far as I understand.