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.

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

Localize the text in the common target #10388

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

JaynieBai
Copy link
Member

Fixes #10171

Context

MsBuild produces unlocalized messages for some specific validation cases that happen in scope of target.
The example of the task usage: https://github.com/YuliiaKovalova/msbuild/blob/be21253d85f7766356880d376e26aaa69c34c4cd/src/Tasks/Microsoft.Common.CurrentVersion.targets#L862

Changes Made

Refactor the existing messages with the recently added task allows to produce localized messages:
https://github.com/dotnet/msbuild/blob/79dff86b18613cfe3510b719ac28e8a8c3e7f96c/src/Tasks/MSBuildInternalMessage.cs , produced by with hardcoded text.

Testing

Notes

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good.

Though - I was not controllig in detail 1:1 equality of the literals in .targets with the new in resx files - I suppose they were copied over (with just applying format parameters)

Copy link
Contributor

@Nirmal4G Nirmal4G left a comment

Choose a reason for hiding this comment

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

Why not use CommonSdk instead of CommonTarget? It could be useful if you decide to do #1686?

Also, Message Code (like MSB3539) should be separately specified to ensure searchability in these files.

Comment on lines 838 to 841
<MSBuildInternalMessage Condition=" '$(_InvalidConfigurationError)' == 'true' and '$(BuildingInsideVisualStudio)' == 'true'" ResourceName="CommonTarget.InvalidConfigurationTextWithBuildingInsideVisualStudio" Severity="Error" FormatArguments="$(MSBuildProjectFile);$(_OriginalConfiguration);$(_OriginalPlatform)"/>
<MSBuildInternalMessage Condition=" '$(_InvalidConfigurationError)' == 'true' and '$(BuildingInsideVisualStudio)' != 'true'" ResourceName="CommonTarget.InvalidConfigurationTextWithoutBuildingInsideVisualStudio" Severity="Error" FormatArguments="$(MSBuildProjectFile);$(_OriginalConfiguration);$(_OriginalPlatform)"/>
<MSBuildInternalMessage Condition=" '$(_InvalidConfigurationWarning)' == 'true' and '$(BuildingInsideVisualStudio)' == 'true'" ResourceName="CommonTarget.InvalidConfigurationTextWithBuildingInsideVisualStudio" Severity="Warning" FormatArguments="$(MSBuildProjectFile);$(_OriginalConfiguration);$(_OriginalPlatform)"/>
<MSBuildInternalMessage Condition=" '$(_InvalidConfigurationWarning)' == 'true' and '$(BuildingInsideVisualStudio)' != 'true'" ResourceName="CommonTarget.InvalidConfigurationTextWithoutBuildingInsideVisualStudio" Severity="Warning" FormatArguments="$(MSBuildProjectFile);$(_OriginalConfiguration);$(_OriginalPlatform)"/>
Copy link
Contributor

@Nirmal4G Nirmal4G Jul 18, 2024

Choose a reason for hiding this comment

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

Suggested change
<MSBuildInternalMessage Condition=" '$(_InvalidConfigurationError)' == 'true' and '$(BuildingInsideVisualStudio)' == 'true'" ResourceName="CommonTarget.InvalidConfigurationTextWithBuildingInsideVisualStudio" Severity="Error" FormatArguments="$(MSBuildProjectFile);$(_OriginalConfiguration);$(_OriginalPlatform)"/>
<MSBuildInternalMessage Condition=" '$(_InvalidConfigurationError)' == 'true' and '$(BuildingInsideVisualStudio)' != 'true'" ResourceName="CommonTarget.InvalidConfigurationTextWithoutBuildingInsideVisualStudio" Severity="Error" FormatArguments="$(MSBuildProjectFile);$(_OriginalConfiguration);$(_OriginalPlatform)"/>
<MSBuildInternalMessage Condition=" '$(_InvalidConfigurationWarning)' == 'true' and '$(BuildingInsideVisualStudio)' == 'true'" ResourceName="CommonTarget.InvalidConfigurationTextWithBuildingInsideVisualStudio" Severity="Warning" FormatArguments="$(MSBuildProjectFile);$(_OriginalConfiguration);$(_OriginalPlatform)"/>
<MSBuildInternalMessage Condition=" '$(_InvalidConfigurationWarning)' == 'true' and '$(BuildingInsideVisualStudio)' != 'true'" ResourceName="CommonTarget.InvalidConfigurationTextWithoutBuildingInsideVisualStudio" Severity="Warning" FormatArguments="$(MSBuildProjectFile);$(_OriginalConfiguration);$(_OriginalPlatform)"/>
<PropertyGroup>
<_InvalidConfigurationMessageResourceName Condition=" '$(BuildingInsideVisualStudio)' == 'true' ">CommonSdk.InvalidConfigurationTextWhenBuildingInsideVisualStudio</_InvalidConfigurationMessageResourceName>
<_InvalidConfigurationMessageResourceName Condition=" '$(BuildingInsideVisualStudio)' != 'true' ">CommonSdk.InvalidConfigurationTextWhenBuildingOutsideVisualStudio</_InvalidConfigurationMessageResourceName>
</PropertyGroup>
<MSBuildInternalMessage Code="MSB3541"
ResourceName="$(_InvalidConfigurationMessageResourceName)"
Severity="$(_InvalidConfigurationMessageSeverity)"
FormatArguments="$(MSBuildProjectFile);$(_OriginalConfiguration);$(_OriginalPlatform)"
Condition="'$(_InvalidConfigurationMessageSeverity)' != ''" />
<PropertyGroup>
<_InvalidConfigurationMessageSeverity />
<_InvalidConfigurationMessageResourceName />
</PropertyGroup>

Linked Changes

If you are applying this patch, replace the _InvalidConfiguration[Warning|Error] prperties with _InvalidConfigurationMessageSeverity like so:

Replace these lines

<_InvalidConfigurationError Condition=" '$(SkipInvalidConfigurations)' != 'true' ">true</_InvalidConfigurationError>
<_InvalidConfigurationWarning Condition=" '$(SkipInvalidConfigurations)' == 'true' ">true</_InvalidConfigurationWarning>

with these lines

      <_InvalidConfigurationMessageSeverity Condition=" '$(SkipInvalidConfigurations)' == 'true' ">Warning</_InvalidConfigurationMessageSeverity>
      <_InvalidConfigurationMessageSeverity Condition=" '$(SkipInvalidConfigurations)' != 'true' ">Error</_InvalidConfigurationMessageSeverity>

Important

You could use the above pattern everywhere else. It makes the code easy to read. I don't know if cleaning up the properties is needed or not but kept there for clarity.

<comment>{StrBegin="MSB3540: "}</comment>
</data>

<data name="CommonTarget.InvalidConfigurationTextWithBuildingInsideVisualStudio">
Copy link
Contributor

@Nirmal4G Nirmal4G Jul 18, 2024

Choose a reason for hiding this comment

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

Suggested change
<data name="CommonTarget.InvalidConfigurationTextWithBuildingInsideVisualStudio">
<data name="CommonSdk.InvalidConfigurationTextWhenBuildingInsideVisualStudio">

<comment>LOCALIZATION: Do not localize the words "BaseOutputPath/OutputPath", "Configuration" and "Platform"</comment>
</data>

<data name="CommonTarget.InvalidConfigurationTextWithoutBuildingInsideVisualStudio">
Copy link
Contributor

@Nirmal4G Nirmal4G Jul 18, 2024

Choose a reason for hiding this comment

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

Suggested change
<data name="CommonTarget.InvalidConfigurationTextWithoutBuildingInsideVisualStudio">
<data name="CommonSdk.InvalidConfigurationTextWhenBuildingOutsideVisualStudio">

@@ -834,23 +834,20 @@ Copyright (C) Microsoft Corporation. All rights reserved.
<Target
Name="_CheckForInvalidConfigurationAndPlatform"
BeforeTargets="$(BuildDependsOn);Build;$(RebuildDependsOn);Rebuild;$(CleanDependsOn);Clean">

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: undo the line deletion!

</PropertyGroup>

<Error Condition=" '$(_InvalidConfigurationError)' == 'true' " Text="$(_InvalidConfigurationMessageText)"/>
<Warning Condition=" '$(_InvalidConfigurationWarning)' == 'true' " Text="$(_InvalidConfigurationMessageText)"/>
<MSBuildInternalMessage ResourceName="$(_InvalidConfigurationMessageResourceName)" Severity="$(_InvalidConfigurationMessageSeverity)" FormatArguments="$(MSBuildProjectFile);$(_OriginalConfiguration);$(_OriginalPlatform)"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about my mistake.

Suggested change
<MSBuildInternalMessage ResourceName="$(_InvalidConfigurationMessageResourceName)" Severity="$(_InvalidConfigurationMessageSeverity)" FormatArguments="$(MSBuildProjectFile);$(_OriginalConfiguration);$(_OriginalPlatform)"/>
<MSBuildInternalMessage
ResourceName="$(_InvalidConfigurationMessageResourceName)"
Severity="$(_InvalidConfigurationMessageSeverity)"
FormatArguments="$(MSBuildProjectFile);$(_OriginalConfiguration);$(_OriginalPlatform)"
Condition="'$(_InvalidConfigurationMessageSeverity)' != ''" />

Important

For the condition, we could have used the same output path check here also
but we must not forget to update both when it changes in the future.
That's why I have used Severity instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks a lot @Nirmal4G

<MSBuildInternalMessage
Condition="'$(_NonExistentProjectReferenceSecuity)' != ''"
ResourceName="CommonSdk.NonExistentProjectReference"
Severity="$(_NonExistentProjectReferenceSecuity)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Severity="$(_NonExistentProjectReferenceSecuity)"
Severity="$(_NonExistentProjectReferenceSeverity)"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Localize MSBuild messages with MSBuildInternalMessage task
3 participants