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.

Sign up for GitHub

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

DotNet take version into account when determining dotnet cli path #2220

Merged
merged 3 commits into from
Jan 10, 2019
Merged

DotNet take version into account when determining dotnet cli path #2220

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3101b20
Merge pull request #2194 from fsharp/release/next
matthid Dec 3, 2018
47d4972
Take version into account when determining dotnet cli path when globa…
severisv Dec 5, 2018
cf3e157
Introduce tryGetSDKVersionFromGlobalJsonDir
severisv Dec 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/app/Fake.DotNet.Cli/DotNet.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,9 +487,17 @@ module DotNet =
}
static member Create() = {
DotNetCliPath =
let version = try Some <| getSDKVersionFromGlobalJson() with _ -> None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually I don't like with _ -> None if some parsing error occurs I want the exception to be thrown or have a warning

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

findPossibleDotnetCliPaths None
|> Seq.tryHead
// shouldn't hit this one because the previous two probe PATH...
|> Seq.tryFind (fun cliPath ->
match version with
| Some version ->
version
|> Path.combine "sdk"
|> Path.combine (Path.getDirectory cliPath)
|> Directory.Exists
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;) )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the change you suggested (hopefully) :)

You also said to document it well, where and how do I document it exactly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea where this would fit, I guess the release notes are the way to go for now.

| None -> true
)
|> Option.defaultWith (fun () -> if Environment.isUnix then "dotnet" else "dotnet.exe")
WorkingDirectory = Directory.GetCurrentDirectory()
CustomParams = None
Expand Down