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

Detect invalid package locales and warn the user #26729

Merged
merged 5 commits into from
Jul 27, 2022

Conversation

baronfel
Copy link
Member

@baronfel baronfel commented Jul 20, 2022

Fixes #26711

There's a long discussion but the gist is that we can detect situations where locale codes are mismatched and tell the user about it. We can even correct those that can be normalized via the BCL culture lookups.

Open questions:

  • what should we do when an invalid culture is found? We have two options I see immediately: include it with a warning, or exclude it. Is there a use case for invalid culture identifiers?
    • decided to allow invalid locale identifiers - ICU data can change, and may not be consistent between build machine and execution machine.
  • what code should we choose?
    • took the next two open numbers
  • need to think about the messages a bit more - is the packageName@packageId format useful?
    • spoke with Jon Douglas and got the normal nuget output format for package names and versions.

@baronfel
Copy link
Member Author

I'm only marking this for review to see if the tests run - there are more open questions

@baronfel baronfel marked this pull request as ready for review July 20, 2022 20:56
@baronfel
Copy link
Member Author

Alright - there's only one major question we should answer - what do we do with assets that are found using completely unknown locale identifiers? We can drop them (currently implemented) or include them. Should we be forwards-compatible for locale identifiers? How often does this data change?

@baronfel
Copy link
Member Author

Country codes change based on an IANA registry: https://www.iana.org/assignments/language-subtag-registry/language-subtag-registry

@baronfel baronfel requested a review from a team July 23, 2022 16:28
@baronfel baronfel added this to the 7.0.1xx milestone Jul 23, 2022
@baronfel baronfel self-assigned this Jul 23, 2022
Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

Overall looks good. Please make sure to do some end to end testing before merging (Ie building projects that reference packages that we know have non-normalized locales, as well as packages that have correct locales).

@baronfel baronfel force-pushed the detect-invalid-package-locales branch 2 times, most recently from 400e4f7 to 59b5991 Compare July 24, 2022 23:56
@baronfel baronfel force-pushed the detect-invalid-package-locales branch from 59b5991 to cbcd33d Compare July 25, 2022 00:00
@baronfel
Copy link
Member Author

Manual testing shows it working as expected:

C:\Users\chethusk\OSS\dotnet-sdk\artifacts\bin\redist\Debug\dotnet\sdk\7.0.100-dev\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(267,5): warning NETSDK1187: Package JavaScriptEngineSwitcher.Core 3.3.0 has a resource with the locale 'ru-ru'. This locale has been normalized to the standard format 'ru-RU' to prevent casing issues in the build. Consider notifying the package author about this casing issue. [C:\Users\chethusk\OSS\dotnet-sdk\blah\blah.csproj]

@baronfel
Copy link
Member Author

One thing I'd like to call out - we've been fairly conservative about introducing new warnings/errors purely on updating the SDK. It typically requires updating a Warning or Analysis level (which by default is tied to TFM), which is a user-driven action. That falls over a bit here because this bug can impact anyone on any TFM, but I just wanted to call out that we are breaking our own rule a bit here.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could not copy Error in docker linux container
2 participants