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

WIP Move to .NET Standard 2.0 #27

Merged
merged 10 commits into from
Sep 17, 2017

Conversation

vasily-kirichenko
Copy link
Collaborator

@vasily-kirichenko vasily-kirichenko commented Sep 9, 2017

Open issues:

  • Seq tests fails because no functions are visible except ofAsyncSeq:

image

image

This is an interesting issue: Seq module is defined twice in ExtCore.Collections namespace, once with ModuleSuffix attribute and once - without it:

  • In Collections.Seq.fs
/// Additional functional operators on sequences.
[<RequireQualifiedAccess; CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
module ExtCore.Collections.Seq
  • In Collections.AsyncSeq.fs
namespace ExtCore.Collections
...
module Seq =
    /// <summary>
    /// Converts asynchronous sequence to a synchronous blocking sequence.
    /// The elements of the asynchronous sequence are consumed lazily.
    /// </summary>
    /// <param name="input"></param>
    /// <returns></returns>
    [<CompiledName("FromAsyncSeq")>]
    let ofAsyncSeq (input : AsyncSeq<'T>) : seq<'T> =
        AsyncSeq.toSeq input

I've commented out ofAsyncSeq and it's fixed the compilation. I suspect it's a regression in F# 4.1

/cc @dsyme

  • All tests pass except single one:
Tests.ExtCore.Control.Collections.Async+Seq+Parallel.batch

  Expected is <System.Int32[10]>, actual is <System.Int32[24]>
  Values differ at index [3]
  Expected: 16
  But was:  4

   at Tests.ExtCore.Control.Collections.Async.Seq.Parallel.batch() in E:\github\ExtCore\ExtCore.Tests\ControlCollections.Async.fs:line 1092
  • I removed the nuspec as it's obsolete now
  • I removed all portable profiles as they are obsolete now. However, I know literally nothing about portable stuff. Have they actually dead now and forever? /cc @enricosada
  • Running dotnet pack in ExtCore folder successfully creates NuGet package:
> dotnet pack
Microsoft (R) Build Engine version 15.3.409.57025 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  ExtCore -> E:\github\ExtCore\ExtCore\bin\Debug\net45\ExtCore.dll
  ExtCore -> E:\github\ExtCore\ExtCore\bin\Debug\netstandard2.0\ExtCore.dll
  Successfully created package 'E:\github\ExtCore\ExtCore\bin\Debug\ExtCore.0.8.47.nupkg'.

I'd like to hear what do you guys thought of all this. Is this the right direction? Have I done everything properly? Thanks!

@vasily-kirichenko vasily-kirichenko changed the title [move to .NET Standard 2.0 / .NET Framework 4.0 WIP Move to .NET Standard 2.0 Sep 9, 2017
@jack-pappas
Copy link
Owner

This is great! Thanks Vasily! Let me take a look over it today and tomorrow.

The one test that's failing is a known bug in the implementation of the Async,Seq.Parallel.batch function -- see #15.

/cc @7sharp9 @jindraivanek

@vasily-kirichenko
Copy link
Collaborator Author

About the issue with duplicated Seq module, see dotnet/fsharp#3565

@vasily-kirichenko
Copy link
Collaborator Author

Apparently NUnit Core test adapter does not support 2.0 nunit/dotnet-test-nunit#122

@vasily-kirichenko
Copy link
Collaborator Author

AppVeyor tries to build it with msbuild 12 and fails.

@vasily-kirichenko
Copy link
Collaborator Author

It installs .net core 2.0, builds the solution and run the tests on Core. All on Windows. I don't know what to do next.

@jack-pappas jack-pappas merged commit f54d036 into jack-pappas:master Sep 17, 2017
@jack-pappas
Copy link
Owner

Thanks Vasily! I've merged your changes. I think I need to make a few more tweaks to the build process, but I'll work on it today. (A few things in ExtCore use F# features restricted to FSharp.Core and require fsc-proto to build. It's possible to build ExtCore with a regular F# compiler but those things are excluded with a preprocessor macro.)

@vasily-kirichenko
Copy link
Collaborator Author

Thanks!

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.

None yet

2 participants