DotNet take version into account when determining dotnet cli path #2220
Conversation
Release 5.11
…l.json is present
src/app/Fake.DotNet.Cli/DotNet.fs
Outdated
| @@ -487,9 +487,17 @@ module DotNet = | |||
| } | |||
| static member Create() = { | |||
| DotNetCliPath = | |||
| let version = try Some <| getSDKVersionFromGlobalJson() with _ -> None | |||
There was a problem hiding this comment.
Usually I don't like with _ -> None if some parsing error occurs I want the exception to be thrown or have a warning
There was a problem hiding this comment.
I completely agree. It's just that getSDKVersionFromGlobalJson fails when global.json is not present, which won't work here.
I didn't want to make a bigger change until the idea was verified, but I suppose having getSDKVersionFromGlobalJson return None when there is no global.json and fail otherwise would make sense?
There was a problem hiding this comment.
Yes or we could introduce a tryGetSDKVersionFromGlobalJson if it doesn't already exist. This would only throw when the file exists and we have some parsing problem and otherwise return Some or None
| version | ||
| |> Path.combine "sdk" | ||
| |> Path.combine (Path.getDirectory cliPath) | ||
| |> Directory.Exists |
There was a problem hiding this comment.
Definitely an interesting approach. Do you think it is "OK" to have different behavior on CLI and fake running the command? Currently I think this should be opt-in but please feel free to tell me otherwise.
But honestly I think this could "just work".
There was a problem hiding this comment.
Having different behavior on CLI and fake in certainly doesn't feel great, but in this case that should only ever occur when there is a global.json containing a version that is not available on CLI, which always fails.
I can't really imagine a case where it would cause trouble, but there are some cases where it would help the user not have to think about every aspect of how everything works, which I think is really one of the big strengths of Fake - some experienced people have made CI stuff much easier for less experienced people.
I do see the point of having "no magic", but sometimes it conflicts with having very simple APIs, and my personal point of view is that there is a middle ground.
I also see when trying to introduce Fake in my dayjob that these kind of details can make a big difference to getting new people excited about Fake.
Opt-in? Could be a good way to do it, any idea how that could look?
There was a problem hiding this comment.
Opt-in? Could be a good way to do it, any idea how that could look?
It would be a simple Options -> Options function including your logic on the DotNetCliPath.
I think in this case we might get away with this "magic" behavior if we properly document it. The only thing is if Microsoft decides to change their folder structure then this logic will obviously break apart. We depend on internals here and if it breaks it will probably be hard to detect and someone needs to understand the new system and implement it (and chances are that's me unfortunately ;) )
There was a problem hiding this comment.
Sorry I've been a little inactive here, but I've had the chance to think a little about this.
I totally understand your point of view, I'm just wondering if this is really adding any value - if the user already has to specify an option it would be just as easy to specify the install location provided by the install step, wouldn't it?
In that case this change is just adding complexity to the module, and in effect giving the users several methods of achieving the same thing (which is not great imo).
My suggestion is we just close this, then :)
There was a problem hiding this comment.
@severisv I think I'll leave that up to you, if you think this is valuable to get in, please fix the first comment and add some documentation around the behavior change and I'll merge this.
If not feel free to close this PR.
If we merge this an people are caught off-guard by this we can still change the behavior back ...
There was a problem hiding this comment.
I made the change you suggested (hopefully) :)
You also said to document it well, where and how do I document it exactly?
There was a problem hiding this comment.
I have no idea where this would fit, I guess the release notes are the way to go for now.
|
Ok lets try this |
|
Thanks! |
What
Make
Fake.DotNettake version into account when looking up location of dotnet.Why
When dotnet is installed using
DotNet.installand the version specified inglobal.jsonit seems unecessary, and perhaps not so beginner friendly to have to specify the location of dotnet in each subsequent call toDotNet.closes #2215