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

net5.0-platform support for Pack #3479

Merged
merged 3 commits into from
Jul 10, 2020
Merged

net5.0-platform support for Pack #3479

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e1356b
add support for net5.0-<platform>
zkat Jul 3, 2020
a509f5d
PR feedback
zkat Jul 8, 2020
5ab918a
make it internal again
zkat Jul 9, 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
20 changes: 15 additions & 5 deletions src/NuGet.Clients/NuGet.CommandLine/Commands/ProjectFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ private void Initialize(dynamic project)
string targetFrameworkMoniker = _project.GetPropertyValue("TargetFrameworkMoniker");
if (!String.IsNullOrEmpty(targetFrameworkMoniker))
{
TargetFramework = new FrameworkName(targetFrameworkMoniker);
TargetFramework = NuGetFramework.Parse(targetFrameworkMoniker);
}

// This happens before we obtain warning properties, so this Logger is still IConsole.
Expand Down Expand Up @@ -170,7 +170,7 @@ private string TargetPath
set;
}

private FrameworkName TargetFramework
private NuGetFramework TargetFramework
{
get;
set;
Expand Down Expand Up @@ -804,7 +804,7 @@ private void AddOutputFiles(Packaging.PackageBuilder builder)
}
else
{
nugetFramework = TargetFramework != null ? NuGetFramework.Parse(TargetFramework.FullName) : null;
nugetFramework = TargetFramework;
}

var projectOutputDirectory = Path.GetDirectoryName(TargetPath);
Expand Down Expand Up @@ -1001,7 +1001,7 @@ private void AddDependencies(Dictionary<String, Tuple<PackageReaderBase, Packagi

if (!props.ContainsKey(NuGetProjectMetadataKeys.TargetFramework))
{
props.Add(NuGetProjectMetadataKeys.TargetFramework, new NuGetFramework(TargetFramework.Identifier, TargetFramework.Version, TargetFramework.Profile));
props.Add(NuGetProjectMetadataKeys.TargetFramework, TargetFramework);
}
if (!props.ContainsKey(NuGetProjectMetadataKeys.Name))
{
Expand Down Expand Up @@ -1421,7 +1421,11 @@ public ReverseTransformFormFile(Packaging.IPackageFile file, IEnumerable<Packagi
{
Path = file.Path + ".transform";
_streamFactory = new Lazy<Func<Stream>>(() => ReverseTransform(file, transforms), isThreadSafe: false);
TargetFramework = FrameworkNameUtility.ParseFrameworkNameFromFilePath(Path, out _effectivePath);
NuGetFramework = NuGet.Packaging.FrameworkNameUtility.ParseNuGetFrameworkFromFilePath(Path, out _effectivePath);
if (NuGetFramework != null && NuGetFramework.Version.Major < 5)
nkolev92 marked this conversation as resolved.
Show resolved Hide resolved
{
TargetFramework = new FrameworkName(NuGetFramework.DotNetFrameworkName);
}
}

public string Path
Expand Down Expand Up @@ -1484,6 +1488,12 @@ public FrameworkName TargetFramework
get;
private set;
}

public NuGetFramework NuGetFramework
{
get;
private set;
}
}
}
}
18 changes: 9 additions & 9 deletions src/NuGet.Core/NuGet.Frameworks/CompatibilityProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,18 +181,18 @@ private bool IsCompatibleWithTarget(NuGetFramework target, NuGetFramework candid

private static bool IsCompatibleWithTargetCore(NuGetFramework target, NuGetFramework candidate)
{
if (target.IsNet5Era)
{
return NuGetFramework.FrameworkNameComparer.Equals(target, candidate)
&& IsVersionCompatible(target.Version, candidate.Version)
&& (!candidate.HasProfile || StringComparer.OrdinalIgnoreCase.Equals(target.Profile, candidate.Profile));
}
else
{
return NuGetFramework.FrameworkNameComparer.Equals(target, candidate)
bool result = NuGetFramework.FrameworkNameComparer.Equals(target, candidate)
&& IsVersionCompatible(target.Version, candidate.Version)
&& StringComparer.OrdinalIgnoreCase.Equals(target.Profile, candidate.Profile);

if (target.IsNet5Era && candidate.HasPlatform)
{
result = result
&& StringComparer.OrdinalIgnoreCase.Equals(target.Platform, candidate.Platform)
&& IsVersionCompatible(target.PlatformVersion, candidate.PlatformVersion);
}

return result;
}

private static bool IsVersionCompatible(Version target, Version candidate)
Expand Down
7 changes: 0 additions & 7 deletions src/NuGet.Core/NuGet.Frameworks/FrameworkConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,6 @@ public static class PlatformIdentifiers
public const string Windows = "Windows";
}

/// <summary>
/// Allowed list of profiles in Net5.0ERA
/// </summary>
internal static HashSet<string> FrameworkProfiles
= new HashSet<string>(StringComparer.OrdinalIgnoreCase)
{ };

public static class FrameworkIdentifiers
{
public const string NetCoreApp = ".NETCoreApp";
Expand Down
14 changes: 14 additions & 0 deletions src/NuGet.Core/NuGet.Frameworks/FrameworkNameProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,20 @@ public bool TryGetShortProfile(string frameworkIdentifier, string profile, out s
return TryConvertOrNormalize(profile, _profilesToShortName, _profileShortToLong, out profileShortName);
}

public bool TryGetShortPlatform(string frameworkIdentifier, Version frameworkVersion, string platformIdentifier, out string platformShortName)
nkolev92 marked this conversation as resolved.
Show resolved Hide resolved
zivkan marked this conversation as resolved.
Show resolved Hide resolved
{
if (StringComparer.OrdinalIgnoreCase.Equals(frameworkIdentifier, FrameworkConstants.FrameworkIdentifiers.NetCoreApp) && frameworkVersion.Major >= 5)
{
platformShortName = platformIdentifier.ToLowerInvariant();
return true;
}
else
{
platformShortName = null;
return false;
}
}

public bool TryGetVersion(string versionString, out Version version)
{
version = null;
Expand Down
1 change: 1 addition & 0 deletions src/NuGet.Core/NuGet.Frameworks/GlobalSuppressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,4 @@
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'bool NuGetFrameworkUtility.IsNetCore50AndUp(NuGetFramework framework)', validate parameter 'framework' 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.Frameworks.NuGetFrameworkUtility.IsNetCore50AndUp(NuGet.Frameworks.NuGetFramework)~System.Boolean")]
[assembly: SuppressMessage("Build", "CA2237:Add [Serializable] to FrameworkException as this type implements ISerializable", Justification = "<Pending>", Scope = "type", Target = "~T:NuGet.Frameworks.FrameworkException")]
[assembly: SuppressMessage("Build", "CA1067:Type NuGet.Frameworks.OneWayCompatibilityMappingEntry should override Equals because it implements IEquatable<T>", Justification = "<Pending>", Scope = "type", Target = "~T:NuGet.Frameworks.OneWayCompatibilityMappingEntry")]
[assembly: SuppressMessage("Globalization", "CA1308:Normalize strings to uppercase", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Frameworks.FrameworkNameProvider.TryGetShortPlatform(System.String,System.Version,System.String,System.String@)~System.Boolean")]
Copy link
Member

Choose a reason for hiding this comment

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

nit: Add a justification.

107 changes: 97 additions & 10 deletions src/NuGet.Core/NuGet.Frameworks/NuGetFramework.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,16 @@ public NuGetFramework(string framework)
}

public NuGetFramework(string framework, Version version)
: this(framework, version, null)
: this(framework, version, string.Empty, FrameworkConstants.EmptyVersion)
{
}

private const int Version5 = 5;

public NuGetFramework(string frameworkIdentifier, Version frameworkVersion, string frameworkProfile)
/// <summary>
/// Creates a new NuGetFramework instance, with an optional profile (only available for netframework)
/// </summary>
public NuGetFramework(string frameworkIdentifier, Version frameworkVersion, string profile)
{
if (frameworkIdentifier == null)
{
Expand All @@ -57,8 +60,47 @@ public NuGetFramework(string frameworkIdentifier, Version frameworkVersion, stri

_frameworkIdentifier = frameworkIdentifier;
_frameworkVersion = NormalizeVersion(frameworkVersion);
_frameworkProfile = frameworkProfile ?? string.Empty;

IsNet5Era = (_frameworkVersion.Major >= Version5 && StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetCoreApp, _frameworkIdentifier));

_frameworkProfile = profile ?? string.Empty;
Platform = string.Empty;
PlatformVersion = FrameworkConstants.EmptyVersion;
}

/// <summary>
/// Creates a new NuGetFramework instance, with an optional platform and platformVersion (only available for net5.0+)
/// </summary>
public NuGetFramework(string frameworkIdentifier, Version frameworkVersion, string platform, Version platformVersion)
Copy link
Member

Choose a reason for hiding this comment

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

Same feedback as above, just confirm that platform and version are still the correct concept names.

{
if (frameworkIdentifier == null)
{
throw new ArgumentNullException("frameworkIdentifier");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new ArgumentNullException("frameworkIdentifier");
throw new ArgumentNullException(nameof(frameworkIdentifier));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was intentional, in order to keep it consistent with the existing API. Are you sure I should change it? I thought we had this discussion before. I'm happy to make this change, though, if you want.

Copy link
Member

Choose a reason for hiding this comment

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

I remember a discussion where someone was using nameof for strings that are compared to command line arguments customers pass in. I argued that it shouldn't use nameof because renaming the variable in code should not break functional behaviour. I lost that argument.

But for validating method parameters, I'd expect it to match the parameter name. If I use NuGet.Frameworks in my app, and I get an argument null exception saying that parameter x should not be null, but none of the method parameters are named x, then it's confusing.

You're right that the exception message will change between package versions if we change the parameter name, so it's a tradeoff of making the error message be consistent with previous versions, or be consistent with the current code. In either case we'll break anyone using named parameters, so it's ideal to avoid renaming parameters if at all possible.

overall, it's more of a nitpick, so I don't care very much. The mandatory/optional TPV is a much more important issue.

}

if (frameworkVersion == null)
{
throw new ArgumentNullException("frameworkVersion");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new ArgumentNullException("frameworkVersion");
throw new ArgumentNullException(nameof(frameworkVersion));

}

if (platform == null)
{
throw new ArgumentNullException("platform");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new ArgumentNullException("platform");
throw new ArgumentNullException(nameof(platform));

}

if (platformVersion == null)
{
throw new ArgumentNullException("platformVersion");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new ArgumentNullException("platformVersion");
throw new ArgumentNullException(nameof(platformVersion));

}

_frameworkIdentifier = frameworkIdentifier;
_frameworkVersion = NormalizeVersion(frameworkVersion);
_frameworkProfile = string.Empty;

IsNet5Era = (_frameworkVersion.Major >= Version5 && StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetCoreApp, _frameworkIdentifier));

Platform = IsNet5Era ? platform : string.Empty;
PlatformVersion = IsNet5Era ? NormalizeVersion(platformVersion) : FrameworkConstants.EmptyVersion;
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 verify that PlatformVersion is not EmptyVersion when Platform is not string.Empty? Only projects are allowed to omit platform version, but that means that it uses the SDK's default platform version, not that platform version is empty.

Also, should we verify that PlatformVersion is EmptyVersion when Platform is string.Empty? It doesn't make sense to not have a platform, but still to have a platform version.

}

/// <summary>
Expand All @@ -77,6 +119,32 @@ public Version Version
get { return _frameworkVersion; }
}

/// <summary>
/// Framework Platform (net5.0+)
/// </summary>
public string Platform
{
get;
private set;
}

/// <summary>
/// Framework Platform Version (net5.0+)
/// </summary>
public Version PlatformVersion
{
get;
private set;
}

/// <summary>
/// True if the platform is non-empty
/// </summary>
public bool HasPlatform
{
get { return !string.IsNullOrEmpty(Platform); }
}

/// <summary>
/// True if the profile is non-empty
/// </summary>
Expand Down Expand Up @@ -112,9 +180,11 @@ public string GetDotNetFrameworkName(IFrameworkNameProvider mappings)
// Check for rewrites
var framework = mappings.GetFullNameReplacement(this);

var result = string.Empty;

if (framework.IsSpecificFramework)
if (framework.IsNet5Era)
{
return GetShortFolderName();
}
else if (framework.IsSpecificFramework)
{
var parts = new List<string>(3) { Framework };

Expand All @@ -125,14 +195,12 @@ public string GetDotNetFrameworkName(IFrameworkNameProvider mappings)
parts.Add(string.Format(CultureInfo.InvariantCulture, "Profile={0}", framework.Profile));
}

result = string.Join(",", parts);
return string.Join(",", parts);
}
else
{
result = string.Format(CultureInfo.InvariantCulture, "{0},Version=v0.0", framework.Framework);
return string.Format(CultureInfo.InvariantCulture, "{0},Version=v0.0", framework.Framework);
}

return result;
}

/// <summary>
Expand Down Expand Up @@ -218,6 +286,25 @@ public virtual string GetShortFolderName(IFrameworkNameProvider mappings)
framework.DotNetFrameworkName));
}
}
else if (IsNet5Era)
{
var platform = string.Empty;
if (!string.IsNullOrEmpty(framework.Platform) && !mappings.TryGetShortPlatform(framework.Framework, framework.Version, framework.Platform, out platform))
{
platform = framework.Platform;
}

if (!string.IsNullOrEmpty(platform))
{
sb.Append("-");
sb.Append(platform);

if (framework.PlatformVersion != FrameworkConstants.EmptyVersion)
{
sb.Append(mappings.GetVersionString(framework.Framework, framework.PlatformVersion));
}
}
}
else
{
// add the profile
Expand Down
41 changes: 31 additions & 10 deletions src/NuGet.Core/NuGet.Frameworks/NuGetFrameworkFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,18 +92,19 @@ public static NuGetFramework ParseFrameworkName(string frameworkName, IFramework
// if the first part is a special framework, ignore the rest
if (!TryParseSpecialFramework(parts[0], out result))
{
string platform = null;
if (!mappings.TryGetIdentifier(parts[0], out platform))
string framework = null;
if (!mappings.TryGetIdentifier(parts[0], out framework))
{
platform = parts[0];
framework = parts[0];
}

var version = new Version(0, 0);
string profile = null;
string platform = null;
var platformVersion = new Version(0, 0);

var versionPart = SingleOrDefaultSafe(parts.Where(s => s.IndexOf("Version=", StringComparison.OrdinalIgnoreCase) == 0));
var profilePart = SingleOrDefaultSafe(parts.Where(s => s.IndexOf("Profile=", StringComparison.OrdinalIgnoreCase) == 0));

if (!string.IsNullOrEmpty(versionPart))
{
var versionString = versionPart.Split('=')[1].TrimStart('v');
Expand All @@ -127,7 +128,7 @@ public static NuGetFramework ParseFrameworkName(string frameworkName, IFramework
profile = profilePart.Split('=')[1];
}

if (StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.Portable, platform)
if (StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.Portable, framework)
&& !string.IsNullOrEmpty(profile)
&& profile.Contains("-"))
{
Expand All @@ -139,7 +140,15 @@ public static NuGetFramework ParseFrameworkName(string frameworkName, IFramework
profile));
}

result = new NuGetFramework(platform, version, profile);
if (version.Major >= 5
&& StringComparer.OrdinalIgnoreCase.Equals(FrameworkConstants.FrameworkIdentifiers.NetCoreApp, framework))
{
result = new NuGetFramework(framework, version, platform ?? string.Empty, platformVersion ?? FrameworkConstants.EmptyVersion);
zkat marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
result = new NuGetFramework(framework, version, profile);
}
}

return result;
Expand Down Expand Up @@ -204,10 +213,22 @@ public static NuGetFramework ParseFolder(string folderName, IFrameworkNameProvid
framework = FrameworkConstants.FrameworkIdentifiers.NetCoreApp;
if (!string.IsNullOrEmpty(profileShort))
{
bool validProfile = FrameworkConstants.FrameworkProfiles.Contains(profileShort);
if (validProfile)
// Find a platform version if it exists and yank it out
var platformChars = profileShort;
var versionStart = 0;
while (versionStart < platformChars.Length
&& IsLetterOrDot(platformChars[versionStart]))
{
versionStart++;
}
string platform = versionStart > 0 ? profileShort.Substring(0, versionStart) : profileShort;
string platformVersionString = versionStart > 0 ? profileShort.Substring(versionStart, profileShort.Length - versionStart) : null;

// Parse the version if it's there.
Version platformVersion = FrameworkConstants.EmptyVersion;
if ((string.IsNullOrEmpty(platformVersionString) || mappings.TryGetVersion(platformVersionString, out platformVersion)))
{
result = new NuGetFramework(framework, version, profileShort.ToLower());
result = new NuGetFramework(framework, version, platform ?? string.Empty, platformVersion ?? FrameworkConstants.EmptyVersion);
}
else
{
Expand All @@ -216,7 +237,7 @@ public static NuGetFramework ParseFolder(string folderName, IFrameworkNameProvid
}
else
{
result = new NuGetFramework(framework, version, string.Empty);
result = new NuGetFramework(framework, version, string.Empty, FrameworkConstants.EmptyVersion);
}
}
else
Expand Down
2 changes: 2 additions & 0 deletions src/NuGet.Core/NuGet.Frameworks/comparers/NuGetFrameworkFullComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ public bool Equals(NuGetFramework x, NuGetFramework y)
return x.Version == y.Version
&& StringComparer.OrdinalIgnoreCase.Equals(x.Framework, y.Framework)
&& StringComparer.OrdinalIgnoreCase.Equals(x.Profile, y.Profile)
&& StringComparer.OrdinalIgnoreCase.Equals(x.Platform, y.Platform)
&& x.PlatformVersion == y.PlatformVersion
&& !x.IsUnsupported;
}

Expand Down
5 changes: 5 additions & 0 deletions src/NuGet.Core/NuGet.Frameworks/def/IFrameworkNameProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ interface IFrameworkNameProvider
/// </summary>
bool TryGetShortProfile(string frameworkIdentifier, string profile, out string profileShortName);

/// <summary>
/// Get the official platform name from the short name.
/// </summary>
bool TryGetShortPlatform(string frameworkIdentifier, Version version, string platform, out string platformShortName);
Copy link
Member

Choose a reason for hiding this comment

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

platformVersion? My understanding is that it's only optional when platform is not specified (string.Empty). Although as I wrote in another comment, I don't understand why customers would use this API rather than NuGetFramework.ToShortFolderName().


/// <summary>
/// Parses a version string using single digit rules if no dots exist
/// </summary>
Expand Down
Loading