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

Support .NET Standard 2.0 and move to VS 2017 #2263

Merged
merged 14 commits into from
Sep 16, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Aug 24, 2017

Initial attempt for converting to new VS2017 csproj file format.

  • NetStandard 2.0
  • Net45, Net40-client, Net35 (Moved net40 to net40-client nuget-lib-folder)
  • Silverlight 4, Silverlight 5, Windows Phone
  • XamarinIOS 1.0
  • MonoAndroid (Upgraded from 2.3 to 4.4)
  • OpenCover
  • Travis NetStandard 2.0
  • Travis Mono 5.2 Net45 (Ver. 5.2 needed for msbuild support)
  • Cherry picking from coreclr-branch (NLog 5.0 BETA)
  • Strong Name Version remains static (ver. 4.0.0.0)
  • xUnit 2.3.0-beta5 (BETA Needed for NetStandard 2.0)
  • xUnit 2.0 testing of net35 and net40-client
  • NLog-Docs-csproj (Maybe handled by GenerateDocumentationFile=true) -> not used
  • NLog-BuildDocPages -> not used for now
  • NLog-CheckSourceCode
  • NLog.Extended package
  • NLog.Schema package (XSD Generation for config-file-intellisense) -> Julian
  • NLog.Config package
  • NLog.Snippets package (Maybe it is replaced by https://github.com/NLog/NLogSnippetsVSIX)
  • NLog.shfbproj (Sandcastle help file) -> revert

Replaces #2175 and #2154 and resolves #2152, resolves #1954 and resolves #1953

@snakefoot snakefoot force-pushed the Vs2017NetCore2 branch 2 times, most recently from f179bb5 to 4958fab Compare August 24, 2017 20:39
@luigiberrettini
Copy link
Contributor

luigiberrettini commented Aug 24, 2017

Number one in using latest tools and contributing to projects with your expertise 👍

Just to avoid confusion I would rename this PR as "VS 2017 and .NET Standard 2.0"

@snakefoot snakefoot force-pushed the Vs2017NetCore2 branch 9 times, most recently from b371a2a to e8d1e40 Compare August 26, 2017 20:22
@snakefoot
Copy link
Contributor Author

@luigiberrettini You can now play around with this nuget-package:

https://ci.appveyor.com/project/nlog/nlog/build/4.5.0-beta5831/artifacts

@snakefoot snakefoot force-pushed the Vs2017NetCore2 branch 5 times, most recently from a32a49a to 457945d Compare August 26, 2017 21:17
@snakefoot
Copy link
Contributor Author

snakefoot commented Aug 26, 2017

@304NotModified It would be nice if this PR is merged before any other PR (When it looks stable), as the conflict rate is quite high with this one. So please delay any merging of any other PR.

@snakefoot snakefoot force-pushed the Vs2017NetCore2 branch 2 times, most recently from ae98ae6 to 05b4296 Compare August 26, 2017 21:32
@codecov
Copy link

codecov bot commented Aug 26, 2017

Codecov Report

Merging #2263 into master will increase coverage by <1%.
The diff coverage is 60%.

@@           Coverage Diff           @@
##           master   #2263    +/-   ##
=======================================
+ Coverage      82%     82%   +<1%     
=======================================
  Files         304     304            
  Lines       21312   21374    +62     
  Branches     2570    2576     +6     
=======================================
+ Hits        17407   17463    +56     
- Misses       3250    3256     +6     
  Partials      655     655

@snakefoot
Copy link
Contributor Author

snakefoot commented Aug 26, 2017

@304NotModified Seems that the AppVeyor-build-image for VS2017 includes NetCore2, but VS2017 has dropped support for Silverlight. And the AppVeyor-build-image for VS2015 does not include NetCore 2.0 SDK.

Any ideas how to solve this? Moving forward without Silverlight ? Am I missing a magic parameter?

Seems I found the solution. Just needed to specify some extra magic parameters for the Silverlight and Windows Phone build. New Nuget-package ready:

https://ci.appveyor.com/project/nlog/nlog/build/4.5.0-beta5853/artifacts

@snakefoot snakefoot force-pushed the Vs2017NetCore2 branch 7 times, most recently from 1d2579e to 54eaa52 Compare August 27, 2017 19:09
@snakefoot
Copy link
Contributor Author

Last question, any idea why appveyor shows only 4291 unit tests? It should be +8000 ?

No idea how xUnit and AppVeyor is working together. Will try and change my net35 + net40 hack, so it doesn't reuse the same net452-FilePath for the 3 tests (net452, net40-client + net35)

@snakefoot
Copy link
Contributor Author

snakefoot commented Sep 11, 2017

Seems the AppVeyor-logic for xUnit has two identifiers for the unit-test-assembly-name (HandleTestAssemblyStarting):

  • FileName of the unit-test-dll (Cannot change this because of InternalsVisibleTo)
  • TargetFrameworkAttribute of the unit-test-dll (Will require artificial build of net461 and net462, so it differs from net452)

Little annoying it is not possible to reuse the same unit-test-dll with different configuration options (and add a prefix/suffix to the unit-test-assembly-name.

@304NotModified
Copy link
Member

304NotModified commented Sep 11, 2017

FileName of the unit-test-dll (Cannot change this because of InternalsVisibleTo)

Can't we add multiple InternalsVisibleTo attributes?

@snakefoot
Copy link
Contributor Author

Well we can add multiple InternalsVisibleTo attributes?

Sure make a PR, that works, then I will merge it into this. Or you can just do it after having merged this to master :)

@luigiberrettini
Copy link
Contributor

The PowerShell error is caused by the launch of an external command and the handling of streams:
http://mnaoumov.wordpress.com/2015/01/11/execution-of-external-commands-in-powershell-done-right/

@snakefoot
Copy link
Contributor Author

snakefoot commented Sep 11, 2017

The PowerShell error is caused by the launch of an external command and the handling of stream

Trying a newer version of xunit.console.runner, instead of the one provided by AppVeyor. Maybe it is better at shielding the PowerShell from the rogue NLog-Unit-Tests. ** Update ** Nope didn't help

@snakefoot snakefoot force-pushed the Vs2017NetCore2 branch 2 times, most recently from 274d83b to 1726984 Compare September 11, 2017 22:49
@snakefoot
Copy link
Contributor Author

snakefoot commented Sep 11, 2017

Have now fixed the rogue NLog UnitTests that spat into Console.Error and Console.Out. Now the PowerShell can sleep at night.

@luigiberrettini
Copy link
Contributor

As for snippets:

  • VSIntegration contains files from 6 years ago
  • uninstall is manual
  • they should definitely be moved to another repo those build creates a NuGet package that people interested in snippets can download and use (maybe snipppets fro mi the NLog repo can be merged with what has been done I need the NLogSnippetsVSIX repo)

Since snippets are six years old I suppose they are not updated on each release and this PR could ignore including their NuGet package creation, but this should be confirmed by @304NotModified 😉

@304NotModified
Copy link
Member

Since snippets are six years old I suppose they are not updated on each release and this PR could ignore including their NuGet package creation, but this should be confirmed by @304NotModified 😉

yes we could ignore the Vsintergration now. Will check this in the list

@304NotModified
Copy link
Member

reminder for myself

  • tests results summary on Appveyor: "Total tests:" (1x) and "=== TEST EXECUTION SUMMARY ==" (3x)
  • tests results summary on Travis: "Total tests:" (1x) and "=== TEST EXECUTION SUMMARY ==" (1x)

@304NotModified 304NotModified merged commit f747cad into NLog:master Sep 16, 2017
@304NotModified
Copy link
Member

304NotModified commented Sep 16, 2017

merged! Great job @snakefoot ! thanks! thanks @luigiberrettini ! 🎉

@snakefoot
Copy link
Contributor Author

@304NotModified Glad it is done. Not a funny PR to get through. Lots of old skeletons suddenly came jumping out of the closet. And the VS2017 tooling was not super obvious when all sources were saying "always use the new dotnet command".

I guess we should checkout if nUnit or MsTest2 (or something else) would perform better than xUnit, so the build-time could be reduced.

@304NotModified
Copy link
Member

I think we could win some time by disabling the xunit warnings (could be disabled I assume). Also the build log is then more readable ;)

@i02coroj
Copy link

Hi all.
Sorry if this is a stupid question.
I'm working right now in a .Net Standard 2.0 project. So I would need use this last NLog version supporting it.
But looking in Nuget gallery I see the package NLog.Web.AspNetCore is not updated since more than 3 months ago, so it obviously doesn't contain all this new stuff.

Do you have in mind upload it? How could I use it in the meantime? Am I forced to include the libraries inside my project? Thank yoj.

@snakefoot
Copy link
Contributor Author

snakefoot commented Sep 26, 2017

@304NotModified I think we could win some time by disabling the xunit warnings (could be disabled I assume).

I think the reason for the slower builds is the change to VS2017 AppVeyor-Build-Image. These are the build-timings on Travis, before and after the change to VS2017:

Before: https://travis-ci.org/NLog/NLog/builds/262388322 - 82.205 (NLog.UnitTests Total: 2042)
After: https://travis-ci.org/NLog/NLog/builds/280157224 - 85.508s (NLog.UnitTests Total: 2135)

Maybe the AppVeyor-build-servers that supports VS2017-images are configured differently?

@304NotModified
Copy link
Member

at least 2017 preview is slower: source

@304NotModified 304NotModified modified the milestones: 4.5, 4.5 beta 1 Sep 30, 2017
@snakefoot snakefoot mentioned this pull request Nov 23, 2017
@snakefoot snakefoot modified the milestones: 4.5 beta 1, 4.5 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants