Csproj format update, fixes #3944

[Vogel612]

No description provided.

[bclothier]

Just few questions....

1. Was it intentional to delete this inspection
2. EasyHook files were apparently deleted. Is that intentional as well? I did not see a corresponding addition; only a PackageReference, and I am assuming we are now referencing the files in the packages folder which isn't a part of the git solution; but a bit unsure why files existed inside the projects originally.
3. There were a number of conditions that were deleted that I don't think should be:

<Error Condition="!Exists('..\packages\Antlr4.CodeGenerator.4.6.4\build\Antlr4.CodeGenerator.props')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Antlr4.CodeGenerator.4.6.4\build\Antlr4.CodeGenerator.props'))" />
    <Error Condition="!Exists('..\packages\Antlr4.CodeGenerator.4.6.4\build\Antlr4.CodeGenerator.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Antlr4.CodeGenerator.4.6.4\build\Antlr4.CodeGenerator.targets'))" />

Judging from other diff, the <Error Condition="..." /> tag seems legal even in the new csproj format. There were a number of projects that had those error condition for both ANTLR and NUnit.

[Vogel612]

Re 1: That inspection had been removed from the csproj files, but not from the git working tree. It uses old API for inspections and refers to resources that have been removed. The move to the new csproj format (which uses globbing to include files) just surfaced that mishap.

Re 2: EasyHook (as well as NLog.Schema and possibly other NuGet Packages) pushed parts of the content they should add to the /packages directory to the solution content. That behaviour is apparently not supported for PackageReference items. The changes in these cases were performed by the automatic NuGet package migration tool in VS. The tool produced a few warnings, but I haven't been able to spot any impact on the functionality in the final deployed assembly.

Re 3: Good spot, I'll make sure to reinstate these error conditions

[codecov]

Codecov Report

Merging #4383 into next will decrease coverage by 6.9%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             next    #4383     +/-   ##
=========================================
- Coverage   63.76%   56.86%   -6.9%     
=========================================
  Files         933      944     +11     
  Lines       31299    32217    +918     
=========================================
- Hits        19955    18318   -1637     
- Misses      11344    13899   +2555

Impacted Files	Coverage Δ
Rubberduck.Parsing/Symbols/AliasDeclaration.cs	0% <0%> (-100%)	⬇️
...crete/ImplicitDefaultMemberAssignmentInspection.cs	5.56% <0%> (-94.44%)	⬇️
Rubberduck.UnitTesting/UnitTesting/TestResult.cs	0% <0%> (-84.62%)	⬇️
Rubberduck.Parsing/ComReflection/ComTypeName.cs	0% <0%> (-61.9%)	⬇️
...bberduck.Parsing/Symbols/PropertyGetDeclaration.cs	39.29% <0%> (-58.93%)	⬇️
...bberduck.Parsing/Symbols/PropertyLetDeclaration.cs	46.15% <0%> (-51.92%)	⬇️
Rubberduck.UnitTesting/UnitTesting/ITestEngine.cs	0% <0%> (-50%)	⬇️
Rubberduck.UnitTesting/UnitTesting/TestEngine.cs	16.55% <0%> (-49.66%)	⬇️
...uck.Parsing/Symbols/ProceduralModuleDeclaration.cs	30.3% <0%> (-48.48%)	⬇️
...ubberduck.Parsing/Symbols/SubroutineDeclaration.cs	54.76% <0%> (-42.86%)	⬇️
... and 266 more

[bclothier]

I believe I have discovered the source of instability in the build process. I think this section in the Rubberduck.Core.csproj is wrong.

  <!--BEGIN XAML WORKAROUND SECTION -->
  <ItemGroup>
    <Compile Include="$(IntermediateOutputPath)**\*.g.cs" Visible="false" />
    <Page Include="**\*.xaml">
      <SubType>Designer</SubType>
      <Generator>MSBuild:Compile</Generator>
      <!-- <Generator>MSBuild:UpdateDesignTimeXaml</Generator>-->
    </Page>
    <Compile Update="**\*.xaml.cs">
      <SubType>Code</SubType>
      <DependentUpon>%(Filename)</DependentUpon>
    </Compile>
    <!-- Resources -->
    <EmbeddedResource Update="Properties\Resources.resx" Generator="ResXFileCodeGenerator" LastGenOutput="Resources.Designer.cs" />
    <Compile Update="Properties\Resources.Designer.cs" AutoGen="True" DependentUpon="Resources.resx" DesignTime="True" />
    <!-- Settings -->
    <None Update="Properties\Settings.settings" Generator="SettingsSingleFileGenerator" LastGenOutput="Settings.Designer.cs" />
    <Compile Update="Properties\Settings.Designer.cs" AutoGen="True" DependentUpon="Settings.settings" />
  </ItemGroup>
  <!-- END XAML WORKAROUND SECTION -->

With this setting, the obj folder in the Rubberduck.Core goes nuts. Even when Visual Studio is just sitting there doing nothing, you can see files being modified, added, deleted nonstop in the obj folder. I think this triggers a infinite recursion trying to resolve all types in the intermediate files.

I made the following changes:

    <!--<Compile Include="$(IntermediateOutputPath)**\*.g.cs" Visible="false" />-->
    <Page Include="**\*.xaml">
      <SubType>Designer</SubType>
      <Generator>MSBuild:Compile</Generator>
      <!-- <Generator>MSBuild:UpdateDesignTimeXaml</Generator>-->
    </Page>

With this change, the obj folder became stable, Visual Studio starts performing better and build is now more consistent. I have not tested whether I have broke the WPF, however. I inferred the settings from a SO thread.

With the build troubles out of the way, I was able to properly test and verify that yes, we must have [assembly: ComVisible(false)]; the default is to make it COM-visible, which causes the MIDL to pick up unwanted types and thus fail with an error about missing forward declarations for certain types.

Please confirm if you'd like my commit to be added or if there's some other preference to merge the changes.

[bclothier]

I do not think we need this. From what I can tell, we don't emit any additional warnings without that setting. Because this impacts the project references (e.g. a CLS compliant project shouldn't reference a non-CLS compliant project), and this is the only time we have this, it's best to not have the attribute so we actually get warnings if this does become an issue.

[bclothier]

This feels a bit more fragile. What if we add new language? Do we have to go and add this everywhere we need i18n? Couldn't we replace .??.resex to .resx?

[Vogel612]

I'm not 100% clear on how wildcards work, but it might be worth a try to unify this, yea

[bclothier]

This is easily my favorite feature of the new csproj format -- ability to standardize all projects. Love that! 👍

[bclothier]

The condition need to include RubberduckTests and RubberduckCodeAnalysisTests; makes no sense to run the analyzers there. (note: fixed in my commit that's not pushed yet)

[Vogel612]

Reminder to self: check whether we can use some of the MSBuild community tasks to simplify our stuff even more: https://github.com/loresoft/msbuildtasks

[Vogel612]

As it stands, this PR will utterly and completely break the UI. That's because when we're initializing compoents based on XAML, we have a System.Uri that looks a bit like this: "/Rubberduck.Core;V2.2.*;[...]".

The second part of that URI is parsed as a System.Version to determine the Assembly to load for the XAML. That's not possible because * is rather obviously not an integer. As such loading any XAML component will result in a FormatException. That means we either don't have a UI or even crash the host as it stands. That's what broke the test ignored in 9555bbb

I'll need some help figuring out a proper fix for this. I assume that dotnet/project-system#1467 is closely realated to this. A symptomatic fix could be to explicitly specify a non-wildcard version for Rubberduck.Core. I don't know whether that will break COM Registration or the deployment process though.

Opinions highly welcome.

[bclothier]

That does not make sense - it seems to be saying that if the project is RubberduckTests, to add RubberduckTestsCodeAnalysis as an analyzer for that project. But the RubberduckTestsCodeAnalysis project is a test project testing the analzyer, not an analyzer project... Or am I misunderstanding?

[bclothier]

Regarding the problem with the WPF URI -- this says the version is optional. Do we really need to have the version embedded in the URI? Seems to me that it'd simplify lot of things if we only cared about the resource name, regardless of the version?

[Vogel612]

Reminder to self: get MSBump to put in versions: https://github.com/BalassaMarton/MSBump

[Vogel612]

Okay I think I'm done now. For me locally running the BuildRegistry script still fails from within VisualStudio. I haven't attempted to run it in msbuild alone. If I run the exact command that VS should run in a powershell host it works as expected and correctly registers the build.

As such I'm assuming that VS is somehow at fault.

The problem with XAML has been symptomatically fixed, pending a more in-depth diagnosis of the underlying problem. I unfortunately am not remotely knowledgeable enough to find out where this goes wrong.

I have not included MSBump (yet?). Currently we're patching versions through AppVeyor, which works fine for AV. Locally the wildcard just goes to a generated buildnumber which is dependent on the timestamp. That way builds will properly generate different versions.

I have not verified that Fakes work as expected. Parsing, UnitTesting and similar things seem to work just as intended from cursory F5 testing. Unless there's something major, this is ready to merge from my side now.

[retailcoder]

Massive props for the massive work here, @Vogel612 - this diff is awesome (negative 46.3K!!) and we're clearly all going to benefit from a leaner .csproj format.
Just needs clarification w/rt the AppVeyor build versioning, though. Looks like the appveyor.yml file handles it fine?