Skip to content

Fixed the Problem with .Net Standard 2.0#282

Merged
tathamoddie merged 19 commits intoTestableIO:masterfrom
TclasenITVT:standard2
Jul 10, 2018
Merged

Fixed the Problem with .Net Standard 2.0#282
tathamoddie merged 19 commits intoTestableIO:masterfrom
TclasenITVT:standard2

Conversation

@TclasenITVT
Copy link
Copy Markdown
Contributor

@TclasenITVT TclasenITVT commented Mar 21, 2018

I only added a compile target for .Net Standard 2.0 to the System.IO.Abstractions.csproj and System.IO.Abstractions.TestHelpers.csproj plus I updated the Version of used Nuget-Packages.
#281

@richardwerkman
Copy link
Copy Markdown

This problem is breaking things for me, could somebody merge this and update nuget?


<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' ">
<PackageReference Include="System.IO.FileSystem.AccessControl">
<Version>4.4.0</Version>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I propose to use version 4.5.0 as it removes DLLs for many targets, but offers single netstandard2.0 for all these platforms added since 4.3.0. It's good that you haven't used 4.3.0 which won't work on Android

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed it to use 4.5.0

@pasn
Copy link
Copy Markdown

pasn commented Jul 4, 2018

Hmm... seems that we don't need netstandard2.0 target. This problem disappears when you update System.IO.FileSystem.AccessControl from version 4.3.0 (minimum required for this library) to 4.4.0 or newer. Starting from 4.4.0, System.Security.Claims and System.Security.Principal libraries are not required anymore.

So it seems like switching minimum required version of System.IO.FileSystem.AccessControl for netstandard1.4 to >=4.5.0 will fix it.

@tathamoddie
Copy link
Copy Markdown
Contributor

So this PR is now redundant / wrong approach?

@pasn
Copy link
Copy Markdown

pasn commented Jul 4, 2018

I would bump System.IO.FileSystem.AccessControl for netstandard1.4 to >=4.5.0 to remove warning for all targeting the library in netstandard1.4. Otherwise developers of netstandard2.0 libraries utilizing System.IO.Abstractions w/o specifying any version of System.IO.FileSystem.AccessControl will get these warnings.

@TclasenITVT
Copy link
Copy Markdown
Contributor Author

TclasenITVT commented Jul 4, 2018

Following @pasn suggestion removing NetStandard2.0 in favor of bumping up System.IO.FileSystem.AccessControl to 4.5.0 in NetStandard1.4 Target.

travis-ci build is failing because test contain not x-plat compatible test.

@jpreese
Copy link
Copy Markdown
Member

jpreese commented Jul 6, 2018

I agree with keeping Standard 1.4 if its possible. We want to keep the library targeting the lowest possible version when possible to be compatible with as many framework versions as we can.

However, I did some local testing with this. The current PR doesn't look like it fixes the issue described. When I switch to .NET Standard 2.0, I'm still seeing the conflict message.

Also, for further clarity, with the current release (2.1.0.185) I'm not seeing a conflict message with 1.4. So I wouldn't think changing the assembly version on AccessControl specifically for 1.4 would resolve anything.

Does it go away for you with these changes @TclasenITVT ?

@pasn
Copy link
Copy Markdown

pasn commented Jul 6, 2018

@jpreese this was never the intention of this PR to remove support for netstandard1.4. netstandard2.0 target was added in addition to existing one. Anyway, new target is now removed and I propose to merge it.

@tathamoddie
Copy link
Copy Markdown
Contributor

Note this comment from @davidfowl, where he says we should target both 1.x and 2.0: https://twitter.com/davidfowl/status/1014560244720996352?s=19

@pasn
Copy link
Copy Markdown

pasn commented Jul 6, 2018

Then maybe it is beneficial - many libraries do this, so it may make sense. Anyway, the original issue is solved.

@jpreese
Copy link
Copy Markdown
Member

jpreese commented Jul 6, 2018

@pasn I'm just a little confused with the PR as it is now. My experience is that 1.4 works without an issue right now. I'm not getting any warnings. if I target .NET Standard 2.0, the warnings start to appear.

If I apply this change to my local environment, 1.4 still works and 2.0 still has warnings. Am I missing something?

@TclasenITVT
Copy link
Copy Markdown
Contributor Author

I have a Piece of Software that targets .netstandard 2 and for testing purposes I switched to the newest version of the System.IO.Abstrations Nuget Package and I don't get any warnings. So I guess we don't need this PR anymore. I could readd the Target for .netstandard 2.0 If that is what you want?

@pasn
Copy link
Copy Markdown

pasn commented Jul 7, 2018

@jpreese What warnings do you have when switching from AccessControl 4.3 to 4.5?

Original bug was about 2 warnings and one of them was about System.Security.Principal.Windows which is no longer required. Do you have the same warnings and is this library referenced (indirectly) by some other nuget in your project?

@jpreese
Copy link
Copy Markdown
Member

jpreese commented Jul 7, 2018

@TclasenITVT that's actually a little surprising.. even with the latest System.IO.Abstractions package, I get warnings in a project that targets .NET Standard 2.0

Based on Fowler's twitter response, we probably need to target .NET Standard 2.0 (and keep 1.4 as well) since it's a warning specific to .NET Standard 2.0

@jpreese
Copy link
Copy Markdown
Member

jpreese commented Jul 7, 2018

@pasn I don't get any warnings until my project targets 2.0

When my project targets 2.0, I get

1>    There was a conflict between "System.Security.Principal.Windows, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" and "System.Security.Principal.Windows, Version=4.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a".
1>    There was a conflict between "System.Security.AccessControl, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" and "System.Security.AccessControl, Version=4.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a".

@jpreese
Copy link
Copy Markdown
Member

jpreese commented Jul 8, 2018

So to resolve this issue with .NET Standard 2, I think we need to add the target back.

@jpreese jpreese added the state: waiting for feedback Issues that are waiting for feedback label Jul 8, 2018
@TclasenITVT
Copy link
Copy Markdown
Contributor Author

TclasenITVT commented Jul 9, 2018

@jpreese I readded the netstandard2.0 target with my latest commit.
It is a library Project if that makes a difference but that would confuse me as It was the same project in which I started to use this Nuget Package and had the warnings myself in the first place. Basically the reason I started this PR.
I also don't get any warnings in my two console applications (one net framework 471 the other netcoareapp 2.1). Maybe there is a certain piece of code you need to use of System.IO.Abstractions that triggers the warnings because It itself is relying on the System.Security.AccessControl or System.Security.Principal.Windows.

@pasn
Copy link
Copy Markdown

pasn commented Jul 9, 2018

I can confirm that I have warnings mentioned in original issue #281 when creating new netstandard2.0 project. Sorry for confusion, but for my other netstandard2.0 project these warnings disappeared.

@pasn
Copy link
Copy Markdown

pasn commented Jul 9, 2018

@TclasenITVT I propose to merge ItemGroups for netstandard 1.4 and 2.0 like this:
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard1.4' or $(TargetFramework)' == 'netstandard2.0'">

Copy link
Copy Markdown
Contributor

@fgreinacher fgreinacher left a comment

Choose a reason for hiding this comment

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

I don’t think you need the explicit package references for netstandard2.0. Can you try just adding the new target and reverting the changes to the references?

@davidfowl
Copy link
Copy Markdown
Contributor

I came here to say the same thing. 4.3.0 versions of packages are for ns1.x. You should try ns2.0 without any dependencies and see if that works.

@TclasenITVT
Copy link
Copy Markdown
Contributor Author

TclasenITVT commented Jul 10, 2018

I tried it locally and could not get it to build. I guess that means we need the explicit references...
All Errors are about Types not found. (e.g. DirectorySecurity, FileSecurity etc just to name a few)

@davidfowl
Copy link
Copy Markdown
Contributor

It's possible you only need System.Security.AccessControl.

… kept the reference in the System.IO.Abstractions.csproj for the System.IO.FileSystem.AccessControl reference
@TclasenITVT
Copy link
Copy Markdown
Contributor Author

@davidfowl you were right.

</PropertyGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.4' ">
<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard2.0'">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Inconsistent whitespace in this block

@tathamoddie
Copy link
Copy Markdown
Contributor

Looking very close now!

@tathamoddie
Copy link
Copy Markdown
Contributor

Can we fix the whitespace (#282 (review)), and rebase it up onto the latest master? Then we can land it as a clean squash instead of the 17 commits we took to get here.

@tathamoddie tathamoddie merged commit 287d771 into TestableIO:master Jul 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state: waiting for feedback Issues that are waiting for feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants