Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Gate warnings for localization asset locales on TFM >= 7 #28060

Merged
merged 4 commits into from
Sep 21, 2022
+82 −51
Merged

Gate warnings for localization asset locales on TFM >= 7 #28060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f07c1d1
gate warnings on TFM > 7
baronfel Sep 20, 2022
9ff7795
more tests, but they fail due to not resolving things?
baronfel Sep 20, 2022
dc4694a
mock package resolver so we can test
baronfel Sep 21, 2022
ff8d7bf
review notes
baronfel Sep 21, 2022
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
93 changes: 47 additions & 46 deletions src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAResolvePackageAssetsTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using FluentAssertions;
using Microsoft.Build.Framework;
using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
Expand Down Expand Up @@ -117,15 +116,11 @@ public void It_does_not_error_on_duplicate_package_names()
new CacheWriter(task); // Should not error
}

[Fact]
public void It_warns_on_invalid_culture_codes_of_resources()
{
string projectAssetsJsonPath = Path.GetTempFileName();
var assetsContent = @"
private static string AssetsFileWithInvalidLocale(string tfm, string locale) => @"
{
`version`: 3,
`targets`: {
`net7.0`: {
`{tfm}`: {
`JavaScriptEngineSwitcher.Core/3.3.0`: {
`type`: `package`,
`compile`: {
Expand All @@ -136,59 +131,65 @@ public void It_warns_on_invalid_culture_codes_of_resources()
},
`resource`: {
`lib/netstandard2.0/ru-ru/JavaScriptEngineSwitcher.Core.resources.dll`: {
`locale`: `what is this even`
`locale`: `{locale}`
}
}
}
}
},
`project`: {
`version`: `1.0.0`,
`frameworks`: {
`{tfm}`: {
`targetAlias`: `{tfm}`
}
}
}
}".Replace("`", "\"");
}".Replace("`", "\"").Replace("{tfm}", tfm).Replace("{locale}", locale);

[InlineData("net7.0", true)]
[InlineData("net6.0", false)]
[Theory]
public void It_warns_on_invalid_culture_codes_of_resources(string tfm, bool shouldHaveWarnings)
{
string projectAssetsJsonPath = Path.GetTempFileName();
var assetsContent = AssetsFileWithInvalidLocale(tfm, "what is this even");
File.WriteAllText(projectAssetsJsonPath, assetsContent);
var task = InitializeTask(out _);
task.ProjectAssetsFile = projectAssetsJsonPath;
task.TargetFramework = "net7.0";
var writer = new CacheWriter(task);
writer.WriteToCacheFile();
var messages = (task.BuildEngine as MockBuildEngine).Warnings;
var invalidContextMessages = messages.Where(msg => msg.Code == "NETSDK1188");
invalidContextMessages.Should().HaveCount(1);
task.TargetFramework = tfm;
var writer = new CacheWriter(task, new MockPackageResolver());
writer.WriteToMemoryStream();
var engine = task.BuildEngine as MockBuildEngine;

var invalidContextWarnings = engine.Warnings.Where(msg => msg.Code == "NETSDK1188");
invalidContextWarnings.Should().HaveCount(shouldHaveWarnings ? 1 : 0);

var invalidContextMessages = engine.Messages.Where(msg => msg.Code == "NETSDK1188");
invalidContextMessages.Should().HaveCount(shouldHaveWarnings ? 0 : 1);

}

[Fact]
public void It_warns_on_incorrectly_cased_culture_codes_of_resources()

[InlineData("net7.0", true)]
[InlineData("net6.0", false)]
[Theory]
public void It_warns_on_incorrectly_cased_culture_codes_of_resources(string tfm, bool shouldHaveWarnings)
{
string projectAssetsJsonPath = Path.GetTempFileName();
var assetsContent = @"
{
`version`: 3,
`targets`: {
`net7.0`: {
`JavaScriptEngineSwitcher.Core/3.3.0`: {
`type`: `package`,
`compile`: {
`lib/netstandard2.0/JavaScriptEngineSwitcher.Core.dll`: {}
},
`runtime`: {
`lib/netstandard2.0/JavaScriptEngineSwitcher.Core.dll`: {}
},
`resource`: {
`lib/netstandard2.0/ru-ru/JavaScriptEngineSwitcher.Core.resources.dll`: {
`locale`: `ru-ru`
}
}
}
}
}
}".Replace("`", "\"");
var assetsContent = AssetsFileWithInvalidLocale(tfm, "ru-ru");
File.WriteAllText(projectAssetsJsonPath, assetsContent);
var task = InitializeTask(out _);
task.ProjectAssetsFile = projectAssetsJsonPath;
task.TargetFramework = "net7.0";
var writer = new CacheWriter(task);
writer.WriteToCacheFile();
var messages = (task.BuildEngine as MockBuildEngine).Warnings;
var invalidContextMessages = messages.Where(msg => msg.Code == "NETSDK1187");
invalidContextMessages.Should().HaveCount(1);
task.TargetFramework = tfm;
var writer = new CacheWriter(task, new MockPackageResolver());
writer.WriteToMemoryStream();
var engine = task.BuildEngine as MockBuildEngine;

var invalidContextWarnings = engine.Warnings.Where(msg => msg.Code == "NETSDK1187");
invalidContextWarnings.Should().HaveCount(shouldHaveWarnings ? 1 : 0);

var invalidContextMessages = engine.Messages.Where(msg => msg.Code == "NETSDK1187");
invalidContextMessages.Should().HaveCount(shouldHaveWarnings ? 0 : 1);
}

private ResolvePackageAssets InitializeTask(out IEnumerable<PropertyInfo> inputProperties)
Expand Down
5 changes: 4 additions & 1 deletion src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/Mocks/MockPackageResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// 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 NuGet.ProjectModel;
using NuGet.Versioning;
using System.IO;

Expand All @@ -24,5 +25,7 @@ public string GetPackageDirectory(string packageId, NuGetVersion version, out st
packageRoot = _root;
return Path.Combine(_root, packageId, version.ToNormalizedString(), "path");
}

public string ResolvePackageAssetPath(LockFileTargetLibrary package, string relativePath) => Path.Combine(GetPackageDirectory(package.Name, package.Version), relativePath);
}
}
}
2 changes: 2 additions & 0 deletions src/Tasks/Microsoft.NET.Build.Tasks/IPackageResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// 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 NuGet.ProjectModel;
using NuGet.Versioning;

namespace Microsoft.NET.Build.Tasks
Expand All @@ -9,5 +10,6 @@ internal interface IPackageResolver
{
string GetPackageDirectory(string packageId, NuGetVersion version);
string GetPackageDirectory(string packageId, NuGetVersion version, out string packageRoot);
string ResolvePackageAssetPath(LockFileTargetLibrary package, string relativePath);
}
}
33 changes: 29 additions & 4 deletions src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,8 +657,8 @@ internal sealed class CacheWriter : IDisposable
private ResolvePackageAssets _task;
private BinaryWriter _writer;
private LockFile _lockFile;
private NuGetPackageResolver _packageResolver;
private LockFileTarget _compileTimeTarget;
private IPackageResolver _packageResolver;
private LockFileTarget _runtimeTarget;
private Dictionary<string, int> _stringTable;
private List<string> _metadataStrings;
Expand All @@ -677,6 +677,15 @@ internal sealed class CacheWriter : IDisposable

private const char RelatedPropertySeparator = ';';

/// <summary>
/// This constructor should only be used for testing - IPackgeResolver carries a lot of
/// state so using mocks really help with testing this component.
/// </summary>
public CacheWriter(ResolvePackageAssets task, IPackageResolver resolver) : this(task)
{
_packageResolver = resolver;
}

public CacheWriter(ResolvePackageAssets task)
{
_targetFramework = task.TargetFramework;
Expand Down Expand Up @@ -1425,14 +1434,30 @@ private void WriteResourceAssemblies()
var normalizedLocale = System.Globalization.CultureInfo.GetCultureInfo(locale).Name;
if (normalizedLocale != locale)
{
_task.Log.LogWarning(Strings.PackageContainsIncorrectlyCasedLocale, package.Name, package.Version.ToNormalizedString(), locale, normalizedLocale);
var tfm = _lockFile.GetTargetAndThrowIfNotFound(_targetFramework, null).TargetFramework;
if (tfm.Version.Major >= 7)
{
_task.Log.LogWarning(Strings.PackageContainsIncorrectlyCasedLocale, package.Name, package.Version.ToNormalizedString(), locale, normalizedLocale);
}
else
{
_task.Log.LogMessage(Strings.PackageContainsIncorrectlyCasedLocale, package.Name, package.Version.ToNormalizedString(), locale, normalizedLocale);
}
}
locale = normalizedLocale;
}
catch (System.Globalization.CultureNotFoundException cnf)
{
_task.Log.LogWarning(Strings.PackageContainsUnknownLocale, package.Name, package.Version.ToNormalizedString(), cnf.InvalidCultureName);
// We could potentially strip this unknown locales at this point, but we do not.
var tfm = _lockFile.GetTargetAndThrowIfNotFound(_targetFramework, null).TargetFramework;
if (tfm.Version.Major >= 7)
{
_task.Log.LogWarning(Strings.PackageContainsUnknownLocale, package.Name, package.Version.ToNormalizedString(), cnf.InvalidCultureName);
} else
{
_task.Log.LogMessage(Strings.PackageContainsUnknownLocale, package.Name, package.Version.ToNormalizedString(), cnf.InvalidCultureName);
}

// We could potentially strip this unknown locale at this point, but we do not.
// Locale data can change over time (it's typically an OS database that's kept updated),
// and the data on the system running the build may not be the same data as
// the system executing the built code. So we should be permissive for this case.
Expand Down