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

VS 2017 / dotnet 1.0.0 tooling #939

Merged
merged 20 commits into from Mar 9, 2017

Conversation

Projects
None yet
5 participants
@adamchester
Copy link
Member

adamchester commented Feb 11, 2017

Please don't merge. This just aims to see what it will take to move Serilog to RC4+ tooling.

@adamchester

This comment has been minimized.

Copy link
Member Author

adamchester commented Feb 11, 2017

A single test is failing and I don't have any clue why.

Failed   Serilog.Tests.Context.LogContextTests.ContextPropertiesCrossAsyncCalls
Error Message:
 System.Collections.Generic.KeyNotFoundException : The given key was not present in the dictionary.
Stack Trace:
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at Serilog.Tests.Context.LogContextTests.<ContextPropertiesCrossAsyncCalls>d__3.MoveNext() in C:\projects\serilog\test\Serilog.Tests\Context\LogContextTests.cs:line 108
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)

It could be that I've missed a build define somewhere?

@adamchester

This comment has been minimized.

Copy link
Member Author

adamchester commented Feb 11, 2017

On the travis side, the dotnet flag doesn't support the RC4 tools yet.

travis-ci/travis-ci#7301

@skomis-mm

This comment has been minimized.

Copy link

skomis-mm commented Feb 14, 2017

@adamchester , yep, dotnet migrate seems to be buggy so there are missing constants for netstandard1.3. These should be added to match project.json:

<PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard1.3' ">
    <DefineConstants>$(DefineConstants);ASYNCLOCAL;HASHTABLE</DefineConstants>
</PropertyGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.3' ">
  <PackageReference Include="System.Collections.NonGeneric" Version="4.0.1" />
</ItemGroup>

adamchester added some commits Feb 14, 2017

travis: use `osx_image` = `xcode7.3`,
because `xcode7.2` is deprecated
@adamchester

This comment has been minimized.

Copy link
Member Author

adamchester commented Feb 14, 2017

Thanks @skomis-mm! I think you were right!

@adamchester

This comment has been minimized.

Copy link
Member Author

adamchester commented Feb 14, 2017

Now looking for a way to make OpenCover find the code coverage correctly. Notice in the build log that no coverage results were found.

This might an issue to follow: OpenCover/opencover#601

adamchester added some commits Mar 4, 2017

@adamchester

This comment has been minimized.

Copy link
Member Author

adamchester commented Mar 4, 2017

OK I think it might be worthwhile getting some scrutiny on this now.

Here are some issues worth considering:

  • Do we even want VS 2017 tooling? I assume yes, otherwise I wouldn't have bothered. However, it might be reasonable to wait for some hypothetical future version ?
    • I hear no objections - I think we want it
  • previously we produced a Serilog nupkg that directly depended on some of the platform assemblies. With the updated tooling, this seems to be discouraged (e.g. implicitly references netstandard.library). If we do what the new tooling encourages, does that require a major version bump?
  • Is dotnet cli 1.0.0-rc4-004771 going to be the right version for VS 2017?
    • No it isn't - now upgraded to 1.0.0

Here are some other things of note:

  • Looks like build failures from build.sh aren't propagating out of the script.
    • Separate issue ouside of this PR?
  • Removed OpenCover/CodeCov because it doesn't work with these tools yet
  • Changed the way osx/linux build.sh grabs the dotnet CLI into ./.dotnetcli
  • Perf tests: might need to upgrade to BenchmarkDotNet 0.10.3 + netcoreapp1.1. They are failing on OSX/Linux now.
@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Mar 5, 2017

previously we produced a Serilog nupkg that directly depended on some of the platform assemblies. With the updated tooling, this seems to be discouraged (e.g. implicitly references netstandard.library). If we do what the new tooling encourages, does that require a major version bump?

Would this change mean that non-.NET Core users end up with a massive dependency install footprint? IIRC the non-.NET Core/.NET Standard targets don't reference the new packages because it results in a massive installation.

@adamchester

This comment has been minimized.

Copy link
Member Author

adamchester commented Mar 5, 2017

@nblumhardt I don't know what the pros or cons are for continuing to reference individual pieces vs. referencing the full netsandard.library metapackage. Here are some further details:

Maybe we should try to keep doing what we've been doing, until there is clear reason to change?

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Mar 7, 2017

It'd be nice to try the new tooling, but I think we probably will get there faster if we use DisableImplicitFrameworkReferences (thanks for the link) and set framework assemblies/package references manually 👍

adamchester added some commits Mar 8, 2017

@adamchester adamchester changed the title [WIP] RC4 tooling [WIP] VS 2017 / dotnet 1.0.0 tooling Mar 8, 2017

@adamchester adamchester changed the title [WIP] VS 2017 / dotnet 1.0.0 tooling VS 2017 / dotnet 1.0.0 tooling Mar 8, 2017

@adamchester

This comment has been minimized.

Copy link
Member Author

adamchester commented Mar 8, 2017

I think this is ready, another pair of eyes would be good.

image

@merbla

This comment has been minimized.

Copy link
Contributor

merbla commented Mar 8, 2017

@adamchester any chance of a squash commit?


<PropertyGroup>
<Description>Simple .NET logging with fully-structured events</Description>
<VersionPrefix>2.4.1</VersionPrefix>

This comment has been minimized.

@merbla

merbla Mar 8, 2017

Contributor

@adamchester @nblumhardt does this warrant 2.5.x? No functional changes?

This comment has been minimized.

@adamchester

adamchester Mar 8, 2017

Author Member

I'm expecting no functional changes. Either way, we will get a new -dev-0000x build and can do further checking right?

This comment has been minimized.

@nblumhardt

nblumhardt Mar 8, 2017

Member

Seems like starting with 2.4.x is pretty reasonable 👍

@adamchester

This comment has been minimized.

Copy link
Member Author

adamchester commented Mar 8, 2017

@merbla sure, doesn't bother me 👍

@merbla

This comment has been minimized.

Copy link
Contributor

merbla commented Mar 8, 2017

@adamchester great work here! Will be interesting to know your thoughts on the best conversion process for sinks etc.

@adamchester

This comment has been minimized.

Copy link
Member Author

adamchester commented Mar 8, 2017

@merbla here's one I started on; serilog/serilog-sinks-email#32

That's just a few small version bumps away from 1.0.0 too.

It's pretty straight forward actually.

<AssemblyName>Serilog</AssemblyName>
<AssemblyOriginatorKeyFile>../../assets/Serilog.snk</AssemblyOriginatorKeyFile>
<SignAssembly>true</SignAssembly>
<PublicSign Condition=" '$(OS)' != 'Windows_NT' ">true</PublicSign>

This comment has been minimized.

@nblumhardt

nblumhardt Mar 8, 2017

Member

What's this odd condition?

This comment has been minimized.

@adamchester

adamchester Mar 8, 2017

Author Member

It was added by dotnet migrate. See dotnet/cli#4309 and dotnet/cli#4311

Without this, I get:
screen shot 2017-03-08 at 16 50 31

This comment has been minimized.

@adamchester

adamchester Mar 8, 2017

Author Member

Actually I'm not sure now - I don't know what to do with this.

screen shot 2017-03-08 at 16 57 31

@skomis-mm
Copy link

skomis-mm left a comment

nit changes to allow project editing without unloading first

Serilog.sln Outdated
Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "Serilog", "src\Serilog\Serilog.xproj", "{803CD13A-D54B-4CEC-A55F-E22AE3D93B3C}"
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{791D4267-0D6F-4FDF-80F2-11F4E793B0F2}"
EndProject
Project("{13B669BE-BB05-4DDF-9536-439F39A36129}") = "Serilog", "src\Serilog\Serilog.csproj", "{AB00B377-9F1E-4D4B-B6B0-B95F53BCAEF1}"

This comment has been minimized.

@skomis-mm

skomis-mm Mar 8, 2017

"Serilog" should have GUID = {9A19103F-16F7-4668-BE54-9A1E7A4F7556} (on the left side) otherwise you'll loose the ability to edit project without unloading first in VS.

Serilog.sln Outdated
EndProject
Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "Serilog.Tests", "test\Serilog.Tests\Serilog.Tests.xproj", "{3C2D8E01-5580-426A-BDD9-EC59CD98E618}"
Project("{13B669BE-BB05-4DDF-9536-439F39A36129}") = "TestDummies", "test\TestDummies\TestDummies.csproj", "{37EF0B5E-0363-4A47-AF3E-51FA6E79E3CF}"
EndProject

This comment has been minimized.

@skomis-mm

skomis-mm Mar 8, 2017

Same as above - {9A19103F-16F7-4668-BE54-9A1E7A4F7556} on the left.

Serilog.sln Outdated
EndProject
Project("{8BB2217D-0F2D-49D1-97BC-3654ED321F3B}") = "TestDummies", "test\TestDummies\TestDummies.xproj", "{2BB12CE5-C867-43BD-AE5D-253FE3248C7F}"
Project("{13B669BE-BB05-4DDF-9536-439F39A36129}") = "Serilog.Tests", "test\Serilog.Tests\Serilog.Tests.csproj", "{B11B911D-977A-42CE-900A-596CF59F6FFA}"
EndProject

This comment has been minimized.

@skomis-mm

skomis-mm Mar 8, 2017

Same as above - {9A19103F-16F7-4668-BE54-9A1E7A4F7556} on the left.

Use magic sln project guids,
so that VS features like live csproj file editing work correctly.
@adamchester

This comment has been minimized.

Copy link
Member Author

adamchester commented Mar 8, 2017

Well nit'ed @skomis-mm - that was actually bugging me 👍

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Mar 8, 2017

Thoughts on Travis? Shall we drop off the OS-X target?

@adamchester

This comment has been minimized.

Copy link
Member Author

adamchester commented Mar 9, 2017

Yeah I think we need to drop OSX builds until Travis OSX is working fast and stable again.

@nblumhardt

This comment has been minimized.

Copy link
Member

nblumhardt commented Mar 9, 2017

Okay, I'm feeling lucky. :shipit: ???

@adamchester

This comment has been minimized.

Copy link
Member Author

adamchester commented Mar 9, 2017

:shipit: (to dev 😸)

@nblumhardt nblumhardt merged commit 9b581e3 into serilog:dev Mar 9, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adamchester adamchester deleted the adamchester:rc4-tooling branch Mar 16, 2017

@johnduhart

This comment has been minimized.

Copy link
Contributor

johnduhart commented on e674726 Mar 27, 2017

@adamchester For somebody following along at home with the transition to MSBuild based projects, what made you choose to explicitly reference the framework libs?

This comment has been minimized.

Copy link
Member Author

adamchester replied Mar 27, 2017

@nblumhardt nblumhardt referenced this pull request Jun 5, 2017

Merged

2.5.0 Release #980

@adamchester adamchester referenced this pull request May 7, 2018

Merged

Enable OSX builds on travis again #1158

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.