-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add support in .NET SDK for implicit versions of AspNetCore.App and .All #2533
Add support in .NET SDK for implicit versions of AspNetCore.App and .All #2533
Conversation
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.
The items are definitely cleaner than a bunch of conditions and properties, so I approve of the approach in principle. My only concern is that we make sure that it doesn't have unintended consequences for VS.
{ | ||
|
||
// TODO: Provide way to opt out of warning when Version is specified (possibly with the DisableImplicitFrameworkReferences property) | ||
public class ApplyImplicitVersions : TaskBase |
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.
nit: sealed
public ITaskItem[] ImplicitPackageReferenceVersions { get; set; } = Array.Empty<ITaskItem>(); | ||
|
||
[Output] | ||
public ITaskItem[] PackageReferencesToUpdate { get; set; } |
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.
nit: private set
for output
// twice when building with implicit restore. So instead we generate the warnings here, and keep them | ||
// in an item where they'll be logged in a target that runs before build, but not before restore. | ||
[Output] | ||
public string[] SdkBuildWarnings { get; set; } |
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.
nit: private set
for output
string versionOnPackageReference = packageReference.GetMetadata(MetadataKeys.Version); | ||
if (string.IsNullOrEmpty(versionOnPackageReference)) | ||
{ | ||
|
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.
nit: extra whitespace
packageReference.SetMetadata("Publish", "true"); | ||
|
||
packageReferencesToUpdate.Add(packageReference); | ||
|
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.
nit: extra whitespace
var implicitReferencesForThisFramework = ImplicitPackageReferenceVersions | ||
.Select(item => new ImplicitPackageReferenceVersion(item)) | ||
.Where(item => item.TargetFrameworkVersion == this.TargetFrameworkVersion) | ||
.ToDictionary(implicitVersion => implicitVersion.Name); |
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.
N is small here, but this wouldn't be much more code if extracted into a simple loop and a separate method.
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.
Are you saying a separate method to construct the dictionary?
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.
Yes, that is what I was saying. Then you could use a few more lines in the extracted method to avoid all this LINQ. The reference to N being small was that it probably doesn't matter much for perf, but this is a new target that runs in design-time builds so I think it might be prudent to err on the safe side.
|
||
<ItemGroup> | ||
<PackageReference Remove="@(PackageReferenceToUpdate)" /> | ||
<PackageReference Include="@(PackageReferenceToUpdate)" /> |
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.
We should check if this has an observable effect on what happens to duplicate package refs with conflicting metadata. F# ran into bugs around this.
{ | ||
_item = item; | ||
} | ||
// The name / Package ID |
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.
nit: newline between closing brace and comment
public string DefaultVersion => _item.GetMetadata("DefaultVersion"); | ||
|
||
public string LatestVersion => _item.GetMetadata("LatestVersion"); | ||
|
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.
nit: extra whitespace
|
||
<UsingTask TaskName="ApplyImplicitVersions" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" /> | ||
|
||
<Target Name="ApplyImplicitVersions" BeforeTargets="_CheckForInvalidConfigurationAndPlatform;CollectPackageReferences"> |
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.
We should make sure to test VS thoroughly with this new dependency on CollectPackageReferences target to set versions: e.g. does it impact the dependency node or nuget package manager UI in any unexpected ways.
Approved for 2.2.1xx. |
|
||
<!-- If we've already warned that an implicit PackageReference was overridden, don't also warn that the version was explicitly | ||
specified --> | ||
<PackageReference Update="@(_PackageReferenceToRemove)" AllowExplicitVersion="true"/> |
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.
Note that this will update all PackageReference items due to dotnet/msbuild#1618
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.
Good catch
@@ -39,7 +39,8 @@ public GivenThatWeWantToBuildANetCoreApp(ITestOutputHelper log) : base(log) | |||
[InlineData("netcoreapp1.0", "1.0.3", "1.0.3", "1.0.3")] | |||
[InlineData("netcoreapp1.1", null, "1.1.2", "1.1.2")] | |||
[InlineData("netcoreapp1.1", "1.1.0", "1.1.0", "1.1.0")] | |||
[InlineData("netcoreapp1.1.1", null, "1.1.1", "1.1.1")] | |||
// Putting a patch in the TargetFramework property is no longer supported with the switch to ImplicitPackageReferenceVersion items |
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.
Hmm... Not sure about breaking this.
51c97c8
to
c62981d
Compare
@nguerrera @livarcocc This PR should be ready for review now. I've removed the WIP tag |
@dotnet/dnceng Leg failed due to out of disk space: https://ci.dot.net/job/dotnet_sdk/job/release_2.2.1xx/job/debug_ubuntu14.04_prtest/109/ |
@dotnet-bot test Ubuntu14.04 Debug |
@dsplaisted Thank you for the tag. Looking into it. |
That's weird. I've never seen it happen twice in a row. Same machine both times. @mmitche machine was |
@dotnet-bot test Ubuntu14.04 Debug |
1 similar comment
@dotnet-bot test Ubuntu14.04 Debug |
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.
Code looks good. Still want to make sure we test VS scenarios and we should think about if anyone else might be depending on PackageReference items at evaluation.
@@ -401,4 +401,7 @@ The following are names of parameters or literal values and should not be transl | |||
<value>NETSDK1070: The application configuration file must have root configuration element.</value> | |||
<comment>{StrBegin="NETSDK1070: "}</comment> | |||
</data> | |||
</root> | |||
<data name="PackageReferenceVersionNotRecommended" xml:space="preserve"> | |||
<value>NETSDK1071: A PackageReference to '{0}' specified a Version of `{1}`. Specifying the version of this package is not recommended. For more information, see https://aka.ms/sdkimplicitrefs</value> |
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.
We're not supposed to use two spaces after a period:
var implicitReferencesForThisFramework = ImplicitPackageReferenceVersions | ||
.Select(item => new ImplicitPackageReferenceVersion(item)) | ||
.Where(item => item.TargetFrameworkVersion == this.TargetFrameworkVersion) | ||
.ToDictionary(implicitVersion => implicitVersion.Name); |
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.
Yes, that is what I was saying. Then you could use a few more lines in the extracted method to avoid all this LINQ. The reference to N being small was that it probably doesn't matter much for perf, but this is a new target that runs in design-time builds so I think it might be prudent to err on the safe side.
itemsToRemove.Add(item); | ||
string message = string.Format(CultureInfo.CurrentCulture, Strings.PackageReferenceOverrideWarning, | ||
item.ItemSpec, | ||
MoreInformationLink); |
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.
nit, you can pass arguments to LogWarning and let it format for you.
Log.LogWarning(Strings.PackageReferenceOverrideWarning, item.ItemSpec, MoreInformationLink);
(I know you just move this.)
// won't generate another error. The easiest way to do this is to add them both to a list of | ||
// items to remove, and then a list of items which gets added back. | ||
itemsToRemove.Add(item); | ||
item.SetMetadata(MetadataKeys.AllowExplicitVersion, "true"); |
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.
This makes me more curious about whether mutations to items are visible outside the task. Logically, I expected itemsToAdd to start via TaskItem(ITaskItem)
"copy" constructor.
fa77a9d
to
901f8c8
Compare
…nces are disabled
901f8c8
to
101e3fd
Compare
To be clear: this only adds support for the version less package references and there is no plan to port the 3.0 |
For the layman, does this mean we wont have to make our unit tests project reference the Web SDK or add a reference to App in our test projects? Implicit version will just work now? |
@dasMulli Yes, we decided not to add @VictorioBerra In 2.2, you will still need to have a In 3.0, the ASP.NET dependency will be expressed as a |
This adds support for implicit versions on PackageReferences to Microsoft.AspNetCore.App and Microsoft.AspNetCore.All. Currently this is implemented in the Web SDK, which causes problems when you have a non-Web project (for example a test project) referencing a Web project. Adding this to the base Microsoft.NET.Sdk will allow the versionless AspNetCore PackageReference in any project.
Description
Move logic to implicitly set version of ASP.NET package references into .NET SDK.
Customer Impact
Allows projects which do not use the Web SDK to reference Microsoft.AspNetCore.App without specifying the version. This is especially important for test projects which reference Web projects. Currently, the test project needs reference the ASP.NET package, and specify the exact version. Since the version is implicit in the ASP.NET project, it's hard to know which version to use, and it can change, leading to package downgrade warnings, if the project is built with a RID specified.
With this change, the test project will be able to specify a versionless
PackageReference
to ASP.NET the same way the web project can.Regression?
No
Risk
In addition to moving the logic, this PR uses an MSBuild task to select the implicit version instead of static MSBuild logic. So there is some risk associated with that change, but it is counterbalanced by the fact that the version selection code becomes a lot clearer, which reduces the risk of getting it wrong.
Notes
Additional PRs will be required along with this PR:
ImplicitPackageReferenceVersion
items in the bundled versions file