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

2186: Look for Paket executable without ".exe" extension on non-Windows platforms. #2191

Merged
merged 6 commits into from
Nov 14, 2018
Merged

2186: Look for Paket executable without ".exe" extension on non-Windows platforms. #2191

merged 6 commits into from
Nov 14, 2018

Conversation

atlemann
Copy link
Contributor

@atlemann atlemann commented Nov 9, 2018

Looking for a file called paket without .exe extension on non-windows platforms before falling back to paket.exe. This is to support Paket bootstrapped in .NET Core "mode".

Fixes #2186

I could not find any tests for this module.

if Environment.isWindows then
windowsName
else
if !! ("./**/" @@ unixName) |> Seq.isEmpty |> not then
Copy link
Member

Choose a reason for hiding this comment

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

if we already search for it why not use that result but search again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I was just thinking that it needs to fall back to mono if Paket is full framework. Then it needs to search for paket.exe. I can try to avoid double search before the merge.

Copy link
Member

Choose a reason for hiding this comment

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

We might need to introduce a tryFindToolFolderInSubPath and call that first and retry with the .exe

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 thought about that. I can try that out instead, but what should the fallback be if it can't find any of them?

Copy link
Member

Choose a reason for hiding this comment

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

I'd problably use try* for the first and the other overload for the second call (ie. just use the default paket.exe as we do now)

@matthid
Copy link
Member

matthid commented Nov 9, 2018

Thanks for adding support for this scenario!

@atlemann
Copy link
Contributor Author

Not sure if using Path.GetDirectoryName is ok. I guess creating that FileInfo object and calling .Directory could throw and exception, which would use the default folder instead. What do you think?

@matthid
Copy link
Member

matthid commented Nov 11, 2018

Not sure if using Path.GetDirectoryName is ok.

Yes that is ok.

I could not find any tests for this module.

Yes we probably should add some tests to this module or at least for the "new" use-case

@atlemann
Copy link
Contributor Author

I guess the try-with can be removed if we keep Path.GetDirectoryName. But I'm just wondering if there was a reason FileInfo.Directory was used originally.

Where should a test file be created. And should it be called PaketSpecs.fs?

@matthid
Copy link
Member

matthid commented Nov 11, 2018

@atlemann You could for example copy https://github.com/fsharp/FAKE/blob/release/next/src/test/Fake.Core.UnitTests/Fake.DotNet.Cli.fs rename and edit accordingly and add it to either https://github.com/fsharp/FAKE/blob/release/next/src/test/Fake.Core.UnitTests/Fake.Core.UnitTests.fsproj or the integration test project.

You should be able to either run the test via command line (I usually use dotnet run --project src/test/Fake.Core.UnitTest -- or dotnet run --project src/test/Fake.Core.UnitTest -- --run "testgroup/testname" to run a specific test.
I think you can also use visual studio to run them but I'm not sure (we might need to update the adapter, I personally almost never use it).

is that the info you wanted?

I guess the try-with can be removed if we keep Path.GetDirectoryName.

Personally I'm always very nervous when I see with _ -> None so removing is fine by me (we can catch a more specific exception later if needed)

Added tests for Fake.IO.Globbing.Tools
Directory.CreateDirectory folder |> ignore
folder

testSequencedGroup "Find tool paths" <|
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why that needs to be sequenced (not that I think it would matter)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because they would conflict when run in parallel due to this search pattern in the impl. !! ("./**/" @@ toolname). Maybe there's a better way to do the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ops, forgot to remove Focused flag

Copy link
Member

Choose a reason for hiding this comment

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

Because they would conflict when run in parallel due to this search pattern in the impl. !! ("./**/" @@ toolname). Maybe there's a better way to do the tests?

Ah, because you find other results... Yes the "ultimate" solution would be to somehow mock "Environment.CurrentDirectory" but it's fine as it is. Can you add that fact as comment for future reader? Something like // sequenced because otherwise folders conflict with '!! "./**"'

matthid
matthid previously approved these changes Nov 14, 2018
@matthid matthid changed the base branch from master to release/next November 14, 2018 20:58
@matthid
Copy link
Member

matthid commented Nov 14, 2018

Thanks a lot for taking care of this! Will be part of the next release (will merge once green)

@matthid matthid merged commit 6ed49b4 into fsprojects:release/next Nov 14, 2018
@atlemann
Copy link
Contributor Author

No problem! You guys are doing all the hard work.

@matthid matthid mentioned this pull request Dec 3, 2018
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.

2 participants