Fix #1808: Convert DotNet API to "module based" #1812
Conversation
|
I will check and fix the CI tomorrow.. |
|
I don't understand where When running the test on my machine, I don't find the file and so the build process is failing. Could you please help me to fix the CI ? |
|
Testbuild.fsx is a temporary copy of build.fsx to test if the new code bootstraps. In order to do breaking changes you can use #if BOOTSTRAP in build.fsx and update to use the new API... |
|
This is a good way to detect breaking changes and to do the work of updating build.fsx before even releasing ;) |
|
@matthid So I need to send a PR with the |
|
Yes the idea is to send the changes with this pr. and I will remove the #if after review and update. exactly |
|
Ok, I pass the first Now I fail on I think, I could make the CI green by using |
|
Hm yes it could be that this only works if the release notes contains the new version. This currently could be considered as bug in the build system as I'd like it to always use the new packages... |
build.fsx
Outdated
|
|
||
| Target.Create "DotNetCoreUnitTests" (fun _ -> | ||
| // dotnet run -p src/test/Fake.Core.UnitTests/Fake.Core.UnitTests.fsproj | ||
| #if BOOTSTRAP | ||
| let processResult = | ||
| DotNet.Raw (withWorkDir root) "src/test/Fake.Core.UnitTests/bin/Release/netcoreapp2.0/Fake.Core.UnitTests.dll" "--summary" |
There was a problem hiding this comment.
Or DotNet.Exec (but only a suggestion, it is as good as Raw, I just think we used Exec already on some places).
On the other side we also used Run on other places. This is indeed not a very consistent naming at the moment.
There was a problem hiding this comment.
I find Exec a good name.
Indeed using Run seems most natural but their is a run command in dotnet so it's can be confusing.
src/app/Fake.DotNet.Cli/DotNet.fs
Outdated
| @@ -5,905 +5,908 @@ | |||
| /// .NET Core + CLI tools helpers | |||
| module Fake.DotNet.Cli | |||
There was a problem hiding this comment.
Can we remove the Cli here? The idea is that people only need to open Fake.Dotnet and have all the relevant modules in scope (otherwise we have a lot of namespace/module open at each fake script.
But I think this is what doesn't work :/
There was a problem hiding this comment.
If it doesn't work we might consider using [<AutoOpen>] here. Normally we don't use AutoOpen anymore but in this scenario it might be a viable workaround (as long as we make sure only sub-modules are contained and no variables/functions).
There was a problem hiding this comment.
I will try to update the module path and see the result.
I am not sure to understand where you want to place [<AutoOpen>] if needed.
There was a problem hiding this comment.
Basically first try to remove ‘.Cli’ from the commented line. Only if that doesn’t work we can put autoopen above the commented line. The effect should be the same for both solutions...
There was a problem hiding this comment.
But don’t worry, I can do this after merging if needed
|
@matthid Ok, now CI is green and I think I made all the requested change. Thanks for helping me understand how |
|
Thanks a lot! |
Because I added a new indentation level, the diff isn't really helpful sorry 😞 I simply remove the
Dotnetprefix from the command and associated commands.I also rename the
DotNetfunction asRaw(from user point of viewDotNet.Raw).One of the conversion I not sure is for
DotNetOptionstype.Also, I followed the name convention used on most of the FAKE 5 api:
DotNet.Buildbut if we follow the F# convention we should writeDotNet.buildno ? Becausebuildis a function.