Update eligible projects to new SDK format. #1761

grokys · 2018-06-22T13:48:59Z

Now we're using VS2017 we can use the new simplified SDK-style .csproj format. This PR updates the projects that can be updated to this format. It also enables Microsoft.CodeAnalysis.FxCopAnalyzers on them, which is the successor to CodeAnalysis. I've not enabled warnings as errors yet though, because these analyzers find a fair few problems that will need to be fixed first.

WPF projects and VSSDK projects can't use the SDK format yet. In addition GitHub.TeamFoundation.15 uses an alias on a nuget package which can't currently be done, so the GitHub.TeamFoundation.X projects haven't been converted.

Neme12 · 2018-06-22T17:33:11Z

VSSDK projects can't use the SDK format yet

I've seen VS extension projects using the new project system in Microsoft repos, for example here:
/~https://github.com/dotnet/roslyn/blob/master/src/VisualStudio/Setup/VisualStudioSetup.csproj
and
/~https://github.com/dotnet/project-system/blob/master/src/ProjectSystemSetup/ProjectSystemSetup.csproj

but haven't found any info or documentation on that. Maybe this is an experimental & undocumented feature?

jcansdale · 2018-06-23T16:51:38Z

I appears the nunit3-vs-adapter recently went through a similar process nunit/nunit3-vs-adapter#499.

Maybe @jnm2 will have some tips for us. 😄

Neme12 · 2018-06-23T16:54:16Z

@jcansdale I think you can use PackageReference even without the new project file format, it should be orthogonal to that.

Neme12 · 2018-06-23T16:57:13Z

In fact I thought this would be at least a 2-step transition with multiple PRs to make this huge change easier to review and take iteratively.

jnm2 · 2018-06-23T17:59:36Z

Sorry to disappoint, but when I looked at the VSIX project, I didn't see any ROI in getting it working with the new SDK. It's possible Oren has something up his sleeve—I know he has WPF working on the new csproj SDK. /~https://github.com/onovotny/MSBuildSdkExtras#msbuildsdkextras

Neme12 · 2018-06-23T18:41:51Z

I don't understand what the problem with WPF is? I just tried creating a new WPF project & changing the csproj and everything worked fine.

The only problem is that .xaml files are not included properly by default, so you have to list each one of them in the csproj just like today. At least that's what I thought until I found this:
microsoft/VSProjectSystem#169 (comment)

So using that, now the csproj looks like:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net46</TargetFramework>
    <OutputType>WinExe</OutputType>
  </PropertyGroup>
  <ItemGroup>
    <Reference Include="System.Xaml" />
    <Reference Include="WindowsBase" />
    <Reference Include="PresentationCore" />
    <Reference Include="PresentationFramework" />
  </ItemGroup>
  <ItemGroup>
    <None Remove="**\*.xaml" />
    <ApplicationDefinition Include="App.xaml">
      <Generator>MSBuild:Compile</Generator>
      <SubType>Designer</SubType>
    </ApplicationDefinition>
    <Page Include="**\*.xaml" Exclude="App.xaml">
      <Generator>MSBuild:Compile</Generator>
      <SubType>Designer</SubType>
    </Page>
    <Compile Update="**\*.xaml.cs">
      <DependentUpon>%(Filename)</DependentUpon>
      <SubType>Code</SubType>
    </Compile>
  </ItemGroup>
</Project>

everything works perfectly and it is stable when adding/removing xaml files inside VS (it doesn't modify the project file to add duplicate items).

for a library, the includes would be simplified to:

  <ItemGroup>
    <None Remove="**\*.xaml" />
    <Page Include="**\*.xaml">
      <Generator>MSBuild:Compile</Generator>
      <SubType>Designer</SubType>
    </Page>
    <Compile Update="**\*.xaml.cs">
      <DependentUpon>%(Filename)</DependentUpon>
      <SubType>Code</SubType>
    </Compile>
  </ItemGroup>

WPF projects and VSSDK projects can't use the SDK format yet. In addition `GitHub.TeamFoundation.15` uses an alias on a nuget package which can't currently be done, so the `GitHub.TeamFoundation.X` projects haven't been converted.

Had to also remove the `<RootNamespace>` as it was causing the resources to get generated in the wrong place.

Also - Removed more unnecessary dependencies - Moved a couple of tests to the right project - Removed `GitHub.Primitives.UnitTests` as it was testing stuff in `GitHub.Exports`, so moved to `GitHub.Exports.UnitTests`

jcansdale · 2018-06-25T23:55:17Z

Directory.Build.Props

+  <PropertyGroup>
+    <Product>GitHub Extension for Visual Studio</Product>
+    <Version>2.5.4.0</Version>
+    <Copyright>Copyright © GitHub, Inc. 2014-2018</Copyright>


We could maybe do something like this to future-proof... 😉

<CopyrightYear>$([System.DateTime]::Now.ToString("yyyy"))</CopyrightYear> <Copyright>Copyright © GitHub, Inc. 2014-$(CopyrightYear)</Copyright>

jcansdale

I'm confused where this project gets its VS SDK references from.

jcansdale · 2018-06-26T09:11:30Z

src/GitHub.Services.Vssdk/GitHub.Services.Vssdk.csproj

-<?xml version="1.0" encoding="utf-8"?>
-<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
-  <Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />
+<Project Sdk="Microsoft.NET.Sdk">


I'm confused where this project gets its VS SDK references from.

It gets them from GitHub.Exports.

Having them be PCL meant that we had to have both a PCL and .netfx version of Splat, which was causing problems with test runners because they would load the wrong version of the library at random.

grokys · 2018-06-26T15:58:29Z

@Neme12 yep, i've managed to get WPF projects working now. Thanks for the pointer - I'd tried something similar before but couldn't get it to work. Not sure what I was missing.

Neme12 · 2018-06-26T16:03:46Z

src/common/xaml.props

+      <DependentUpon>%(Filename)</DependentUpon>
+      <SubType>Code</SubType>
+    </Compile>
+    <Page Include="**\*.xaml">


The reason why I included <None Remove="**\*.xaml" /> before this in my example is that without that, when adding a new xaml control inside VS, it adds a new line here with <None Remove="AddedFile.xaml" />

But I have no idea how this behaves and what VS would do when this is in am imported project. Did you check that it works properly and doesn't modify the csproj?

Hmm yeah you're right, it does modify the .csproj...

Looks like this is the associated issue: dotnet/project-system#3639

I think for the moment I'm going to revert the change for converting our WPF projects to the new csproj and try again when 15.8 is available.

I didn't think this was a bug. They are included as None by default so VS tries to do its best to undo that and make sure they're only included as Page.

This reverts commit aeca53a.

Conflicts: test/Helpers/SplatModeDetectorSetUp.cs

They weren't getting included in the vsix.

jnm2 · 2018-07-16T01:20:37Z

@jcansdale Hey, here's an example of a new-style VSIX project by @sharwell: /~https://github.com/tunnelvisionlabs/JustMyCodeToggle/blob/master/Tvl.VisualStudio.JustMyCodeToggle/Tvl.VisualStudio.JustMyCodeToggle.csproj

jnm2 · 2018-07-19T01:26:15Z

It's been amazing. In all seriousness, I wonder what the telemetry would report if it was a native and documented feature.

grokys · 2018-08-24T14:45:22Z

@sharwell just got back to this and tried using MSBuild.Sdk.Extras but I'm still getting the:

  <ItemGroup>
    <Compile Remove="Controls\UserControl1.xaml.cs" />
  </ItemGroup>

added whenever I add a XAML/cs file combination, and they don't appear nested.

clairernovotny · 2018-08-24T14:51:59Z

Have you added <ExtrasEnableWpfProjectSetup>true</ExtrasEnableWpfProjectSetup> to your project? That turns on the WPF defaults.

grokys · 2018-08-25T22:32:24Z

@onovotny yeah, I have. Just pushed the commit with the change in. If you want to try it, try going to GitHub.UI and adding a WPF user control and saving the solution.

grokys · 2018-09-05T09:14:12Z

@onovotny this seems to be fixed with MSBuild.Sdk.Extras 1.6.52 - thanks!

shana

Looks good in general, I'm just wondering where the settings for code analysis are. The csproj point to a specific ruleset, but this new format doesn't seem to have that setting or any configuration to enable/disable and set warnings as errors. Given how many warnings are on CI, I'm guessing it's using the default ruleset for these?

shana · 2018-09-06T12:50:59Z

test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs

@@ -17,7 +17,7 @@

 public class VSGitExtTests
 {
-    public class TheConstructor : TestBaseClass
+    public class TheConstructor


Why is this inheritance changing?

Ummm... I can't remember. I'll put this back.

grokys · 2018-09-06T15:08:59Z

Looks good in general, I'm just wondering where the settings for code analysis are.

Yes it's using the default ruleset for now, and not enabling warnings as errors. The new analyser has a lot more checks that are very useful and that I don't want to disable (such as always using ConfigureAwait() on Tasks) but I didn't want to litter this PR with changes to fix these warnings.

My reasoning was that we should fix these warnings and enable warnings as errors in a separate PR.

Can't remember why this was removed.

Really no idea why I removed these...

clairernovotny · 2018-09-07T15:56:59Z

Directory.Build.Props

@@ -1,7 +1,7 @@
 <Project>
  <PropertyGroup>
    <Product>GitHub Extension for Visual Studio</Product>
-    <Version>2.5.5.0</Version>
+    <Version>2.5.6.0</Version>


Do you need to manually version? There are better ways to do this :)

Oh really? Please do elucidate! What would be a better way to do this?

I've always hated version stamping. I'd be interested in finding the least-hacky way! 😉

I have a project that matches this one very closely -- xUnit for Devices. It has a VSIX for its templates and auto-versions.

/~https://github.com/xunit/devices.xunit

Here are some key things:

Uses Nerdbank.GitVersioning to get a version for each commit. That handles the csproj's.

Build scripts update other xml assets to match: /~https://github.com/xunit/devices.xunit/blob/master/.vsts-shared.yml#L31-L36

jcansdale

This seems to work well and it's impressive how much the size of .csproj files have been reduced. I think if we delay merging this now, we'll have nasty merge issues further down the road. I'd say LGTM since we're at the beginning a of a dev cycle.

grokys requested review from shana, StanleyGoldman and jcansdale June 22, 2018 13:48

grokys force-pushed the refactor/sdk-csproj branch from 434565b to 624b6c6 Compare June 22, 2018 14:03

grokys force-pushed the refactor/sdk-csproj branch from 624b6c6 to f58e177 Compare June 25, 2018 09:07

grokys added 2 commits June 25, 2018 11:10

Update eligable src/ projects to new SDK format.

59a61ce

WPF projects and VSSDK projects can't use the SDK format yet. In addition `GitHub.TeamFoundation.15` uses an alias on a nuget package which can't currently be done, so the `GitHub.TeamFoundation.X` projects haven't been converted.

Added FxCopAnalyzers to SDK projects.

3eeb718

grokys force-pushed the refactor/sdk-csproj branch from 5b691b1 to 3eeb718 Compare June 25, 2018 09:10

grokys added 4 commits June 25, 2018 11:51

Explicitly include resx file in GitHub.App.

deaad94

Had to also remove the `<RootNamespace>` as it was causing the resources to get generated in the wrong place.

Don't need to include dependent packages.

bce1750

Update test projects to SDK csproj format.

7e49c44

Also - Removed more unnecessary dependencies - Moved a couple of tests to the right project - Removed `GitHub.Primitives.UnitTests` as it was testing stuff in `GitHub.Exports`, so moved to `GitHub.Exports.UnitTests`

Make tests run in ncrunch.

d00c6e5

meaghanlewis added this to the 2.5.4 milestone Jun 25, 2018

jcansdale reviewed Jun 25, 2018

View reviewed changes

jcansdale reviewed Jun 26, 2018

View reviewed changes

grokys added 2 commits June 26, 2018 16:22

Remove PCL akavache and splat libs.

4c28e46

Having them be PCL meant that we had to have both a PCL and .netfx version of Splat, which was causing problems with test runners because they would load the wrong version of the library at random.

Convert WPF assemblies to new csproj format.

aeca53a

Neme12 reviewed Jun 26, 2018

View reviewed changes

grokys added 3 commits June 27, 2018 09:44

Revert "Convert WPF assemblies to new csproj format."

b1958bd

This reverts commit aeca53a.

Merge branch 'master' into refactor/sdk-csproj

e97e222

Conflicts: test/Helpers/SplatModeDetectorSetUp.cs

Include serilog packages in GitHub.VisualStudio.

8e495ad

They weren't getting included in the vsix.

meaghanlewis removed this from the 2.5.4 milestone Jul 13, 2018

grokys mentioned this pull request Jul 19, 2018

Refactored Clone Dialog #1787

Merged

donokuda mentioned this pull request Jul 31, 2018

Usernames do not show up in review comments #1819

Closed

grokys added 3 commits August 23, 2018 15:37

Merge branch 'master' into refactor/sdk-csproj

7217eff

Update Directory.Build.Props when bumping version.

8834657

Use MSBuild.Sdk.Extras for WPF projects.

7f97764

grokys added 5 commits August 28, 2018 18:23

Merge branch 'master' into refactor/sdk-csproj

de441e2

Pass correct VS version to msbuild.

b04949a

Fix CA errors.

706280c

Merge branch 'master' into refactor/sdk-csproj

8aed135

Update MSBuild.Sdk.Extras version.

e5f8504

grokys changed the title ~~WIP: Update eligible projects to new SDK format.~~ Update eligible projects to new SDK format. Sep 5, 2018

meaghanlewis added this to the 2.5.6 milestone Sep 5, 2018

Merge branch 'master' into refactor/sdk-csproj

2e5ffb0

shana reviewed Sep 6, 2018

View reviewed changes

grokys added 4 commits September 6, 2018 17:23

Fix CA errors.

3490860

Reinstate base class.

99d416f

Can't remember why this was removed.

Reinstate other VSGitExtTests base classes.

c05585e

Really no idea why I removed these...

Bump version in Directory.Build.props.

7e6a1f6

clairernovotny reviewed Sep 7, 2018

View reviewed changes

Merge branch 'master' into refactor/sdk-csproj

cf25357

jcansdale approved these changes Sep 10, 2018

View reviewed changes

grokys merged commit 4eef5b3 into master Sep 10, 2018

grokys deleted the refactor/sdk-csproj branch September 10, 2018 12:00

grokys mentioned this pull request Oct 5, 2018

Unify Newtonsoft.Json versions to 9.0.1. #1975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update eligible projects to new SDK format. #1761

Update eligible projects to new SDK format. #1761

grokys commented Jun 22, 2018 •

edited

Loading

Neme12 commented Jun 22, 2018 •

edited

Loading

jcansdale commented Jun 23, 2018

Neme12 commented Jun 23, 2018

Neme12 commented Jun 23, 2018

jnm2 commented Jun 23, 2018 •

edited

Loading

Neme12 commented Jun 23, 2018 •

edited

Loading

jcansdale Jun 25, 2018

jcansdale left a comment

jcansdale Jun 26, 2018

grokys Jun 26, 2018

grokys commented Jun 26, 2018

Neme12 Jun 26, 2018 •

edited

Loading

grokys Jun 26, 2018

grokys Jun 26, 2018 •

edited

Loading

grokys Jun 27, 2018

Neme12 Jun 27, 2018 •

edited

Loading

jnm2 commented Jul 16, 2018

jnm2 commented Jul 19, 2018

grokys commented Aug 24, 2018

clairernovotny commented Aug 24, 2018

grokys commented Aug 25, 2018

grokys commented Sep 5, 2018

shana left a comment

shana Sep 6, 2018

grokys Sep 6, 2018

grokys commented Sep 6, 2018

clairernovotny Sep 7, 2018

grokys Sep 7, 2018

jcansdale Sep 7, 2018

clairernovotny Sep 7, 2018

jcansdale left a comment

Update eligible projects to new SDK format. #1761

Update eligible projects to new SDK format. #1761

Conversation

grokys commented Jun 22, 2018 • edited Loading

Neme12 commented Jun 22, 2018 • edited Loading

jcansdale commented Jun 23, 2018

Neme12 commented Jun 23, 2018

Neme12 commented Jun 23, 2018

jnm2 commented Jun 23, 2018 • edited Loading

Neme12 commented Jun 23, 2018 • edited Loading

Choose a reason for hiding this comment

jcansdale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grokys commented Jun 26, 2018

Neme12 Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grokys Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neme12 Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

jnm2 commented Jul 16, 2018

jnm2 commented Jul 19, 2018

grokys commented Aug 24, 2018

clairernovotny commented Aug 24, 2018

grokys commented Aug 25, 2018

grokys commented Sep 5, 2018

shana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grokys commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcansdale left a comment

Choose a reason for hiding this comment

grokys commented Jun 22, 2018 •

edited

Loading

Neme12 commented Jun 22, 2018 •

edited

Loading

jnm2 commented Jun 23, 2018 •

edited

Loading

Neme12 commented Jun 23, 2018 •

edited

Loading

Neme12 Jun 26, 2018 •

edited

Loading

grokys Jun 26, 2018 •

edited

Loading

Neme12 Jun 27, 2018 •

edited

Loading