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

Update eligible projects to new SDK format. #1761

Merged
merged 26 commits into from Sep 10, 2018

Conversation

Projects
None yet
8 participants
@grokys
Contributor

grokys commented Jun 22, 2018

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.

@grokys grokys requested review from shana, StanleyGoldman and jcansdale Jun 22, 2018

@Neme12

This comment has been minimized.

Show comment
Hide comment
@Neme12

Neme12 Jun 22, 2018

Contributor

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?

Contributor

Neme12 commented Jun 22, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jcansdale

jcansdale Jun 23, 2018

Contributor

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. 😄

Contributor

jcansdale commented Jun 23, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Neme12

Neme12 Jun 23, 2018

Contributor

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

Contributor

Neme12 commented Jun 23, 2018

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

@Neme12

This comment has been minimized.

Show comment
Hide comment
@Neme12

Neme12 Jun 23, 2018

Contributor

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.

Contributor

Neme12 commented Jun 23, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jnm2

jnm2 Jun 23, 2018

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

jnm2 commented Jun 23, 2018

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

This comment has been minimized.

Show comment
Hide comment
@Neme12

Neme12 Jun 23, 2018

Contributor

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>
Contributor

Neme12 commented Jun 23, 2018

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>

grokys added some commits Jun 22, 2018

Update eligable src/ projects to new SDK format.
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.
Explicitly include resx file in GitHub.App.
Had to also remove the `<RootNamespace>` as it was causing the resources to get generated in the wrong place.
Update test projects to SDK csproj format.
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`

@meaghanlewis meaghanlewis added this to the 2.5.4 milestone Jun 25, 2018

Show outdated Hide outdated Directory.Build.Props Outdated
@jcansdale

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

<?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">

This comment has been minimized.

@jcansdale

jcansdale Jun 26, 2018

Contributor

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

@jcansdale

jcansdale Jun 26, 2018

Contributor

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

This comment has been minimized.

@grokys

grokys Jun 26, 2018

Contributor

It gets them from GitHub.Exports.

@grokys

grokys Jun 26, 2018

Contributor

It gets them from GitHub.Exports.

grokys added some commits Jun 26, 2018

Remove PCL akavache and splat libs.
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

This comment has been minimized.

Show comment
Hide comment
@grokys

grokys Jun 26, 2018

Contributor

@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.

Contributor

grokys commented Jun 26, 2018

@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.

Show outdated Hide outdated src/common/xaml.props Outdated

grokys added some commits Jun 27, 2018

Merge branch 'master' into refactor/sdk-csproj
 Conflicts:
	test/Helpers/SplatModeDetectorSetUp.cs
Include serilog packages in GitHub.VisualStudio.
They weren't getting included in the vsix.

@meaghanlewis meaghanlewis removed this from the 2.5.4 milestone Jul 13, 2018

@jnm2

This comment has been minimized.

Show comment
Hide comment
@sharwell

This comment has been minimized.

Show comment
Hide comment
@sharwell

sharwell Jul 16, 2018

If you want WPF projects to work, I recommend using https://github.com/onovotny/MSBuildSdkExtras by @onovotny.

sharwell commented Jul 16, 2018

If you want WPF projects to work, I recommend using https://github.com/onovotny/MSBuildSdkExtras by @onovotny.

Merge branch 'master' into refactor/sdk-csproj
 Conflicts:
	src/GitHub.Api/packages.config
	src/GitHub.App/packages.config
	src/GitHub.Exports/packages.config
@jcansdale

This comment has been minimized.

Show comment
Hide comment
@jcansdale

jcansdale Jul 18, 2018

Contributor

@jnm2, @sharwell,

Thanks for the pointers. I definitely need the JustMyCodeToggle extension - installed. 🎉

At the very least I'm going to steal that project template for my https://github.com/jcansdale/TemporaryProjects extension! 😄

Contributor

jcansdale commented Jul 18, 2018

@jnm2, @sharwell,

Thanks for the pointers. I definitely need the JustMyCodeToggle extension - installed. 🎉

At the very least I'm going to steal that project template for my https://github.com/jcansdale/TemporaryProjects extension! 😄

@grokys

This comment has been minimized.

Show comment
Hide comment
@grokys

grokys Aug 24, 2018

Contributor

@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.

Contributor

grokys commented Aug 24, 2018

@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.

@onovotny

This comment has been minimized.

Show comment
Hide comment
@onovotny

onovotny Aug 24, 2018

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

onovotny commented Aug 24, 2018

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

@grokys

This comment has been minimized.

Show comment
Hide comment
@grokys

grokys Aug 25, 2018

Contributor

@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.

Contributor

grokys commented Aug 25, 2018

@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 added some commits Aug 28, 2018

@grokys

This comment has been minimized.

Show comment
Hide comment
@grokys

grokys Sep 5, 2018

Contributor

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

Contributor

grokys commented Sep 5, 2018

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

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

@meaghanlewis meaghanlewis added this to the 2.5.6 milestone Sep 5, 2018

@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?

Show outdated Hide outdated test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs Outdated
@grokys

This comment has been minimized.

Show comment
Hide comment
@grokys

grokys Sep 6, 2018

Contributor

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.

Contributor

grokys commented Sep 6, 2018

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.

grokys added some commits Sep 6, 2018

Reinstate base class.
Can't remember why this was removed.
Reinstate other VSGitExtTests base classes.
Really no idea why I removed these...
Show outdated Hide outdated Directory.Build.Props Outdated
@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 grokys merged commit 4eef5b3 into master Sep 10, 2018

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@grokys grokys deleted the refactor/sdk-csproj branch Sep 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment