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

Remove FSharp.Compiler.Tools and move to SDK based projects #3713

Merged
merged 7 commits into from Nov 22, 2019

Conversation

@forki
Copy link
Member

forki commented Nov 11, 2019

This removes the usages of FSharp.Compiler.Tools and moves Paket projects to the new dotnet SDK.

replaces #3569

@forki forki force-pushed the fullsdk branch 7 times, most recently from 2941449 to 2d063df Nov 11, 2019
@@ -24,7 +24,6 @@ NUGET
FSharp.Core (>= 4.1) - restriction: < netstandard1.6
FSharp.Core (>= 4.1.17) - restriction: >= netstandard1.6
NETStandard.Library (>= 1.6.1) - restriction: >= netstandard1.6
FSharp.Compiler.Tools (4.1.23)

This comment has been minimized.

Copy link
@forki

forki Nov 12, 2019

Author Member

/cc @cartermp :P

@forki forki force-pushed the fullsdk branch 21 times, most recently from c63f600 to 159be8c Nov 12, 2019
@@ -586,11 +586,6 @@ let ``#2694 paket fixnuspec should not remove project references``() =
| None -> Assert.Fail("Expected package to still contain the FSharp.Core reference!")
| Some s -> ignore s

// Should we remove Microsoft.NETCore.App?

This comment has been minimized.

Copy link
@forki

forki Nov 13, 2019

Author Member

@matthid sorry to bother you mate. This is the only integration test that is red. I tried to port everything to the SDK and somewhere here in this PR there is something that changes nuspec.Dependencies.Value.Length from 3 to 2.
The comment above the assertion is already very suspicious, but still I'm wondering what changed. Any ideas?

This comment has been minimized.

Copy link
@matthid

matthid Nov 14, 2019

Member

Can you you check and post what was in the list before and what is there now to see what has disappeared?

Github seems to make it a bit harder to see the code when force pushes are used apparently...

This comment has been minimized.

Copy link
@forki

forki Nov 14, 2019

Author Member

Dependencies:

[(PackageName ("library","library"), VersionRequirement (Minimum 1.0.0,No),
  ExplicitRestriction >= netcoreapp2.0);
 (PackageName ("FSharp.Core","fsharp.core"),
  VersionRequirement (Minimum 4.2.3,No), ExplicitRestriction >= netcoreapp2.0);
 (PackageName ("Microsoft.NETCore.App","microsoft.netcore.app"),
  VersionRequirement (Minimum 2.0.0,No), ExplicitRestriction >= netcoreapp2.0)]

so it's removing Microsoft.NETCore.App

This comment has been minimized.

Copy link
@matthid

matthid Nov 14, 2019

Member

I think Microsoft.NETCore.App is indeed special in a way.
Question is if we have added some kind of special casing for it in the code-base?
Seems like it was magically added before this PR, because in the lockfile of this test it is not even referenced, see https://github.com/fsprojects/Paket/blob/fullsdk/integrationtests/scenarios/i002694/before/paket.lock
So I guess it is "safe" that it is no longer there. I'm more wondering why it was there before this PR, which only really changes project setup?

Maybe some DEFINES are not like they where before?

This comment has been minimized.

Copy link
@forki

forki Nov 14, 2019

Author Member

Yes. I tried convert the compiler switches but maybe I lost one.
I also updated the sdk to 3.0.100. Maybe that's relevant?

This comment has been minimized.

Copy link
@forki

forki Nov 15, 2019

Author Member

ok confirmed. I updated the sdk in the test PR (see #3718) and the dependency is gone as well.

This comment has been minimized.

Copy link
@matthid

matthid Nov 15, 2019

Member

Ah because we use the SDK nuspec output as input for ourself. It seems like they have decided to filter this one. So at least that confirms that we should filter as well if needed?

@forki forki force-pushed the fullsdk branch 7 times, most recently from d5f13fb to ae30a2b Nov 13, 2019
@forki forki force-pushed the fullsdk branch from ae30a2b to 434c567 Nov 21, 2019
enricosada added 4 commits Nov 21, 2019
- install as .NET tool instead of Dotnet CLI tool
- restore the local tools (`dotnet tool restore`) before invoke fake
- install as .NET tool instead of Dotnet CLI tool
- restore the local tools (`dotnet tool restore`) before invoke fake
the generated xmldoc will be in the generate package
install .net core sdk pinned in global.json
@enricosada

This comment has been minimized.

Copy link
Collaborator

enricosada commented Nov 21, 2019

@forki pushed some commit, see log, mostly to use dotnet-mergenupkg v3 as tool

checking the repo, things who can be done:

  • use --no-build after the first dotnet build paket.sln to speedup, because pack/test is rebuilding (i tried to do it, dunno why failed because build was needed, something touched the files)
  • use dotnet.sourcelink, instead of sourcelink
  • use dotnet test for quicktest, instead of nunit-console
  • make paket a normal .net tool (PackAsTool = true), and include the paket.exe as content. This will deprecate dotnet-mergenupkg

i dont know if i can contribute these, but i listed it anyway as tech debt

@forki forki force-pushed the fullsdk branch from 46070a3 to a5dbc6c Nov 22, 2019
@forki forki closed this Nov 22, 2019
@forki forki reopened this Nov 22, 2019
@forki forki changed the title [WIP] Remove FSharp.Compiler.Tools and move to SDK based projects Remove FSharp.Compiler.Tools and move to SDK based projects Nov 22, 2019
@forki forki merged commit f0f0795 into master Nov 22, 2019
4 checks passed
4 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@isaacabraham

This comment has been minimized.

Copy link
Contributor

isaacabraham commented Nov 22, 2019

@enricosada can you create issues for these?

@forki forki deleted the fullsdk branch Nov 22, 2019
@enricosada

This comment has been minimized.

Copy link
Collaborator

enricosada commented Nov 27, 2019

@isaacabraham i fixed these in PR #3734

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.