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

MS Config and DependencyModel refs updated #96

Merged
merged 3 commits into from
Apr 11, 2018
Merged

MS Config and DependencyModel refs updated #96

merged 3 commits into from
Apr 11, 2018

Conversation

MV10
Copy link
Contributor

@MV10 MV10 commented Apr 10, 2018

Per #83, updated and tested references to Microsoft.Extensions.DependencyModel
and Microsoft.Extensions.Configuration.*. Updated .NET Standard from 1.6 to 2.0.

Confirmed both framework sample versions also run correctly. In all project files, old .NET Framework and .NET Standard refs were updated separately (conditional TFMs) to latest supporting versions. Also added a comment to the test project explaining how to run the multi-targeted tests from the command line (it was new to me, I suspect it is not commonly known).

@MV10 MV10 changed the title project files updated MS Config and DependencyModel refs updated Apr 10, 2018
@MV10
Copy link
Contributor Author

MV10 commented Apr 10, 2018

I've never used Travis, not sure why it blows up with a netstandard1.6 error... there is nothing left in any part of the project which references it. Everything else works, hopefully it's an easy fix.

Also, didn't bump the version, not sure whether you'd consider this a minor or patch change.

/usr/share/dotnet/sdk/2.1.4/Sdks/Microsoft.NET.Sdk/build/Microsoft.PackageDependencyResolution.targets(167,5): error : Assets file '/home/travis/build/serilog/serilog-settings-configuration/src/Serilog.Settings.Configuration/obj/project.assets.json' doesn't have a target for '.NETStandard,Version=v1.6'. Ensure that restore has run and that you have included 'netstandard1.6' in the TargetFrameworks for your project. [/home/travis/build/serilog/serilog-settings-configuration/src/Serilog.Settings.Configuration/Serilog.Settings.Configuration.csproj]

@adamchester
Copy link
Member

adamchester commented Apr 10, 2018

Hey @MV10, you should try changing buld.sh which is locked to netstandard1.6

@MV10
Copy link
Contributor Author

MV10 commented Apr 10, 2018

@adamchester Thanks! The file isn't in the SLN view and I didn't think to jump back to the file view. All good now, much appreciated.

Copy link
Member

@nblumhardt nblumhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just wondering if we need to define APPDOMAIN for the netstandard2.0 target for completeness, now? Or if we can remove it entirely? (Not sure why we use it in this lib.)

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="2.0.1" />
</ItemGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'net451' ">
<DefineConstants>$(DefineConstants);APPDOMAIN</DefineConstants>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the features behind #if APPDOMAIN now usable on .NET Standard 2.0?

@MV10
Copy link
Contributor Author

MV10 commented Apr 11, 2018

Since APPDOMAIN was specific to net451 I didn't give it any consideration. The only use is here. Frankly, I'm not really clear about what that bit of code is meant to accomplish, but all that System.IO path and directory stuff is Windows-specific, so I assume .NET Standard / Core doesn't need this, and I would expect the existing code can remain unchanged.

To be honest, my focus in this change was updating MS Config to v2, so as long as the DependencyModel stuff built and tested clean, I figured it was good. But I'm happy to expand on the PR if you think something else is needed.

@nblumhardt
Copy link
Member

Ah, great 👍 - that code enables using types without listing all of the assemblies in serilog:using keys on the full framework. It probably will kick in when targeting .NET Framework using the .NET Standard 2.0 targets, so perhaps we remove the #if and enable it on all platforms? I'm reasonably certain AppDomain.BaseDirectory is available on .NET Standard 2.

@MV10
Copy link
Contributor Author

MV10 commented Apr 11, 2018

@nblumhardt Good call, you're right. I'd moved netstandard2_0 to be the first TFM which makes VS default to that, so as a first look Intellisense was satisfied without the constant. Builds ok, tests pass, Travis is happy.

Not sure why I thought System.IO was framework-level rather than standard-API. I suppose I haven't had much call for raw file I/O lately. Anyway, I think we're good to go: newer libs and less code, can't argue with that!

@nblumhardt nblumhardt merged commit f396e87 into serilog:dev Apr 11, 2018
@nblumhardt
Copy link
Member

👍

We might have to see how the combined Framework 4.5.1/Standard 2.0 targeting works out in practice - have had some issues with this kind of setup in the past; shouldn't be any trouble dropping 4.5.1 support from 3.0.0 of this package if we end up having to.

Thanks for your efforts, @MV10 !

@MV10 MV10 deleted the netstandard2_0 branch April 12, 2018 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants