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

warning when dot is omitted from targetframework version in net5.0+ #3625

Closed
wants to merge 6 commits into from
Closed

warning when dot is omitted from targetframework version in net5.0+ #3625

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0954642
error when TPV is missing from dependencies
zkat Aug 28, 2020
1815b11
format fix
zkat Aug 28, 2020
f648027
add platform version check to pack, too
zkat Aug 31, 2020
5ef3780
warn on net5+ TFMs when dot is missing
zkat Sep 1, 2020
7869d57
fix detection of dots on dashed frameworks
zkat Sep 2, 2020
af6ee20
Update src/NuGet.Core/NuGet.Build.Tasks.Pack/PackTaskLogic.cs
zkat Sep 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions src/NuGet.Core/NuGet.Build.Tasks.Pack/PackTaskLogic.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ public PackArgs GetPackArgs(IPackTaskRequest<IMSBuildItem> request)
// This only needs to happen when packing via csproj, not nuspec.
packArgs.PackTargetArgs.AllowedOutputExtensionsInPackageBuildOutputFolder = InitOutputExtensions(request.AllowedOutputExtensionsInPackageBuildOutputFolder);
packArgs.PackTargetArgs.AllowedOutputExtensionsInSymbolsPackageBuildOutputFolder = InitOutputExtensions(request.AllowedOutputExtensionsInSymbolsPackageBuildOutputFolder);
packArgs.PackTargetArgs.TargetPathsToAssemblies = InitLibFiles(request.BuildOutputInPackage);
packArgs.PackTargetArgs.TargetPathsToSymbols = InitLibFiles(request.TargetPathsToSymbols);
packArgs.PackTargetArgs.TargetPathsToAssemblies = InitLibFiles(request.BuildOutputInPackage, request);
packArgs.PackTargetArgs.TargetPathsToSymbols = InitLibFiles(request.TargetPathsToSymbols, request);
packArgs.PackTargetArgs.AssemblyName = request.AssemblyName;
packArgs.PackTargetArgs.IncludeBuildOutput = request.IncludeBuildOutput;
packArgs.PackTargetArgs.BuildOutputFolder = request.BuildOutputFolders;
Expand Down Expand Up @@ -396,7 +396,7 @@ public bool BuildPackage(PackCommandRunner runner)
return runner.RunPackageBuild();
}

private IEnumerable<OutputLibFile> InitLibFiles(IMSBuildItem[] libFiles)
private IEnumerable<OutputLibFile> InitLibFiles(IMSBuildItem[] libFiles, IPackTaskRequest<IMSBuildItem> request)
{
var assemblies = new List<OutputLibFile>();
if (libFiles == null)
Expand All @@ -410,7 +410,7 @@ private IEnumerable<OutputLibFile> InitLibFiles(IMSBuildItem[] libFiles)
var finalOutputPath = assembly.GetProperty("FinalOutputPath");

// Fallback to using Identity if FinalOutputPath is not set.
// See bug https://github.com/NuGet/Home/issues/5408
// See bug https://github.com/NuGet/Home/issues/5408
if (string.IsNullOrEmpty(finalOutputPath))
{
finalOutputPath = assembly.GetProperty(IdentityProperty);
Expand All @@ -437,6 +437,31 @@ private IEnumerable<OutputLibFile> InitLibFiles(IMSBuildItem[] libFiles)
throw new PackagingException(NuGetLogCode.NU5027, string.Format(CultureInfo.CurrentCulture, Strings.InvalidTargetFramework, finalOutputPath));
}

if (!string.IsNullOrEmpty(targetFramework))
{
var fw = NuGetFramework.Parse(targetFramework);
if (!string.IsNullOrEmpty(fw.Platform) && fw.PlatformVersion == FrameworkConstants.EmptyVersion)
{
throw new PackagingException(
NuGetLogCode.NU1012,
string.Format(CultureInfo.CurrentCulture, Strings.InvalidPlatformVersion, targetFramework)
);
}
var isNet5EraTfm = fw.Version.Major >= 5 &&
kartheekp-ms marked this conversation as resolved.
Show resolved Hide resolved
StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetCoreApp, fw.Framework);
var isNetStandard = StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetStandard, fw.Framework);
zkat marked this conversation as resolved.
Show resolved Hide resolved
if (isNet5EraTfm || isNetStandard)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we checking the netstandard versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NuGet/Home#9215 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

There'll be no new versions of netstandard.
The true risk here is what happens when we hit net10.0. The risk is not there with netstandard.

Keep in mind that there are existing packages/projects with netstandard. I see no reason to raise an extra warning for something that won't really affect them.

{
var dotIdx = targetFramework.IndexOf('.');
var dashIdx = targetFramework.IndexOf('-');
var isDottedFwVersion = (dashIdx > -1 && dotIdx > -1 && dotIdx < dashIdx) || (dashIdx == -1 && dotIdx > -1);
if (!isDottedFwVersion)
{
request.Logger.LogWarning(string.Format(CultureInfo.CurrentCulture, Strings.MissingRequiredDot, targetFramework));
}
}
}

assemblies.Add(new OutputLibFile()
{
FinalOutputPath = finalOutputPath,
Expand Down Expand Up @@ -655,7 +680,7 @@ private IEnumerable<ContentMetadata> GetContentMetadata(IMSBuildItem packageFile
var newTargetPath = Path.Combine(targetPath, identity);
// We need to do this because evaluated identity in the above line of code can be an empty string
// in the case when the original identity string was the absolute path to a file in project directory, and is in
// the same directory as the csproj file.
// the same directory as the csproj file.
newTargetPath = PathUtility.EnsureTrailingSlash(newTargetPath);
newTargetPaths.Add(newTargetPath);
}
Expand Down Expand Up @@ -813,7 +838,7 @@ private static void InitializeProjectDependencies(

var versionToUse = new VersionRange(targetLibrary.Version);

// Use the project reference version obtained at build time if it exists, otherwise fallback to the one in assets file.
// Use the project reference version obtained at build time if it exists, otherwise fallback to the one in assets file.
if (projectRefToVersionMap.TryGetValue(projectReference.ProjectPath, out var projectRefVersion))
{
versionToUse = VersionRange.Parse(projectRefVersion, allowFloating: false);
Expand Down Expand Up @@ -872,7 +897,7 @@ private static void InitializePackageDependencies(
// Add each package dependency.
foreach (var packageDependency in packageDependencies)
{
// If we have a floating package dependency like 1.2.3-xyz-*, we
// If we have a floating package dependency like 1.2.3-xyz-*, we
// use the version of the package that restore resolved it to.
if (packageDependency.LibraryRange.VersionRange.IsFloating)
{
Expand Down
23 changes: 20 additions & 3 deletions src/NuGet.Core/NuGet.Build.Tasks.Pack/Strings.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion src/NuGet.Core/NuGet.Build.Tasks.Pack/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,17 +159,25 @@
<value>PackageVersion string specified '{0}' is invalid.</value>
<comment>{0} is the version.</comment>
</data>
<data name="InvalidPlatformVersion" xml:space="preserve">
<value>Platform version is missing from '{0}'</value>
<comment>{0} is the target framework string.</comment>
</data>
<data name="InvalidTargetFramework" xml:space="preserve">
<value>Invalid target framework for the file '{0}'.</value>
</data>
<data name="IsPackableFalseError" xml:space="preserve">
<value>This project cannot be packaged because packaging has been disabled. Add &lt;IsPackable&gt;true&lt;/IsPackable&gt; to the project file to enable producing a package from this project.</value>
</data>
<data name="MissingRequiredDot" xml:space="preserve">
<value>Dots in TargetFramework versions are required. The missing dot in '{0}' might cause future breakage.</value>
Copy link
Member

Choose a reason for hiding this comment

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

@JonDouglas do you have comments about this user-facing error message?

Copy link
Contributor

@JonDouglas JonDouglas Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@zkat & @zivkan

Dots in TargetFramework versions are required. The missing dot in '{0}' might cause future breakage.

->

The TargetFrameworkVersion value `{0}` was not recognized. It may be missing a dotted version number.

The TargetFrameworkVersion value `{0}` was not recognized. It may be missing a version number separated by periods.

or

The TargetFrameworkVersion value `{0}` is not valid. Ensure your TargetFrameworkVersion includes a dotted version number.

The TargetFrameworkVersion value `{0}` is not valid. Ensure your TargetFrameworkVersion includes a version number separated by periods.

Copy link
Member

Choose a reason for hiding this comment

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

The framework is technically valid and understood...just might not work the way that customer intended it to.

In particular the problem is that net50 is parses to version 5.0, and the eventual progression of NET will eventually lead to version 10.0, so if people still write frameworks with versions then net10, it'd get parsed to 1.0 while they might expect it to parse to 10.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, and we want to change the behavior of the customer to promote the best practice.

This is similar with mobile TFMs such as monoandroid10 vs. monoandroid10.0. One is 1.0 & the other is 10.0.

So TargetFramework = monoandroid, TargetFrameworkVersion = v1.0

If anything, we should be promoting the separator across the board, it's the best practice is it not? Everything in the NET 5 spec suggests to use a period separator for all TargetBlahVersion properties.

https://github.com/dotnet/designs/blob/master/accepted/2020/net5/net5.md#upgrading-the-os-bindings

https://github.com/dotnet/designs/blob/master/accepted/2020/net5/net5.md#lighting-up-on-later-os-versions

The bigger question is best practices for the <TargetFramework> element here. Is it period separator or not? Prior to .NET Core it looks like no, it doesn't matter. After .NET Core it seems that it should be included. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Separator across the board is the right approach.

Everything in the NET 5 spec suggests to use a period separator for all TargetBlahVersion properties.

Those are not user defined usually.
The idea is that the TargetFramework property can be anything.
NuGet doesn't parse it, which is why the check is only happening at pack time.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we create an issue on the SDK team, or contribute a pull request ourselves, to do it in the SDK instead? It doesn't make much sense to me to do at pack time. It should be done at restore, but only when the monikers were not provided and the TargetFramework property was parsed, hence why it can only be done in the SDK and not in NuGet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zivkan So what I'm hearing is that the suggestion is to drop this PR in favor of doing this SDK-side in a more general way?

As far as whether to warn always or warn only for certain frameworks -- I think the concern was that this could get very noisy, very quickly for people. I mean even for us, we'd need to change a bunch of net45/net472 items in our tests and projects?

Copy link
Member

Choose a reason for hiding this comment

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

yes, my opinion is close this PR, and someone should make a change in the SDK instead.

The key is, do not warn if my csproj has:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFrameworks>whatever</TargetFrameworks>
  </PropertyGroup>

  <PropertyGroup Condition=" '$(TargetFramework)' == 'whatever' ">
    <TargetFrameworkIdentifier>.NETCoreApp</TargetFrameworkIdentifier>
    <TargetFrameworkVersion>v5.0</TargetFrameworkIdentifier>
    <TargetFrameworkMoniker>$(TargetFrameworkIdentifier),Version=$(TargetFrameworkVersion)</TargetFrameworkMoniker>
  </PropertyGroup>
</Project>

This csproj doesn't have a . in the TargetFramework value, but it doesn't need one because it correctly sets the 3 other properties needed to correctly define the canonical target framework. Only when the SDK parses the TargetFramework value to set the other properties itself, in that situation warn about missing dots.

And yes, you're right about the noisy warnings. Just like this PR, we should only warn when the TFM ends up being TFI='.NETCoreApp' and TFV >= '5.0'. Or any TFI='.NETStandard' apparently.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should still do it on our side in pack because people can manually create packages. We should catch things like that in the nuspec and package folders.

<comment>{0} is the target framework string.</comment>
</data>
<data name="NoPackItemProvided" xml:space="preserve">
<value>No project was provided to the PackTask.</value>
</data>
<data name="NuGetLicenses_LicenseUrlCannotBeUsedInConjuctionWithLicense" xml:space="preserve">
<value>The PackageLicenseUrl is being deprecated and cannot be used in conjunction with the PackageLicenseFile or PackageLicenseExpression.</value>
<comment>Please don't localize PackageLicenseUrl, PackageLicenseFile and PackageLicenseExpression.</comment>
</data>
</root>
</root>
6 changes: 3 additions & 3 deletions src/NuGet.Core/NuGet.Commands/CommandRunners/PackCommandRunner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ private void InitCommonPackageBuilderProperties(PackageBuilder builder)
builder.MinClientVersion = _packArgs.MinClientVersion;
}

CheckForUnsupportedFrameworks(builder);
CheckForBadFrameworks(builder);

ExcludeFiles(builder.Files);
}
Expand Down Expand Up @@ -839,7 +839,7 @@ private PackageArchiveReader BuildFromProjectFile(string path)
return null;
}

private void CheckForUnsupportedFrameworks(PackageBuilder builder)
private void CheckForBadFrameworks(PackageBuilder builder)
{
foreach (FrameworkAssemblyReference reference in builder.FrameworkReferences)
{
Expand Down Expand Up @@ -988,7 +988,7 @@ private static string ResolvePath(IPackageFile packageFile, string basePath)
private void BuildSymbolsPackage(string path)
{
PackageBuilder symbolsBuilder = CreatePackageBuilderFromNuspec(path);
if (_packArgs.SymbolPackageFormat == SymbolPackageFormat.Snupkg) // Snupkgs can only have 1 PackageType.
if (_packArgs.SymbolPackageFormat == SymbolPackageFormat.Snupkg) // Snupkgs can only have 1 PackageType.
{
symbolsBuilder.PackageTypes.Clear();
symbolsBuilder.PackageTypes.Add(PackageType.SymbolsPackage);
Expand Down
2 changes: 1 addition & 1 deletion src/NuGet.Core/NuGet.Commands/GlobalSuppressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'void PackCommandRunner.AddLibraryDependency(LibraryDependency dependency, ISet<LibraryDependency> list)', validate parameter 'list' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Commands.PackCommandRunner.AddLibraryDependency(NuGet.LibraryModel.LibraryDependency,System.Collections.Generic.ISet{NuGet.LibraryModel.LibraryDependency})")]
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'void PackCommandRunner.AddPackageDependency(PackageDependency dependency, ISet<PackageDependency> set)', validate parameter 'set' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Commands.PackCommandRunner.AddPackageDependency(NuGet.Packaging.Core.PackageDependency,System.Collections.Generic.ISet{NuGet.Packaging.Core.PackageDependency})")]
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'PackageArchiveReader PackCommandRunner.BuildPackage(PackageBuilder builder, string outputPath = null)', validate parameter 'builder' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Commands.PackCommandRunner.BuildPackage(NuGet.Packaging.PackageBuilder,System.String)~NuGet.Packaging.PackageArchiveReader")]
[assembly: SuppressMessage("Build", "CA1822:Member CheckForUnsupportedFrameworks does not access instance data and can be marked as static (Shared in VisualBasic)", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Commands.PackCommandRunner.CheckForUnsupportedFrameworks(NuGet.Packaging.PackageBuilder)")]
[assembly: SuppressMessage("Build", "CA1822:Member CheckForUnsupportedFrameworks does not access instance data and can be marked as static (Shared in VisualBasic)", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Commands.PackCommandRunner.CheckForBadFrameworks(NuGet.Packaging.PackageBuilder)")]
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'string PackCommandRunner.GetInputFile(PackArgs packArgs)', validate parameter 'packArgs' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Commands.PackCommandRunner.GetInputFile(NuGet.Commands.PackArgs)~System.String")]
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'string PackCommandRunner.GetOutputFileName(string packageId, NuGetVersion version, bool isNupkg, bool symbols, SymbolPackageFormat symbolPackageFormat, bool excludeVersion = false)', validate parameter 'version' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Commands.PackCommandRunner.GetOutputFileName(System.String,NuGet.Versioning.NuGetVersion,System.Boolean,System.Boolean,NuGet.Commands.SymbolPackageFormat,System.Boolean)~System.String")]
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'string PackCommandRunner.GetOutputPath(PackageBuilder builder, PackArgs packArgs, bool symbols = false, NuGetVersion nugetVersion = null, string outputDirectory = null, bool isNupkg = true)', validate parameter 'packArgs' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Commands.PackCommandRunner.GetOutputPath(NuGet.Packaging.PackageBuilder,NuGet.Commands.PackArgs,System.Boolean,NuGet.Versioning.NuGetVersion,System.String,System.Boolean)~System.String")]
Expand Down
21 changes: 21 additions & 0 deletions src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,12 @@ public async Task<RestoreResult> ExecuteAsync(CancellationToken token)
_success = false;
}

if (!CheckPlatformVersions())
{
// the errors will be added to the assets file
_success = false;
}

// evaluate packages.lock.json file
var packagesLockFilePath = PackagesLockFileUtilities.GetNuGetLockFilePath(_request.Project);
var isLockFileValid = false;
Expand Down Expand Up @@ -404,6 +410,21 @@ public async Task<RestoreResult> ExecuteAsync(CancellationToken token)
}
}

private bool CheckPlatformVersions()
{
IEnumerable<NuGetFramework> badPlatforms = _request.Project.TargetFrameworks.Select(tfm => tfm.FrameworkName).Where(fw => !string.IsNullOrEmpty(fw.Platform) && (fw.PlatformVersion == FrameworkConstants.EmptyVersion));
if (badPlatforms.Any())
{
NuGetFramework fw = badPlatforms.First();
_logger.Log(RestoreLogMessage.CreateError(NuGetLogCode.NU1012, string.Format(CultureInfo.CurrentCulture, Strings.Error_PlatformVersionNotPresent, fw.Framework, fw.Platform)));
return false;
}
else
{
return true;
}
}

private async Task<bool> AreCentralVersionRequirementsSatisfiedAsync()
{
// The dependencies should not have versions explicitelly defined if cpvm is enabled.
Expand Down
9 changes: 9 additions & 0 deletions src/NuGet.Core/NuGet.Commands/Strings.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading