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

Move to .NET Standard 2.0 #307

Merged
merged 3 commits into from
Jan 9, 2019

Conversation

ViktorHofer
Copy link
Contributor

@ViktorHofer ViktorHofer commented Jul 20, 2018

Fixes #284

I'm from the .NET team and we are currently using this package in of our repos xunit-performance. As we are facing some package downgrades with the existing nuget package I upgraded it to target NS2.0.

Changes:

  • src assembly commandline.dll now targets netstandard2.0 and the xunit test library commandline.tests now targets netcoreapp2.0 (not 2.1 as I wasn't sure if the CI already uses the 2.1 SDK)
  • Updated dependencies (xunit, FluentAssertions)
  • Enable test run in CI. After updating the dependencies 20 tests failed because they are asserting of string which changed by xunit. I recommend removing such flaky tests. I disabled them for now: f151ae1
  • Replace Paker with the new in-built PackageReference and copy the 4 used files manually into the project to avoid adding new nuget dependencies
  • Replace nuspecs with the new in-built csproj attributes
  • Converge Test projects
  • Update build scripts

Requirements:

  • Visual Studio 2017
  • .NET Core SDK >= v2.0

Please let me know if you have any questions.

@AlexeiScherbakov
Copy link

Can you add multitargeting for .NET Standard and Full .NET Framework (not only netstandard)?
Described in:
https://docs.microsoft.com/en-us/dotnet/standard/frameworks
https://docs.microsoft.com/en-us/dotnet/core/porting/project-structure

For example:
netstandard2.0;net47

Why? When target only netstandard2.0 it brings many dependencies which are included in full framework.

@ViktorHofer
Copy link
Contributor Author

Why? When target only netstandard2.0 it brings many dependencies which are included in full framework.

This is resolved with >=net472 when referencing a netstandard2.0 library as it is the first version that doesn't need an additional support targeting pack to fully satisfy the netstandard2.0 surface area.

@AlexeiScherbakov
Copy link

AlexeiScherbakov commented Jul 26, 2018 via email

@ViktorHofer
Copy link
Contributor Author

This is why MS make multitarget in projects. Some library must exist in
most major .NET versions

Wrong. The reason why we added multitargeting is convenience. Instead of having several different project files you now just have one and cross-compile.

Some library must exist in most major .NET versions

That's what .NET Standard is for.

I don't have a strong opinion. I can add the net470 tfm is you want so. But first I would like to see a response from one of the project maintainers to understand if and when they are willing to merge this PR.

@ericnewton76
Copy link
Member

ericnewton76 commented Jul 30, 2018

@ViktorHofer Thanks for your work on this PR. Lot of changes but I'm thinking a lot of them are definitely "for the better", since NetStandard-2.0 baseline removes a lot of unnecessary framework probing.

Although I do have exactly one issue of trying to get the library to run on NetFramework-3.5 but it is just a single ask for the time being.

@nemec and i are evaluating this and I was thinking of bumping to v3 to incorporate this change.

Although I kinda wanted to remove some of the (weird-to-C#-people) F#-isms and introduce a fluent interface similar to Commander (https://www.npmjs.com/package/commander) for the v3 bump, we can probably reserve that for the v4 bump instead.

@AlexeiScherbakov
Copy link

AlexeiScherbakov commented Jul 30, 2018 via email

@ViktorHofer
Copy link
Contributor Author

ViktorHofer commented Aug 5, 2018

@ericnewton76 great to hear. Please let me know what's necessary to get this merged. I really would like to get this into master asap as we internally depend on this library and right now using a forked release.

EDIT:

Although I do have exactly one issue of trying to get the library to run on NetFramework-3.5 but it is just a single ask for the time being.

Please ask yourself if you really want this. Servicing net35 is a HUGE burden and limits yourself in numerous ways.

@ericnewton76
Copy link
Member

@ViktorHofer merged your PR into a specific branch: ViktorHofer-netstandard20, just a few questions:

Just wondering whats the correct course of action on these two:

  • I see several //[Fact] and xunit warns that the associated test should be marked with Fact
  • Same with //[Theory],

@ViktorHofer
Copy link
Contributor Author

That are the disabled tests that assert on console outputs that vary. (described in top post). I disabled them with this commit: f151ae1

I suggest removing these tests completely or rewriting them. If you want I can also do that, as you wish.


github gsscoder/CSharpx src/CSharpx/Maybe.cs
github gsscoder/CSharpx src/CSharpx/EnumerableExtensions.cs
github gsscoder/railwaysharp src/RailwaySharp/ErrorHandling.cs
Copy link
Member

@ericnewton76 ericnewton76 Aug 13, 2018

Choose a reason for hiding this comment

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

I'm not sure we can get rid of paket so easily, these files 3 files (Maybe.cs, EnumerableExtensions.cs, ErrorHandling.cs) get pulled in from their github project and compiled into the project. This was done to keep dependencies down and promote some code reuse.

And this is why paket works so much better than Nuget, because Nuget constantly made assumptions that were just not good in the real world.

Sigh. Not sure what to do here. I think the best approach is let nuget handle the nuget dependencies (which paket frankly usually handled better in most cases) and let paket handle the source code imports for the time being.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, I added these few files manually and removed Paket as with PackageReference and the new csproj sdk format it's unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no reason to use Paket here as the source code files probably won't change anymore (they didn't for multiple years). Also csharpx clearly states that the files can either be consumed via Paket or by manually copying them:

Source files are conceived to be manually added to other projects or by setting a reference via Paket. See these paket.dependencies and paket.references as example.

https://github.com/gsscoder/CSharpx#csharpx

I suggest to not make things more complicate as they need to be and just use nuget and get rid of Paket.

@ericnewton76
Copy link
Member

fyi, will finalize this later today.

@ViktorHofer
Copy link
Contributor Author

ok thanks. what about our Paket discussion?

@ViktorHofer
Copy link
Contributor Author

Any updates?

@ghost
Copy link

ghost commented Aug 29, 2018

This would be great to have, hitting NETStandard issues here

@ejsmith
Copy link

ejsmith commented Sep 6, 2018

This is a huge improvement and I would love to see this get merged in ASAP as well. Thanks!

@moh-hassan
Copy link
Collaborator

moh-hassan commented Sep 16, 2018

CSharpx is still pre-release since three years. csharpx

So, copying files is better than paket do that job.
With the integration of msbuild 15.1 with nuget and using PackageReference are becoming better than using Paket.

@ViktorHofer
Copy link
Contributor Author

ping @ericnewton76

@ericnewton76
Copy link
Member

Sorry, ran into a hellacious work issue just before a little R&R.

I'll try to push this out sometime this week.

@SolidSoftwareServices
Copy link

please generate nuget for .net standard 2.0

@ghost
Copy link

ghost commented Oct 20, 2018

@ericnewton76 please please please merge
#351

@moh-hassan
Copy link
Collaborator

moh-hassan commented Oct 21, 2018

I did git clone to the project and merged the branch 'ViktorHofer-netstandard20' on my machine.

in vs2017.8
The compilation is successful
The unit test is successful for total 326 unit test cases.

I tested the project for multi-target (net45 and netstandard2.0) and compilation/unit test pass successfully.

The Conflicting files:

The file: 'tests/CommandLine.Tests/CommandLine.Tests.csproj': no problem because it's the old project file.
The file 'tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs' is expected because there are many modification due to the breaking change of FluentAssertions library version "5.4.1" (full support for netstandard2.0)

For example, the line#127

       ((NotParsed<Options_With_Required_Set_To_True>)result).Errors.ShouldBeEquivalentTo(expectedResult);

is modified by ViktorHofer to

        ((NotParsed<Options_With_Required_Set_To_True>)result).Errors.Should().BeEquivalentTo(expectedResult);

So I suggest merging 'ViktorHofer-netstandard20' branch, resolving the confliction based on the above notes and select (theirs) of ViktorHofer modifications.

The auto generated package v3.0.0:

<package xmlns="http://schemas.microsoft.com/packaging/2012/06/nuspec.xsd">
  <metadata>
	<id>CommandLineParser</id>
	<version>3.0.0</version>
	<title>Command Line Parser Library</title>
	<authors>gsscoder,nemec,ericnewton76</authors>
	<owners>gsscoder,nemec,ericnewton76</owners>
	<requireLicenseAcceptance>false</requireLicenseAcceptance>
	<licenseUrl>https://raw.githubusercontent.com/gsscoder/commandline/master/doc/LICENSE</licenseUrl>
	<projectUrl>https://github.com/gsscoder/commandline</projectUrl>
	<iconUrl>https://raw.githubusercontent.com/commandlineparser/commandline/master/art/CommandLine20.png</iconUrl>
	<description>Terse syntax C# command line parser for .NET.  For FSharp support see CommandLineParser.FSharp.  The Command Line Parser Library offers to CLR applications a clean and concise API for manipulating command line arguments and related tasks.</description>
	<copyright>Copyright (c) 2005 - 2018 Giacomo Stelluti Scala &amp; Contributors</copyright>
	<tags>command line commandline argument option parser parsing library syntax shell</tags>
	<dependencies>
	  <group targetFramework=".NETFramework4.5" />
	  <group targetFramework=".NETStandard2.0" />
	</dependencies>
  </metadata>
</package>

@ejsmith
Copy link

ejsmith commented Oct 22, 2018

Ugh. Really would love to have this. This is a blocking issue for anyone wanting to use this library from .net core.

@ericnewton76
Copy link
Member

@moh-hassan oh man, thanks for doing that pull, I've been unable to find some time to devote to it lately.

This really helps me out, knowing that somebody's been able to independently verify it (I have to get latest NetCore even installed yet)

Will respond back here when I can pull a few hours for this. Should be able to this week! ::Fingers crossed::

@moh-hassan
Copy link
Collaborator

@ericnewton76
Glad to hear that Netstandard2.0 is coming soon :)
Let me know for any help needed to publish Netstandard2.0

@TethrRichardS
Copy link

bump

@ghost
Copy link

ghost commented Nov 16, 2018

@ericnewton76 Thanks for such a great library. Is there any ETA on when this will be merged? Currently blocked with netstandard issue

@bdominguez
Copy link

Bump, any news?

@gitfool
Copy link

gitfool commented Dec 15, 2018

Please merge this and release a new NuGet package. 🍺

@ViktorHofer
Copy link
Contributor Author

@ericnewton76 what's the status? I feel kind of neglected.

@ejsmith
Copy link

ejsmith commented Jan 3, 2019

At some point someone is just going to have to fork this repo and publish a new NuGet package. 😕

@moh-hassan
Copy link
Collaborator

@ericnewton76
What the status of netstandard2.0 ?
Can you share your plan for publishing it .
We have issues in netcoreapp2.x when using netstandard1.5 version.

@bdominguez
Copy link

Any news?

@ericnewton76
Copy link
Member

Sorry I've been extremely tied up. Been working multiple nights til at least 2am (last night 5am) trying to get a release out for work.

@ericnewton76 ericnewton76 merged commit bf05d8e into commandlineparser:master Jan 9, 2019
@moh-hassan
Copy link
Collaborator

Thank you very much @ericnewton76 for merging and Congratulations Netstandard2 for the community.

@ericnewton76
Copy link
Member

Newest version supporting Net Standard 2.0 is now built and published.

https://www.nuget.org/packages/CommandLineParser/2.4.3

https://github.com/commandlineparser/commandline/releases/tag/v2.4.3

@gitfool
Copy link

gitfool commented Jan 10, 2019

Thanks, @ericnewton76. Don't burn out!

@ericnewton76
Copy link
Member

ericnewton76 commented Jan 10, 2019

@gitfool Thanks man, lol. I say, as I continue on at 12:30am, probably for another couple of hours... joys of working from home and somewhat overpromising complex software... unrelated to this of course, @ViktorHofer did most of the work. I don't think I'd have gotten to the whole conversion process in a reasonable amount of time... lol

@UlyssesWu
Copy link

I also hope there will be multi-targeting for the nuget package - I think netstandard2.0;net461 should work but I hope the netfx version can be lower (e.g. net45). Or, maybe there could be 2 separate packages, one for .NET Standard (netfx>=472 & netcore) and one for .NET fx (netfx < 472 & maybe netfx < 4).

The point is, .NET fx 4.7.2 is not that popular especially in those old Win7 users. I'm not going to ask every user to install .NET fx 4.7.2 since it's not actually necessary for our functionality. when I set lower .NET fx versions for my project (net461), this lib adds so many System.*.dll to my release. I understand these dlls are just for redirection and are small in size, but it still looks redundant.

@ViktorHofer
Copy link
Contributor Author

I'm not going to ask every user to install .NET fx 4.7.2

Most of your customers/users should have net472 already installed as it's deployed over Windows Update. I don't see any reason to target a lower netfx version?

@ViktorHofer ViktorHofer deleted the netstandard20 branch April 23, 2019 15:19
@UlyssesWu
Copy link

Well, if a user is still using Win7, you can imagine he/she won't even turn on Windows Update to let MS notice him/her to update to Win10.

Thanks for the reply, and I'm going to handle this problem (compile a net46 version) by myself.

@ViktorHofer
Copy link
Contributor Author

Well, if a user is still using Win7, you can imagine he/she won't even turn on Windows Update to let MS notice him/her to update to Win10.

I wouldn't recommend that as you would be an easy target for tons of exploits. Being online without having the latest security patches (which includes .NET Framework) is like using the metro without having a ticket - risky.

@UlyssesWu
Copy link

You're definitely right, but some users may just choose to install minimal security updates without any functionality updates like .net, or they uninstall that for some reasons IDK... Anyway, I don't think I can just rely on Windows Update to install netfx 4.7.2 for everyone. And, I would like to see more users using my programs, rather than switching to others just because I require them to install netfx 4.7.2 which has seldom things to do with the actual functionality.

@ViktorHofer
Copy link
Contributor Author

I see. Why not use .NET core instead with which you can deploy the framework together with your app?

@UlyssesWu
Copy link

UlyssesWu commented Apr 23, 2019

The same reason - it looks redundant with many files, and bigger.

Thank you for listening to my complaints. Targeting to lower .net versions is a compromise and I should just do it by myself. As a developer, I would be happier if my program runs perfectly from WinXP to Win10, rather than "although .net 4.5 is enough for all functions in my program, you still need .net 4.7 if you don't want to mess up with a bunch of files".

@ViktorHofer
Copy link
Contributor Author

The same reason - it looks redundant with many files, and bigger.

You probably want to wait for .NET Core 3.0 where you can link everything down to a single file.

Thanks for developing such a nice library anyway!

I'm not the author 😁

@UlyssesWu
Copy link

Ah, I fogot what Author means in github issues. Plus there is also a Contributor label. Sorry for that.

But I'm also glad that I can talk about these with a .net team member!

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.

Target .NET Standard 2.0
10 participants