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

Fix for #2361 #2364

Merged
merged 1 commit into from
Aug 25, 2019
Merged

Fix for #2361 #2364

merged 1 commit into from
Aug 25, 2019

Conversation

seanamosw
Copy link
Contributor

Description

Fixes #2361 by inverting the original logic.
The code used to first search for paket. On Windows when using the paket dotnet tool installed with -toolpath, this would find a paket subfolder of the dotnet tool.
It would never reach the code to search forpaket.exe.

I've changed this to first search for paket.exe, if not found, then it will search for paket.

@matthid
Copy link
Member

matthid commented Aug 1, 2019

Thanks for taking care of this. I guess this does fix #2242 as well?

@matthid
Copy link
Member

matthid commented Aug 1, 2019

Should we add a test for this?

Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

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

Looks reasonable.

@seanamosw
Copy link
Contributor Author

Thanks for taking care of this. I guess this does fix #2242 as well?

Yes, it looks to be exactly the same issue.

Should we add a test for this?

I could see it being useful to catch potential future regressions here.

@seanamosw
Copy link
Contributor Author

@matthid Is there anything more you need done for this? We have some windows devs/windows builds that are blocked by this.

@matthid
Copy link
Member

matthid commented Aug 7, 2019

I assumed/hoped you would add the test :)

builds that are blocked by this.

Couldn't you just 'dotnet pack' this module and add it to your internal feed to unblock yourself temporarily?

@seanamosw
Copy link
Contributor Author

seanamosw commented Aug 7, 2019

We use paket.template files, obviously don't want to rework that.
Edit: I misunderstood. Yes, we could do that.

I'll see if I can get a test added for this.

@matthid matthid added the needs-tests This PR needs tests in order to be accepted label Aug 17, 2019
@matthid matthid merged commit 2ff27b9 into fsprojects:release/next Aug 25, 2019
@matthid matthid mentioned this pull request Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-tests This PR needs tests in order to be accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

findPaketExecutable finds incorrect path on Windows when using Paket dotnet cli tool
2 participants