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

Update projects to .NET sdk, support netstandard2.0, drop tfms, etc #38

Merged
merged 61 commits into from
Mar 2, 2019

Conversation

enricosada
Copy link
Collaborator

@enricosada enricosada commented Feb 20, 2019

create a modern package, use .net sdk, cleanup infra, add ci.

Supported frameworks

add support for netstandard2.0

drop support for discontinued net40-client target framework.
it's not used anymore in .NET >= 4.5
ref https://docs.microsoft.com/en-us/dotnet/framework/deployment/client-profile

drop support for netstandard1.6, it's useful only in .NET Core v1.
replaced by netstandard2.0

drop support for .NETPortable( portable-net45+win8+wp8+wpa81 ) target framework.
the PCL profiles are replaced by .NET Standard.

  • the compiler defines PCL is not used anymore. The code is not yet removed
    because it's a big change, but because the compiler define is not set, the
    code is unused

Dependencies

pin FSharp.Core to be >= v4.3.4 for all target framework to simplify deps

Projects

add global.json to pin .net core sdk version to make build deterministic

upgrade form FSharp.NET.Sdk style (deprecated in .NET Core Sdk 1.0) to .NET Sdk fsproj

cleanup shared props and fsproj, using conventions of .NET Sdk:

  • refactor common properties in an automatically imported Directory.Build.props
  • move props near usage in projects
  • remove now useless FParsec.Samples.Common.targets
  • upgrade tests from netcoreapp2.0 to netcoreapp2.1

remove old VS11 projects, new projects works in VS

Build script

update pack.psi powershell script, to use dotnet commands instead of msbuild/nuget
nupkgs are generated in ~/bin/nupkg

CI

Add appveyor:

  • run pack.ps1
  • version suffix autogenerated by buildId/branch/PR. no suffix if is a tag (so just tag to generate a stable version)
  • generated nupkgs are in the artifacts tab of appveyor, ready to download

Add travis:

  • osx/ubuntu
  • run dotnet build on both Release and Release-LowTrust configurations
  • run tests only if netcoreapp. It can run also net45 test using mono but i am too lazy to write the if in the travis script (but the matrix is there)

Issues

fix #34 (support netstandard and net tfms)
fix #21 (just run dotnet commands, but replacing the powershell is easy too)

drop support for `.NETPortable`( `portable-net45+win8+wp8+wpa81` )  target framework
…rget framework

remove compiler define and code for `PCL_FPARSEC`

the compiler defines `PCL` is not used anymore.
The code is not yet removed because it's a big change, but because the compiler define is not
set, the code is unused
@buybackoff
Copy link

buybackoff commented Feb 27, 2019

I'd prefer to only sign the .Net framework assemblies, until there's a clear need to also sign the netstandard ones. (Shipping signed assemblies is a decision that can't be reverted without potentially breaking downstream projects.)

Signed assemblies do not create any problems for unsigned consumers, but unsigned ones are impossible to consume from signed ones. Clear need is when a netstandard assembly A is consumed by another netstandard assembly B which also targets .NET classic and netcore, and the B assembly is consumed by a .NET classic assembly C that is signed. If A (FParsec is this case) is not signed then B cannot be signed and C cannot consume it. Please make all assemblies signed and commit the key to the repo. I believe this is de facto a standard for open projects (e.g. Dapper, Nodatime).

Maybe we can string sign also netstandard because less maintenance (one if less)

Signed is much easier to maintain not only than than two but also than unsigned - you do it once, forget about it and never get issues/complaints that some signed assembly cannot use yours.

@enricosada
Copy link
Collaborator Author

enricosada commented Feb 27, 2019

I'd like to not enter the discussion about strong naming here, because is always really long. Just to be clear, for me is a big no.

What matter is that the fparsec net45 is strong signed, that's ok and must continue to be like that (it's out of scope for this PR).

I checked the built assemblies and are strong signed

>sn -vf lib\net45\FParsecCS.dll

Microsoft (R) .NET Framework Strong Name Utility  Version 4.0.30319.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Assembly 'lib\net45\FParsecCS.dll' is valid

@stephan-tolksdorf as a note, i tried in https://github.com/enricosada/fparsec/tree/split_prop to split the LowTrust in multiple fsproj (so FParsec.fsproj and FParsec.BigData.fsproj) to remove the need for multiple configurations.
But dunno, was not worth it, because we trade some issues for others.

So @stephan-tolksdorf for me this PR is ready

@cartermp
Copy link

Any discussion about strong naming should involve Paket vs. NuGet, Tabs vs. Spaces, and Vim vs. Emacs 😄

@buybackoff
Copy link

buybackoff commented Feb 27, 2019 via email

@stephan-tolksdorf
Copy link
Owner

@enricosada Thanks again for your work on this PR! I'm going to review it this weekend so that we can merge it.

I'm curious, why did you switch to conditionals based on TargetFramework from TargetFrameworkIdentifier in 63f5049 (after deleting the fallback initialization of TargetFrameworkIdentifier in 5231fbd)?

@enricosada
Copy link
Collaborator Author

Because the strong sign wasn’t enabled.

that property TargetFrameworkIdentity is set from a taarget later than the conditional evaluation, so was evaluated to false.

@stephan-tolksdorf
Copy link
Owner

that property TargetFrameworkIdentity is set from a taarget later than the conditional evaluation, so was evaluated to false.

I think that's why I originally set the TargetFrameworkIdentifier manually when it was empty in FParsec.Common.targets (before 5231fbd), but I'm fine with just using TargetFramework directly instead.

Do you still want to add the SourceLink support?

@enricosada
Copy link
Collaborator Author

Yes, i’ll add sourcelink later in another PR
This one is already big enough

@stephan-tolksdorf
Copy link
Owner

Great work @enricosada! I think this PR is ready to be merged.

I've made some minor adjustments. Let me know if you find any of them objectionable.

If you think the PR is ready, please squash the PR, give it a descriptive commit message and then rebase merge it with master. (I've invited you as a GitHub contributor.)

Thank you so much for your contribution!

@enricosada enricosada merged commit 61894fe into stephan-tolksdorf:master Mar 2, 2019
@enricosada
Copy link
Collaborator Author

Done, thanks @stephan-tolksdorf

@enricosada enricosada mentioned this pull request Mar 2, 2019
@rmunn
Copy link
Contributor

rmunn commented Mar 2, 2019

As the one who reported #21, I'm happy to see that someone tackled this. Thanks @enricosada!

@enricosada
Copy link
Collaborator Author

ref #39 to track sourcelink

@cartermp
Copy link

cartermp commented Mar 3, 2019

Great work @enricosada

@btrepp
Copy link

btrepp commented May 2, 2019

Is it possible to get this released as another RC?. Currently have issues with mixing netframework and netstandard libs.

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.

Assembly version mismatch, caused by netstandard1.6 support Cross-platform build, maybe with Paket+FAKE?
7 participants