Change resource compilation process to match PowerShell Core #916

JamesWTruher · 2018-03-02T21:08:35Z

PR Summary

Follow the pattern from PowerShell Core with regard to resources. Resources for an assembly go into a resources directory and use the same tool as PowerShell Core for creating the strongly typed resource class.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
User facing documentation needed
[] Change is not breaking
Make sure you've added a new test if existing tests do not effectively test the code changed
This PR is ready to merge and is not work in progress
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

more details

This follows the process that is used in PowerShell Core, except that the directories
in PSSA do not follow the same pattern as PSCore where the assembly name matches the directory
name. When editing resources, you should not use Visual Studio, because it creates the *.Designer.cs file which is no longer used.
Change the strings for the engine to be named EngineStrings rather than just Strings.
Add the ResGen program to the repo to create the strongly typed string files
Move the resources to a resources directory and remove the Designer.cs files
Add a -ResGen parameter to the build script to do on-demand resource generation
update appveyor script to build resources

Additionally, I remove -verbose from tests which used them when importing the scriptanalyzer module.

I have also moved some documentation around, moving the developer process, etc, to a new document as well as add coding guidelines

This follows the process that is used in PowerShell Core, except that the directories in PSSA do not follow the same pattern as PSCore where the assembly name matches the directory name. When editing resources, you should not use Visual Studio, because it creates the *.Designer.cs file which is no longer used. Change the strings for the engine to be named EngineStrings rather than just Strings. Add the ResGen program to the repo to create the stongly typed string files Move the resources to a resources directory and remove the Designer.cs files Add a `-ResGen` parameter to the build script to do on demand resource generation update appveyor script to build resources

bergmeister

So far, the changes look OK. The change to EngineString is good and reducing the verbosity of Import-Module in tests. The ReadMe.md and ReleaseMaker.psm1 files need to be adapted for the new -ResGen switch. There is one breaking change at the moment from a developer's perspective: I will not be able to just open Visual Studio in a clean checkout any more and just build or just edit resources in VS without having to rebuild ressources myself as a manual step, therefore to me this feels a bit like a step backwards but I can also understand that you want to make the process more similar and make it easier to build/develop on non-Windows systems...
Should we therefore maybe have a pre-build step on the projects to generate the resources automatically since it does not seem to be too expensive (around 1 second) at the moment? Then we also would not need the new switch any more and building in VS would work again out of the box again.
I assume the new -Docs switch is work in progress.
It would also be nice as part of this PR to move out the developer documentation from the main ReadMe into its own document because a lot of people (including me) visit the repo often just to find a copy-paste-able example of how to do suppressions, etc. (and it increased the loading time of the page)

JamesWTruher · 2018-03-04T02:19:57Z

visual studio is a windows only solution, so from a xplat perspective it's a problem. Also, Visual Studio seems to insist on these "Designer.cs" files which makes no sense in our environment (is there a way to suppress that or define where those files go? I'm a VS novice at best). Quite frankly, we could build resources automatically, I had just followed the model in PSCore, but that repo has a bunch more resources and it takes quite a bit longer, and that is not our problem.

At the end of the day, I'm really not fussed about not being able to build from VS at all (I see it as a nice to have rather than a must have), I would much rather be focussed on building out of VSCode which is xplat.

Some of the other changes on the way is improved documentation, including developer considerations, and moving those out of the README.md is one of the things I have planned.

bergmeister · 2018-03-04T11:02:17Z

Ok, understood.
We could have a pre-build task in the csproj of the Engine project (on which the Rules project depends on) like e.g.

  <Target Name="PreBuild" BeforeTargets="PreBuildEvent">
    <Exec Command="dotnet run" WorkingDirectory="$(ProjectDir)\..\ResGen" />
  </Target>

This worked fine via command line to build without the -ResGen switch. The lag of 1-2 seconds is noticeable but we could maybe tweak it with conditions (if you still want to keep the option of turning it on/off via command line) or to only run on full build or rebuild. In Visual Studio I had to build twice though for some reason for it to work. Although VS is not completely xplat (at least there is VS for Mac), it is still a commonly known and used tool by most .Net developers and the fact that PSCore has a PR about being able to build via VS shows that people actually do care about that. I totally accept the direction of being able to build/dev xplat in VSCode and therefore do not want to block this PR but also want to ensure that VS development is not too disadvantaged, which could hinder PRs from the Windows community. Feel free to continue with the PR, the one-off initialisation/update of ResGen is acceptable at the moment for VS development, we can tweak it later.

bergmeister · 2018-03-04T11:06:19Z

ResGen/ResGen.csproj

+    <TargetFramework>netcoreapp2.0</TargetFramework>
+    <AssemblyName>resgen</AssemblyName>
+    <OutputType>Exe</OutputType>
+    <RuntimeIdentifiers>win7-x86;win7-x64;osx.10.12-x64;linux-x64</RuntimeIdentifiers>


Why do we need the RIDs if the ResGen tool runs only locally on the build machine?

I just borrowed this from the PSCore repo, didn't really change anything except the program.cs file

also, I really like the Target in Engine.csproj, I'll change to use that rather than the explicit -ResGen switch.

…arate files Also add coding-guidelines and resource building documents

Update name of engine strings to avoid collision with rule strings Update tests to not import the module with -verbose`nAdd development focused documentation and remove from the main README

bergmeister

Overall, it looks OK but a subset of the comments below should be addressed. What is weird that I can see my recent commits (that are already in development) in the diff. I always do a pull of the upstream remote, which does a merge (instead of a rebase) and then GitHub always shows me the true difference.

bergmeister · 2018-03-06T22:15:29Z

ResGen/ResGen.csproj

@@ -0,0 +1,11 @@
+<Project Sdk="Microsoft.NET.Sdk">


Maybe consider adding this project to the main solution file (sln)?

since I don't use VS at all, I'm not quite sure what this would actually look like. Would you be willing to do that after this is merged?

Ok, I can do that. There might be a later PR from me to try to improve the VS experience (without affecting the VSCode experience). What is really nice with VS it that when I build in debug, then I can just go to the net451 bin folder of the rules and easily attach the debugger to it with ipmo .\Microsoft.Windows.PowerShe ll.ScriptAnalyzer.dll; Invoke-sccriptanalyzer -scriptdefinition '$foo=1' -AttachAndDebug

bergmeister · 2018-03-06T22:19:04Z

buildCoreClr.ps1

    [switch]$Build,
    [switch]$Uninstall,
    [switch]$Install,
+    [switch]$Docs,


I cannot see any usage of this switch?

yah - that will be a different PR (auto build docs) - pulled

bergmeister · 2018-03-06T22:22:11Z

buildCoreClr.ps1

    Write-Progress "Building Engine"
    Push-Location Engine\
-    dotnet build Engine.csproj --framework $Framework --configuration $Configuration
+    $null = dotnet build Engine.csproj --framework $Framework --configuration $Configuration


As long as issue #913 is not resolved and we do not have 'warning as error' as a build option, don't you think it would make sense to see the build output?

I'm ok with that - my preference is usually to see nothing, but not all share that view.

bergmeister · 2018-03-06T22:28:50Z

docs/dev-process/coding-guidelines.md

@@ -0,0 +1,215 @@
+# C# Coding Guidelines


This file looks like a copy pasted version of the one of PowerShell Core. Don't you think it makes sense to rather link it to it instead or highlight the differences if there are any? Just my 2 cents, you can still decide at the end.

yah - referring to the PSCore doc makes sense. I'll do that

bergmeister · 2018-03-06T22:34:03Z

docs/dev-process/resx-files.md

+## Editing `.resx` files
+
+**Do not edit** `.resx` files from Visual Studio. 
+It will try to create `.cs` files for you and you will get whole bunch of hard-to-understand errors.


I don't have this problem and I am using the latest update of VS. The only problem that I have though is that I have to build twice the first time in a clean checkout for some reason (it seems VS does not wait for the pre-build target to finish)

I took this from the PSCore documents. Does the *.Designer.cs file not get created? If not, I can remove this bit, but if the file is created, that's a problem.

My VS (latest version 15.6.1 on Win10) does not create such files any more in your branch, maybe older versions used to do that? Maybe it is better to just put a more generic disclaimer text in there saying that it is recommended to use the latest version that one might need to build twice the first time (to me this seems to be a bug that VS does not wait for the pre-build target to finish)

bergmeister · 2018-03-06T22:37:57Z

README.md

-* Adding/Removing resource strings
-
-For adding/removing resource strings in the `*.resx` files, it is recommended to use `Visual Studio` since it automatically updates the strongly typed `*.Designer.cs` files. The `Visual Studio 2017 Community Edition` is free to use but should you not have/want to use `Visual Studio` then you can either manually adapt the `*.Designer.cs` files or use the `New-StronglyTypedCsFileForResx.ps1` script although the latter is discouraged since it leads to a bad diff of the `*.Designer.cs` files.
+#### Building And Testing


It does not seem to me to it should be a sub-section of Installation. I think this should be rather have the same hierarchical level as Installation or Suppressing Rules.

…nalyzer into development

… output

bergmeister · 2018-03-10T21:00:11Z

It seems OK to me now, if you could just briefly comment on whether you want to address the last 2 remaining comments about the readme/coding-guidelines or not, I'd be happy to approve it.

bergmeister · 2018-03-23T19:55:00Z

Please resolve merge conflicts

JamesWTruher · 2018-03-30T18:35:49Z

enough time has passed that the merge conflicts have gotten pretty twisted. I'm going to close and open a new PR

bergmeister · 2018-03-30T19:15:38Z

Ok, sorry. @JamesWTruher I pulled your branch in and resolved the merge conflicts in my branch here
You should be able to just pull it into your branch as follows:

git remote add bergmeister https://github.com/bergmeister/PSScriptAnalyzer.git
git pull bergmeister jameswtruher/ResGen_ConflictResolved

rjmholt · 2019-01-29T21:45:21Z

The key change here for me would be to not check Strings.Designer.cs in.

It's a huge source of merge conflicts, and ones which have no simple fix. When rebasing you're forced to either regenerate the file in VS several times (which is particularly undesirable when I don't work in VS) or accept that some commits in the new history will be broken and fix them all at once in the final commit.

Neither of those situations is palatable to me at all, and I'd happily accept a few more seconds of build time if it means not needing to wait for VS to load or click through menus in the arcane "save and await magic to happen" ritual.

Just my 2 cents.

bergmeister · 2019-01-29T21:53:17Z

@rjmholt The PR was fine but then died due to merge conflicts. I'd be happy to revive a PR with a similar change set and get it in if it builds fine the first time when opening the solution in VS

bergmeister · 2019-03-01T19:05:01Z

@rjmholt It is expected that the .Net Core SDK will re-add the capability of generating strongly typed resources for 3.0, the current milestone is MSBuild 16.1 (16.0 will release on April 2), so I guess we'll be able to get this feature 'for free' at some point this summer dotnet/msbuild#2272

SteveL-MSFT added the in progress label Mar 2, 2018

JamesWTruher force-pushed the jameswtruher/ResGen branch from b0ab31f to e72bfc5 Compare March 2, 2018 21:46

JamesWTruher requested a review from bergmeister March 2, 2018 22:35

bergmeister reviewed Mar 3, 2018

View reviewed changes

bergmeister added the Area - Build label Mar 3, 2018

bergmeister reviewed Mar 4, 2018

View reviewed changes

JamesWTruher added 3 commits March 5, 2018 15:18

Split README.md and move building/testing and release building to sep…

5c522ac

…arate files Also add coding-guidelines and resource building documents

Change resource generation to match PSCore

2e7a37b

Update name of engine strings to avoid collision with rule strings Update tests to not import the module with -verbose`nAdd development focused documentation and remove from the main README

Merge branch 'jameswtruher/resgen2' into development

bd3038b

JamesWTruher changed the title ~~WIP: Change resource compilation process to match PowerShell Core~~ Change resource compilation process to match PowerShell Core Mar 6, 2018

JamesWTruher removed the in progress label Mar 6, 2018

bergmeister requested changes Mar 6, 2018

View reviewed changes

JamesWTruher added 2 commits March 9, 2018 12:43

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

2b5e127

…nalyzer into development

Remove unused parameter and allow output from dotnet build to flow as…

ec7268e

… output

JamesWTruher force-pushed the jameswtruher/ResGen branch from 66a9c80 to ec7268e Compare March 9, 2018 20:43

JamesWTruher closed this Mar 30, 2018

Change resource compilation process to match PowerShell Core #916

Change resource compilation process to match PowerShell Core #916

Uh oh!

Conversation

JamesWTruher commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

more details

Uh oh!

bergmeister left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesWTruher commented Mar 4, 2018

Uh oh!

bergmeister commented Mar 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesWTruher Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister commented Mar 10, 2018

Uh oh!

bergmeister commented Mar 23, 2018

Uh oh!

JamesWTruher commented Mar 30, 2018

Uh oh!

bergmeister commented Mar 30, 2018

Uh oh!

rjmholt commented Jan 29, 2019

Uh oh!

bergmeister commented Jan 29, 2019

Uh oh!

bergmeister commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

JamesWTruher commented Mar 2, 2018 •

edited

Loading

bergmeister left a comment •

edited

Loading

bergmeister commented Mar 4, 2018 •

edited

Loading

bergmeister left a comment •

edited

Loading

JamesWTruher Mar 9, 2018 •

edited

Loading

bergmeister Mar 6, 2018 •

edited

Loading

bergmeister Mar 6, 2018 •

edited

Loading

bergmeister commented Mar 1, 2019 •

edited

Loading