Skip to content
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

Convert project format to msbuild15 #277

Merged
merged 15 commits into from
Aug 5, 2017
Merged

Convert project format to msbuild15 #277

merged 15 commits into from
Aug 5, 2017

Conversation

Kesmy
Copy link
Contributor

@Kesmy Kesmy commented Jul 20, 2017

Addresses #258 , converting project files to the new csproj format and updating the build tools version.

This breaks design/build-time compatibility with older versions of Visual Studio/MSBuild, but should have no effect on consumers.

The PCL.Specs project cannot currently be converted, as .NET Portable is not supported by the new format.

@dnfclas
Copy link

dnfclas commented Jul 20, 2017

@Kesmy,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

@dnfclas
Copy link

dnfclas commented Jul 20, 2017

@Kesmy, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

@reisenberger
Copy link
Member

reisenberger commented Jul 20, 2017

@Kesmy HUGE, huge thanks for the work on this, awesome to see this so far progressed! 👍 👍

I ran up the cake build script (and visual studio build obv) and picked up just a few (hopefully small to fix) issues from the cake build:

Nuget packages made by cake build aren't installable

Inspected the packages with NuGetPackageExplorer and it looks like the dlls all got pushed down an extra folder, eg into lib\net45\net45\Polly.dll rather than just lib\net45\Polly.dll. Used NuGetPackageExplorer to edit a package (remove the extra folder) and then it installed fine. So guess we just need to fix this in the new build.

Cake build seems to compile Polly.Net40Async twice:

========================================
__BuildSolutions
========================================
[...stuff elided...]

  Polly.Net45 -> C:\ThirdPartyGitHub\Kesmy\Polly\src\Polly.Net45\bin\Release\net45\Polly.dll
  Polly.Net45.Specs -> C:\ThirdPartyGitHub\Kesmy\Polly\src\Polly.Net45.Specs\bin\Release\net45\Polly.Net45.Specs.dll
  Polly.NetStandard11 -> C:\ThirdPartyGitHub\Kesmy\Polly\src\Polly.NetStandard11\bin\Release\netstandard1.1\Polly.dll
  Polly.Pcl.Specs -> C:\ThirdPartyGitHub\Kesmy\Polly\src\Polly.Pcl.Specs\bin\Release\Polly.Pcl.Specs.dll
  Polly.Net40Async -> C:\ThirdPartyGitHub\Kesmy\Polly\src\Polly.Net40Async\bin\Release\net40\Polly.Net40Async.dll
  Polly.Net40Async -> C:\ThirdPartyGitHub\Kesmy\Polly\src\Polly.Net40Async\bin\Release\net40\Polly.Net40Async.dll
  Polly.Net40Async.Specs -> C:\ThirdPartyGitHub\Kesmy\Polly\src\Polly.Net40Async.Specs\bin\Release\net45\Polly.Net40Async.Specs.dll

Cake script seems to only detect the PCL.Specs tests to run:

========================================
__RunTests
========================================
Executing task: __RunTests
xUnit.net Console Runner (64-bit .NET 4.0.30319.42000)
  Discovering: Polly.Pcl.Specs
  Discovered:  Polly.Pcl.Specs
  Starting:    Polly.Pcl.Specs
  Finished:    Polly.Pcl.Specs
=== TEST EXECUTION SUMMARY ===
   Polly.Pcl.Specs  Total: 1386, Errors: 0, Failed: 0, Skipped: 0, Time: 109.365s
Finished executing task: __RunTests

What to do with the PCL.Specs project

Great question. (We left that targeting PCL when adopting .NET Standard 1.0, to be sure we were serving our then PCL users correctly, but it's well past time to move on 😁 )

What are your thoughts on a replacement runtime for the specs proj which exercises the Polly.NetStandard11 offering? I would happy that we still judiciously choose a single runtime target against which to test Polly.NetStandard11: netcoreapp1.1, as FluentValidation use, seems as good a choice as any. Open to views though ...

(I have seen projects - such as FluentAssertions in the past - add test projects for all relevant runtimes, but they've scaled that back again now given how broad .NetStandard is 😄 . ).


It would be totally awesome if you had time to looks at those. All contribs get credit in the ReadMe, btw!

Thanks again for your contribution to Polly!

Properties element in the Net40Async dependency for Specs is triggering a rebuild.
@Kesmy
Copy link
Contributor Author

Kesmy commented Jul 21, 2017

Build and test issues should now be fixed, though it appears the build server can't locate msbuild.

@Kesmy
Copy link
Contributor Author

Kesmy commented Jul 21, 2017

@reisenberger I can't get tests passing on my machine with the netcoreap1.1 re-target.

It looks like that will also require more substantial changes to the build, including adding dotnet build/dotnet test steps specifically for the re-targeted Specs project. I'm not terribly familiar with Cake or Appveyor, so I'm not sure exactly what will need to happen to get dotnet tooling running.

I'm not averse to giving it a shot, but I don't know if it belongs in this PR given that updating the project formats have so far required no code changes.

Note that the netcoreapp1.1 retarget was just a test. I'm not familiar enough with the differences between the various "new" .NET targets to make any educated suggestions about which one is most appropriate.

@reisenberger
Copy link
Member

@Kesmy Big thanks again for this contribution, which is incredibly useful! I am happy to dive in and have a closer look what we can do with the existing PCL.Specs.

@Kesmy
Copy link
Contributor Author

Kesmy commented Jul 22, 2017

@reisenberger It turns out that I completely overlooked the CollectionBehavior attribute that is decorating PCL.Specs. I've added it and tests now pass.

I suddenly have a lot of free time on my hands, and will see about updating the Cake script to handle the netcoreapp tests tomorrow, then will push again.

@Kesmy
Copy link
Contributor Author

Kesmy commented Jul 22, 2017

@reisenberger If you have no reservations with targeting netcoreapp1.1 in specs, this should now be ready to go.

@reisenberger
Copy link
Member

@Kesmy Awesome! This is performing well, both in VS2017 and the cake build. I'll close-read the file changes, as last check, later in the week. Thx for all yr contribution!

(May not get merged for few days, just while we first push out a handful of minor in-prog fixes 267, 265, 270. I plan that this conversion to VS2017 format/builds then forms a separate release - only out of an abundance of caution, so that we can isolate any issues eg from the build change, to that one difference; though not expecting any. Those other PRs entirely orthogonal to yours, so I don't expect any merge conflicts.)

@reisenberger reisenberger merged commit a7021c5 into App-vNext:master Aug 5, 2017
@reisenberger reisenberger added this to the v5.3.1 milestone Aug 5, 2017
@Kesmy Kesmy deleted the convert-project-format-to-msbuild15 branch August 5, 2017 22:58
@reisenberger
Copy link
Member

@Kesmy Thanks for your awesome contribution to Polly! Credit added for your contribution to the project readme!

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

Successfully merging this pull request may close these issues.

3 participants