Skip to content

Use dotnet strong typed resource builder #12355

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

Closed
Ilya (iSazonov) wants to merge 8 commits into from
Closed

Use dotnet strong typed resource builder #12355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5f12cde
Resgen.proj works
iSazonov Apr 14, 2020
5ea39a8
UI works
iSazonov Apr 14, 2020
88f22bd
Microsoft.Management.UI.Internal works
iSazonov Apr 15, 2020
35df12d
Rest csproj works
iSazonov Apr 17, 2020
94136ad
Remove ReadyToRun
iSazonov Apr 17, 2020
b7742a4
Put generated files in gen folder
iSazonov May 28, 2020
863e9bb
Remove workarround for CimCmdlets project
iSazonov Jun 15, 2020
7e7cf9a
Add link to comment
iSazonov Jun 15, 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
23 changes: 22 additions & 1 deletion PowerShell.Common.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@

<TargetFramework>net5.0</TargetFramework>
<LangVersion>8.0</LangVersion>
<PublishReadyToRun>true</PublishReadyToRun>
<PublishReadyToRun Condition=" '$(Configuration)' != 'Debug' ">true</PublishReadyToRun>

<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
Expand All @@ -111,6 +111,27 @@
<HighEntropyVA>true</HighEntropyVA>
</PropertyGroup>

<PropertyGroup>
<StronglyTypedOutputFolder>gen</StronglyTypedOutputFolder>
</PropertyGroup>

<Target Name = "CreateDirectories" BeforeTargets = "BeforeResGen">
<MakeDir
Directories = "$(StronglyTypedOutputFolder)"/>
</Target>

<ItemGroup>
<!-- We skip this for Microsoft.Management.UI.Internal because we have to use a workaround for WPF project. See https://github.com/microsoft/msbuild/issues/4751 -->
<EmbeddedResource Update = "resources/*.resx" Condition = "'$(MSBuildProjectName)' != 'Microsoft.Management.UI.Internal'">
Comment thread
daxian-dbw marked this conversation as resolved.
Outdated

@TravisEz13 Travis Plunk (TravisEz13) Jun 9, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

comment that this is a workaround for dotnet/msbuild#4751

@iSazonov Ilya (iSazonov) Jun 15, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

<Generator>ResXFileCodeGenerator</Generator>
<StronglyTypedLanguage>CSharp</StronglyTypedLanguage>

<StronglyTypedFileName>$(StronglyTypedOutputFolder)/%(Filename).cs</StronglyTypedFileName>
<StronglyTypedClassName>%(Filename)</StronglyTypedClassName>
<StronglyTypedManifestPrefix>$(RootNamespace).resources</StronglyTypedManifestPrefix>
</EmbeddedResource>
</ItemGroup>

<PropertyGroup>
<DefineConstants>$(DefineConstants);CORECLR</DefineConstants>
<IsWindows Condition="'$(IsWindows)' =='true' or ( '$(IsWindows)' == '' and '$(OS)' == 'Windows_NT')">true</IsWindows>
Expand Down
8 changes: 4 additions & 4 deletions build.psm1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,10 +430,10 @@ Fix steps:

# handle ResGen
# Heuristic to run ResGen on the fresh machine
if ($ResGen -or -not (Test-Path "$PSScriptRoot/src/Microsoft.PowerShell.ConsoleHost/gen")) {
Write-Log -message "Run ResGen (generating C# bindings for resx files)"
Start-ResGen
}
#if ($ResGen -or -not (Test-Path "$PSScriptRoot/src/Microsoft.PowerShell.ConsoleHost/gen")) {

@TravisEz13 Travis Plunk (TravisEz13) Apr 21, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should do more clean-up or file an issue to do it

@iSazonov Ilya (iSazonov) Apr 21, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, after MSFT team confirm that the PR is good I will clean up all scripts.

# Write-Log -message "Run ResGen (generating C# bindings for resx files)"
# Start-ResGen
#}

# Handle TypeGen
# .inc file name must be different for Windows and Linux to allow build on Windows and WSL.
Expand Down
62 changes: 61 additions & 1 deletion src/Microsoft.Management.UI.Internal/Microsoft.PowerShell.GraphicalHost.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk.WindowsDesktop" ToolsVersion="15.0">
<Project Sdk="Microsoft.NET.Sdk.WindowsDesktop" ToolsVersion="15.0" InitialTargets = "PSResources">
<Import Project="..\..\PowerShell.Common.props" />
<PropertyGroup>
<Description>Assembly containing WPF code for Out-GridView, HelpWindow, and Show-Command</Description>
Expand All @@ -12,6 +12,66 @@
<DefineConstants>$(DefineConstants);CORECLR</DefineConstants>
</PropertyGroup>

<PropertyGroup>
<StronglyTypedOutputFolder>gen</StronglyTypedOutputFolder>
<SkipWPFcsproj>$([System.String]::Copy('$(MSBuildProjectFile)').Contains('wpftmp'))</SkipWPFcsproj>
</PropertyGroup>

<Target Name = "CreateDirectories" BeforeTargets = "PSResources">
<MakeDir
Directories = "$(StronglyTypedOutputFolder)"/>
</Target>

<ItemGroup>
<!-- We have to use the workaround for the WPF project.
.Net 5 Preview.2 has some issues with WPF projects:
- Strong typed resources does not work as expected out of box.
(Really they are not generated at all.)
So we assign items in 'PSEmbeddedResource' to expected format
and explicitly call 'GenerateResource' task from custom 'PSResources' target.
- .Net 5 Preview.2 generates a temporory csproj file
that duplicates call our custom 'PSResources' target.
To avoid this we skip a project with 'wpftmp' in name.

@daxian-dbw Dongbo Wang (daxian-dbw) May 11, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ilya (@iSazonov) Thanks for this work! I have 3 concerns:

  1. The strongly typed .cs files are not generated during the build, and thus reference to the resource strings like ExtendedTypeSystem.InvalidCastExceptionWithInnerException are all shown as errors in VS Code. It's likely the same in Visual Studio, but I didn't try.
  2. Is this workaround still needed? We have moved to .NET 5 preview.4.
  3. Question: is this really more maintainable than using ResGen as before? Adding targets to .csproj always scares me. What are the benefits moving to ResXFileCodeGenerator?

@iSazonov Ilya (iSazonov) May 12, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Resx files are converted to cs files well for me when I run Start-PSBuild. Or I don't understand your question?

I see no activity in MSBuild and SDK. They use tricky way to compile WPF but I don't think they are going to change this now.

It seems strong typed resources are rarely used in while and in this sense it is less maintainable.

Adding targets to .csproj always scares me.

I guess you are thinking about daily enhancements. In fact, our compile workflow is stable for years. So why not solve the following:

  • Localization Localization story #666
    I expect we get an increase in the number of users in 10x or 100x in next dev cycle. And user will ask about localization. This change is one step to MSFT team to make this process public and save their resources.
  • Allow build that does not require circular dependency. #5777
    The change is one step to removing dependency on PowerShell to compile the project that helps third-party distributers.
    We have still to run TypeGen first. But it is a stable too and we could move the step to csproj too. As result (1) third-party distributers will be able to compile PowerShell without having to install PowerShell, (2) we will be able to compile in VS Code and VS without Start-PSBuild.

@daxian-dbw Dongbo Wang (daxian-dbw) May 12, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Resx files are converted to cs files well for me when I run Start-PSBuild. Or I don't understand your question?

I checked out the resgen-msbuild-3 branch, did a clean build using Start-PSBuild -Clean with dotnet SDK 5.0.100-preview.3.20216.6.
After that, I couldn't find the .cs files generated from the resource files. The only gen folder I could find is src\Microsoft.Management.UI.Internal\gen.

The following screenshot is what I get after opening src\System.Management.Automation folder in VS Code. A lot of errors because the property references to resources don't work anymore.

image

It seems strong typed resources are rarely used in while and in this sense it is less maintainable.

Just so I'm clear, you agree that the ResGen we use before this PR is more maintainable, right?

I guess you are thinking about daily enhancements.

I'm not thinking about enhancement, but purely maintenance, e.g. something changed (either dotnet sdk or the PS project itself) and we have to change this target to make it continue to work ...

So why not solve the following:

It's not clear to me how this will help solving those 2 issues,

@iSazonov Ilya (iSazonov) May 12, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

After that, I couldn't find the .cs files generated from the resource files. The only gen folder I could find is src\Microsoft.Management.UI.Internal\gen.

The files are generated in obj\ folder where *.resource are. If Start-PSBuild success you have the files.
VS Code works well for me too (after restart?).

Just so I'm clear, you agree that the ResGen we use before this PR is more maintainable, right?
I'm not thinking about enhancement, but purely maintenance

ResGen tool is not changed for years. We don't any maintenance.
.Net team is porting MSBuild step-by-step and it is not high priority for them. But if they did I think they will avoid breaking changes.

It's not clear to me how this will help solving those 2 issues,

Currently we have to use Build.psm1 module to build PowerShell. Third-party distribution maintainers have to port the module to bash shell to build PowerShell without PowerShell. If we change the module we break their process, they have to port again. If all build logic is in csproj files they get updates automatically.
With the change they should have a workaround only for TypeGen - resource generation now works with dotnet build. Additional benefits - now if we change any resx file it will be automatically compiled without needs to run Start-PSBuild -ResGen. We could make the same for TypeGen too.

@daxian-dbw Dongbo Wang (daxian-dbw) May 12, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The files are generated in obj\ folder where *.resource are. If Start-PSBuild success you have the files.
VS Code works well for me too (after restart?).

Found the .cs files under obj\Debug\netcoreapp5.0, but opening in VS Code still doesn't work -- a lot errors due to the resource string references. Travis Plunk (@TravisEz13) do you see the same issue?

Currently we have to use Build.psm1 module to build PowerShell.
Additional benefits - now if we change any resx file it will be automatically compiled without needs to run Start-PSBuild -ResGen.

ResGen doesn't depend on PowerShell. Anyone wants to build without PowerShell can do it as described in our doc: https://github.com/PowerShell/PowerShell/blob/master/docs/building/internals.md#resgen

I agree that's a benefit, but personally, I don't think it's convincing ... I personally don't want to incorporate everything into msbuild, as I personally believe a script is more maintainable than black magic in msbuild 😄

@iSazonov Ilya (iSazonov) May 14, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It seems I can repo. It seems it is OmniSharp issue. After I restart VS Code I see the reds. Then I run Start-PSBuild and the reds goes away.

@daxian-dbw Dongbo Wang (daxian-dbw) May 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I did run Start-PSBuild -Clean before starting VS Code. Also, we need to make sure opening VS Code with each .csproj folder should work too, for example, opening VS Code with the src/System.Management.Automation folder.

@iSazonov Ilya (iSazonov) May 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I see the same. Open SMA folder - I see the reds. Run dotnet build from the folder - the reds goes away.
It is OmniSharp. I can open new issue there. dotnet/vscode-csharp#3781

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) May 14, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ilya (@iSazonov) are you running from Start-PSBuild -Clean? I'm not seeing OmniSharp pick up any of the generated classes after a clean build. 😕

@iSazonov Ilya (iSazonov) May 14, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

After switching to the branch I run Start-PSBuild -Clean, then run VS Code. After that I run Start-PSBuild and select a file.

-->
<PSEmbeddedResource Include = "resources/*.resx" Condition = "'$(SkipWPFcsproj)' == 'false'">
<Generator>ResXFileCodeGenerator</Generator>
<StronglyTypedLanguage>CSharp</StronglyTypedLanguage>

<PublicClass>$([System.String]::new('%(Filename)').StartsWith('Public', System.StringComparison.OrdinalIgnoreCase))</PublicClass>

<StronglyTypedManifestPrefix>$(RootNamespace).resources</StronglyTypedManifestPrefix>
<StronglyTypedManifestPrefix Condition = "'%(PublicClass)' == 'true'">$(RootNamespace).resources.public</StronglyTypedManifestPrefix>

<StronglyTypedFileName>$(StronglyTypedOutputFolder)/%(Filename).cs</StronglyTypedFileName>
<StronglyTypedFileName Condition = "'%(PublicClass)' == 'true'">$(StronglyTypedOutputFolder)/$([System.String]::Copy('%(Filename)').Substring(7)).cs</StronglyTypedFileName>

<StronglyTypedClassName>%(Filename)</StronglyTypedClassName>
<StronglyTypedClassName Condition = "'%(PublicClass)' == 'true'">$([System.String]::Copy('%(Filename)').Substring(7))</StronglyTypedClassName>

<ManifestResourceName>$(RootNamespace).resources.%(PSEmbeddedResource.StronglyTypedClassName)</ManifestResourceName>
<ManifestResourceName Condition = "'%(PublicClass)' == 'true'">$(RootNamespace).resources.public.%(PSEmbeddedResource.StronglyTypedClassName)</ManifestResourceName>
</PSEmbeddedResource>
</ItemGroup>

<Target Name="PSResources" Condition = "'$(SkipWPFcsproj)' == 'false'">

@JoeRobich Joey Robichaud (JoeRobich) May 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe you should be declaring your Target's Inputs and Outputs, similar to Arcade's resource generator.

https://github.com/dotnet/arcade/blob/d6baec6b932970d1ef47238db8288b9da6e2c9ff/src/Microsoft.DotNet.Arcade.Sdk/tools/GenerateResxSource.targets#L45-L59

<GenerateResource
Sources="@(PSEmbeddedResource)"
UseSourcePath="true"
StateFile="$(StronglyTypedOutputFolder)/$(MSBuildProjectFile).GenerateResource.cache"
StronglyTypedClassName="%(PSEmbeddedResource.StronglyTypedClassName)"
StronglyTypedFileName="%(PSEmbeddedResource.StronglyTypedFileName)"
StronglyTypedLanguage="%(PSEmbeddedResource.StronglyTypedLanguage)"
StronglyTypedNamespace="%(PSEmbeddedResource.StronglyTypedNamespace)"
StronglyTypedManifestPrefix="%(PSEmbeddedResource.StronglyTypedManifestPrefix)"
PublicClass="%(PSEmbeddedResource.PublicClass)"
OutputResources="@(PSEmbeddedResource->'$(StronglyTypedOutputFolder)/%(ManifestResourceName).resources')"
ExecuteAsTool="false"
MSBuildRuntime="$(GenerateResourceMSBuildRuntime)"
MSBuildArchitecture="$(GenerateResourceMSBuildArchitecture)">

</GenerateResource>
</Target>

<ItemGroup>
<Resource Include="resources\Graphics\Add16.png" />
<Resource Include="resources\Graphics\CloseTile16.png" />
Expand Down