-
Notifications
You must be signed in to change notification settings - Fork 75
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
Core clr support. #57
Changes from 41 commits
db5dfee
70dfbb9
aa1d1e8
355d853
28f94cd
a650509
240f9b8
3474d62
ea78150
db5fa67
27cf645
3a05b52
6e5cc65
42f01a7
c80f99b
a141135
50e4167
c757405
6e7d717
23a8560
46b3046
18a9373
e5759cc
4a6e356
9abb5ab
1d7d009
60f5bf4
66c43a1
df0c1fc
a7959dd
1dc959a
9b8ebc1
64fb3a3
28036b1
065b67c
ed08e6b
858e8cd
ce21901
77495a3
8474c23
019ea75
4ff2ab8
19a4b8e
aa1b190
0c0c8c1
a1a7dc1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
os: Visual Studio 2015 | ||
|
||
configuration: Release | ||
test: | ||
assemblies: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -171,6 +171,28 @@ Target "ReleaseGitHub" (fun _ -> | |
|> Async.RunSynchronously | ||
) | ||
|
||
// .NET Core SDK and .NET Core | ||
|
||
let assertExitCodeZero x = if x = 0 then () else failwithf "Command failed with exit code %i" x | ||
|
||
Target "Build.NetCore" (fun _ -> | ||
Shell.Exec("dotnet", "restore") |> assertExitCodeZero | ||
Shell.Exec("dotnet", "--verbose pack --configuration Release", "src/Argu") |> assertExitCodeZero | ||
) | ||
|
||
Target "RunTests.NetCore" (fun _ -> | ||
Shell.Exec("dotnet", "--verbose run --configuration Release", "tests/Argu.NetCore.Tests") |> assertExitCodeZero | ||
) | ||
|
||
let isDotnetSDKInstalled = try Shell.Exec("dotnet", "--version") = 0 with _ -> false | ||
|
||
Target "Nuget.AddNetCore" (fun _ -> | ||
let nupkg = sprintf "../../bin/Argu.%s.nupkg" (release.NugetVersion) | ||
let netcoreNupkg = sprintf "bin/Release/Argu.%s.nupkg" (release.NugetVersion) | ||
|
||
Shell.Exec("dotnet", sprintf """mergenupkg --source "%s" --other "%s" --framework netstandard1.6 """ nupkg netcoreNupkg, "src/Argu/") |> assertExitCodeZero | ||
) | ||
|
||
|
||
Target "Release" DoNothing | ||
|
||
|
@@ -191,7 +213,10 @@ Target "Default" DoNothing | |
"Default" | ||
==> "PrepareRelease" | ||
==> "Build.Net35" | ||
=?> ("Build.NetCore", isDotnetSDKInstalled) | ||
=?> ("RunTests.NetCore", isDotnetSDKInstalled) | ||
==> "NuGet" | ||
=?> ("Nuget.AddNetCore", isDotnetSDKInstalled) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This target should probably fail if CoreCLR is not installed, otherwise we risk pushing bad packages from machines that may lack it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I'd be in favor of renaming the "NuGet" target to "NuGet.Paket" so that "NuGet" builds a complete package. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eiriktsarpalis it's easy with fake to make the conditional as you wish or change it based on env var, etc. it's written like that so doesnt break the build when someones doesnt have that installed, that's my main fear. Np about renaming |
||
==> "GenerateDocs" | ||
==> "ReleaseDocs" | ||
==> "NuGetPush" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"projects":[ "src", "tests" ] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ | |
open System | ||
open System.IO | ||
open System.Collections.Generic | ||
|
||
#if !CORE_CLR | ||
open System.Configuration | ||
open System.Reflection | ||
|
||
|
@@ -68,4 +70,14 @@ type ConfigurationReader = | |
|
||
/// Create a configuration reader instance using an F# function | ||
static member FromFunction(reader : string -> string option, ?name : string) = | ||
new FunctionConfigurationReader(reader, ?name = name) :> IConfigurationReader | ||
new FunctionConfigurationReader(reader, ?name = name) :> IConfigurationReader | ||
|
||
static member DefaultReader () = ConfigurationReader.FromAppSettings() | ||
#else | ||
/// Configuration reader implementations | ||
type ConfigurationReader = | ||
static member DefaultReader () = | ||
{ new IConfigurationReader with | ||
member x.Name = "Default - Empty Configuration Reader" | ||
member x.GetValue k = null } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather this failed with a NotSupportedException |
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,11 @@ open FSharp.Reflection | |
open FSharp.Quotations | ||
open FSharp.Quotations.Patterns | ||
|
||
#if NETSTANDARD1_6 | ||
let allBindings = true | ||
#else | ||
let allBindings = BindingFlags.NonPublic ||| BindingFlags.Public ||| BindingFlags.Static ||| BindingFlags.Instance | ||
#endif | ||
|
||
let inline arguExn fmt = Printf.ksprintf(fun msg -> raise <| new ArguException(msg)) fmt | ||
|
||
|
@@ -115,14 +119,24 @@ type Unchecked = | |
} | ||
|
||
type MemberInfo with | ||
member m.ContainsAttribute<'T when 'T :> Attribute> () = | ||
m.GetCustomAttributes(typeof<'T>, true) |> Array.isEmpty |> not | ||
member m.ContainsAttribute<'T when 'T :> Attribute> () : bool= | ||
m.GetCustomAttributes(typeof<'T>, true) |> Seq.isEmpty |> not | ||
|
||
member m.TryGetAttribute<'T when 'T :> Attribute> () = | ||
match m.GetCustomAttributes(typeof<'T>, true) with | ||
member m.TryGetAttribute<'T when 'T :> Attribute> () : 'T option = | ||
match m.GetCustomAttributes(typeof<'T>, true) |> Seq.toArray with | ||
| [||] -> None | ||
| attrs -> attrs |> Array.last |> unbox<'T> |> Some | ||
| attrs -> attrs |> Array.last :?> 'T |> Some | ||
|
||
#if CORE_CLR | ||
type Type with | ||
member x.GetCustomAttributes(t, b : bool) = x.GetTypeInfo().GetCustomAttributes(t, b) | ||
|
||
member m.ContainsAttribute<'T when 'T :> Attribute> () : bool= | ||
m.GetTypeInfo().ContainsAttribute<'T>() | ||
|
||
member m.TryGetAttribute<'T when 'T :> Attribute> () : 'T option = | ||
m.GetTypeInfo().TryGetAttribute<'T>() | ||
#endif | ||
|
||
type IDictionary<'K,'V> with | ||
member d.TryFind k = | ||
|
@@ -131,19 +145,25 @@ type IDictionary<'K,'V> with | |
else None | ||
|
||
|
||
#if !CORE_CLR | ||
let currentProgramName = lazy(System.Diagnostics.Process.GetCurrentProcess().MainModule.ModuleName) | ||
#else | ||
// error FS0039: The value, constructor, namespace or type 'Process' is not defined | ||
let currentProgramName = lazy("wtf") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Surely, there must be a way to get the process name here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can look again, but I couldn't make if work at the time. But you are right it shouldn't stay this way :) |
||
#endif | ||
|
||
type UnionCaseInfo with | ||
member uci.GetAttributes<'T when 'T :> Attribute> (?includeDeclaringTypeAttrs : bool) = | ||
let includeDeclaringTypeAttrs = defaultArg includeDeclaringTypeAttrs false | ||
|
||
let caseAttrs = uci.GetCustomAttributes typeof<'T> | ||
let caseAttrs = uci.GetCustomAttributes typeof<'T> |> Seq.cast<Attribute> | ||
let attrs = | ||
if includeDeclaringTypeAttrs then | ||
uci.DeclaringType.GetCustomAttributes(typeof<'T>, false) | ||
|> Seq.cast<Attribute> | ||
|> Seq.append caseAttrs | ||
else | ||
caseAttrs :> _ | ||
caseAttrs | ||
|
||
attrs |> Seq.map (fun o -> o :?> 'T) | ||
|
||
|
@@ -152,7 +172,7 @@ type UnionCaseInfo with | |
|
||
match uci.GetCustomAttributes typeof<'T> with | ||
| [||] when includeDeclaringTypeAttrs -> | ||
match uci.DeclaringType.GetCustomAttributes(typeof<'T>, false) with | ||
match uci.DeclaringType.GetCustomAttributes(typeof<'T>, false) |> Seq.toArray with | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the change here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On dotnetcore the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wow, the coreclr team has taken breaking compatibility really seriously 👎 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's good to see the CoreCLR/CoreFx teams taking this opportunity to fix up some poor APIs in the original BCL. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eiriktsarpalis that's because .net core started as minimal with a cleanup of api. Instead of do like i think, ihmo, is better to evolve the api, if updating the code is easy, instead of adding duplicated functions. But they got the issue about compatibility (drop in replacement), so each version of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @enricosada I see your point, my feeling though is that such superficial changes cause more problems than what they allegedly solve. It's good that the coreclr team has come to terms with this. |
||
| [||] -> None | ||
| attrs -> Some (attrs.[0] :?> 'T) | ||
| [||] -> None | ||
|
@@ -162,7 +182,7 @@ type UnionCaseInfo with | |
let includeDeclaringTypeAttrs = defaultArg includeDeclaringTypeAttrs false | ||
if uci.GetCustomAttributes typeof<'T> |> Array.isEmpty |> not then true | ||
elif includeDeclaringTypeAttrs then | ||
uci.DeclaringType.GetCustomAttributes(typeof<'T>, false) |> Array.isEmpty |> not | ||
uci.DeclaringType.GetCustomAttributes(typeof<'T>, false) |> Seq.isEmpty |> not | ||
else | ||
false | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
{ | ||
"version": "3.1.0", | ||
"buildOptions": { | ||
"debugType": "portable", | ||
"compilerName": "fsc", | ||
"define": [ | ||
"CORE_CLR" | ||
], | ||
"compile": { | ||
"includeFiles": [ | ||
"AssemblyInfo.fs", | ||
"Types.fs", | ||
"Attributes.fs", | ||
"Utils.fs", | ||
"ConfigReaders.fs", | ||
"UnionArgInfo.fs", | ||
"PreCompute.fs", | ||
"UnParsers.fs", | ||
"ParseResults.fs", | ||
"Parsers/Common.fs", | ||
"Parsers/Cli.fs", | ||
"Parsers/KeyValue.fs", | ||
"ArgumentParser.fs" | ||
] | ||
} | ||
}, | ||
"tools": { | ||
"dotnet-mergenupkg": "1.0.*", | ||
"dotnet-compile-fsc": { | ||
"version": "1.0.0-preview2-*", | ||
"imports": "dnxcore50" | ||
} | ||
}, | ||
"frameworks": { | ||
"netstandard1.6": { | ||
"buildOptions": { | ||
"define": [ | ||
"NETSTANDARD1_5", | ||
"NETSTANDARD1_6" | ||
] | ||
}, | ||
"dependencies": { | ||
"System.Xml.XDocument": "4.0.11", | ||
"Microsoft.FSharp.Core.netcore": "1.0.0-alpha-160629", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be removed and https://www.nuget.org/packages/FSharp.Core/4.0.1.7-alpha used instead below instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the @ctaggart my pr i remove the .net 40 version from project.json, so no problem about that. |
||
"NETStandard.Library": "1.6.0" | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do the packages need to be merged after creation? Can this not be handled by paket.template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventually it will
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@forki can't we just manually specify the CoreCLR bits in the
files
section? Or is there more to it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can, but lets use merge for now and do that when paket is ready.
it's all a bit of bootstrapping problem. Argu needs FAKE/Paket featrues and paket needs argu...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not atm, afaik, it's up to paket team, i'd like an helper cmd too.
That's the minimal flow (maintenance, easy to test) afaik to add support for .net core.
Also like that you dont need to redefine the dependencies inside paket.template (paket doesnt manage project.json), and are always aligned with
project.json
(used for build)Are merged only:
lib/netstandard1.6
(the .net core assemblies).netstandard1.6
, used only for that profile (no issue with others)not the metadata like authors, etc, that's defined in paket.template already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eiriktsarpalis @forki files is ok, problem are dependencies becasue doesnt support deps by conditional fw ( ref fsprojects/Paket#913 ).
propose by me workaround fsprojects/Paket#1539
and .net core discussion (the one we are having there) fsprojects/Paket#1793
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as note: i have written the
dotnet mergenupkg
tool for project without paket, just to make it easier the integration, but works where paket is used too. i see that as a temporary solution to an issue (until project.json => msbuild )There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm already creating a dnc helper in fake. Will send pull request to Enricos fork of argu soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@forki can we use argu or better chessie as the example for fake new dnc helpers?
In another pr if possibile, so we can iterate the design.
Atm this workflow work really well, checklist of steps, i'd like to make people use that atm, and update to fake helpers later.
i think that can speedup a little .net core adoption.
Also i think current one is not too bad, because make easy to see dotnet command, so can be run locally, no magic (atm people see dnc sdk as magic)