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

add support for net40 and net45 #387

Closed
wants to merge 0 commits into from
Closed

add support for net40 and net45 #387

wants to merge 0 commits into from

Conversation

moh-hassan
Copy link
Collaborator

@moh-hassan moh-hassan commented Jan 10, 2019

@ericnewton76
Copy link
Member

Thanks for starting this. It was next on my list.

Looks like fsharp project wont work on anything prior to 4.5. We can probably split that build off for any <4.5.

I've been seeing requests for framework 3.5 support, although off the top of my head I can't think of how cumbersome that might be.

moh-hassan referenced this pull request Jan 30, 2019
Added explicit support for .NET 4.6.1 and .NET Core 2.0.
@ericnewton76
Copy link
Member

ericnewton76 commented Feb 1, 2019

This looks good, although the FSharp package is failing because FSharp isnt supported at the net40 level.

@moh-hassan Any ideas on how to prevent it from building when doing a net40 build? We can probably separate it out somehow. The build is getting complicated... lol

@ericnewton76 ericnewton76 added the merge-pending This PR looks good and most likely gets into next release label Feb 1, 2019
@moh-hassan
Copy link
Collaborator Author

moh-hassan commented Feb 1, 2019

Hi @ericnewton76 ,
You can use conditional build
Modify the xml project `CommandLine.csproj' and replace

	  <ItemGroup>
		<PackageReference Include="FSharp.Core" Version="4.5.1" Condition="'$(BuildTarget)' == 'fsharp'" />
	  </ItemGroup>

With Condition:

	  <ItemGroup Condition="'$(TargetFramework)' != 'net40' and '$(BuildTarget)' == 'fsharp'">
		<PackageReference Include="FSharp.Core" Version="4.5.1" Condition="'$(BuildTarget)' == 'fsharp'" />
	  </ItemGroup>

I generated Fsharp package by the command:

             msbuild /t:pack /p:BuildTarget=fsharp

I get a package: `CommandLineParser.FSharp.2.5.0.nupkg' in my modified pR
the package include net40,net45,net461 and netstandard2

Let me know i this done in your side, and ready for ping. I'm online now.

Can you check the generated package.
CommandLineParser.FSharp.2.5.0.nupkg.zip

@ericnewton76
Copy link
Member

ericnewton76 commented Feb 1, 2019 via email

@moh-hassan
Copy link
Collaborator Author

moh-hassan commented Feb 1, 2019

@ericnewton76
To check that all tests pass in both net461 and netcoreapp2.0, I modified the Test Project as multi-target project:

            <TargetFrameworks>net461;netcoreapp2.0</TargetFrameworks>

I run dotnet test and find 14 test fail in net461 due to the reasons we discussed in the issue-389

I fixed the failed test by using the solution I explained in the same issue

All tests pass Successfully in both net461 and netcoreapp2.0
The PR support: net40,net45,net461,netstandard2.0,netcoreapp2.0 and Fsharp fix is included.

The test pass in CI Appveyor

with the green success text:

NET461

Test run for C:\projects\commandline\tests\CommandLine.Tests\bin\Debug\net461\CommandLine.Tests.dll(.NETFramework,Version=v4.6.1)
....
Total tests: 335. Passed: 335. Failed: 0. Skipped: 0.
Test Run Successful.

NETCoreApp2.0

Test run for C:\projects\commandline\tests\CommandLine.Tests\bin\Debug\netcoreapp2.0\CommandLine.Tests.dll(.NETCoreApp,Version=v2.0
...
Total tests: 335. Passed: 335. Failed: 0. Skipped: 0.
Test Run Successful.

The confliction in the merge is:

	<<<<<<< master
		<TargetFrameworks>netstandard2.0;net40;net45;net461;netcoreapp2.0</TargetFrameworks>
	=======
		<TargetFrameworks>netstandard2.0; net461; netcoreapp2.0</TargetFrameworks>
	>>>>>>> master

which can be resolved manually
Let me know if i missed something to do.

@scriptator
Copy link

What's the status of this issue? It would be really great if it could be merged soon

@vgalka-sl
Copy link

@moh-hassan @ericnewton76
Is there any progress on this? Where does it stand? May I offer my assistance if needed?
We really need the library to be compatible with .NET 4.5 for our product.

@moh-hassan
Copy link
Collaborator Author

moh-hassan commented Apr 3, 2019

The PR is ready to be merged:
It pass the unit test in netcoreapp2.0 and net461 and[ CI Appveyor](https://ci.appveyor.com/project
/moh-hassan/commandline)
It support: netstandard2.0;net40;net45;net461;netcoreapp2.0

The current Conflicting files:
src/CommandLine/CommandLine.csproj

  	  <<<<<<< master	
   <TargetFrameworks>netstandard2.0;net40;net45;net461;netcoreapp2.0</TargetFrameworks>
  	=======
  		<TargetFrameworks>netstandard2.0; net461; netcoreapp2.0</TargetFrameworks>
  	>>>>>>> master

It Can be resolved to:

	<TargetFrameworks>netstandard2.0;net40;net45;net461;netcoreapp2.0</TargetFrameworks>

tests/CommandLine.Tests/Unit/Text/HelpTextTests.cs

  <<<<<<< master
  	public class HelpTextTests: BaseTest
  =======
  	public class HelpTextTests : IDisposable
  >>>>>>> master

can be resolved to:

public class HelpTextTests: BaseTest,IDisposable 

@ericnewton76
Is there any modification that I can do for merging the PR

@moh-hassan
Copy link
Collaborator Author

@vgalka-sl
The PR is ready to be merged after resolving the confliction as I explained above.

@vgalka-sl
Copy link

@vgalka-sl
The PR is ready to be merged after resolving the confliction as I explained above.

@moh-hassan Thank you! Seems Eric is busy, so we just have to wait.
Btw, maybe you can rebase and resolve the conflict yourself to make it "cleaner"?

@moh-hassan
Copy link
Collaborator Author

moh-hassan commented Apr 8, 2019

@ericnewton76
@vgalka-sl

I have to resolve an error (not related to the current PR) after rebasing , may be due to merging a previous commit:

A compilation error:
Error CS0102 The type 'ErrorType' already contains a definition for 'VersionRequestedError'

'VersionRequestedError' is duplicated in the same file: Error.cs in line63 and line68

Also, resolving the following compilation errors:

  • CS0160 A previous catch clause already catches all exceptions of this or of a super type ('Exception') CommandLine (commandline\src\CommandLine\Core\ReflectionExtensions.cs 113)
  • Error CS0103 The name 'InvalidAttributeConfigurationError' does not exist in the current context CommandLine (src\CommandLine\Core\ReflectionExtensions.cs 115)

Can you confirm these modifications?

Edit:
There is a compilation error of the last merge. Have a look to this discussion.

@moh-hassan
Copy link
Collaborator Author

The new PR#428 submitted by @Wind010 is fine and fix the issue of compilation errors and I can rebase Upstream after it's (PR#428) merged.

@moh-hassan
Copy link
Collaborator Author

Moving the PR to a separate branch in a new PR# 430

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-pending This PR looks good and most likely gets into next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants