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

Use vswhere to search for vs2017+ install directory to locate msbuild #1752

Closed
wants to merge 2 commits into from
Closed

Use vswhere to search for vs2017+ install directory to locate msbuild #1752

wants to merge 2 commits into from

Conversation

yuyoyuppe
Copy link

FAKE assumes VS is installed in ProgramFilesX86, which wasn't the case for me. We could use vswhere utility with reliable location to determine VS2017+ installation path.

@realvictorprm
Copy link

Good job @yuyoyuppe! 😃

@@ -96,6 +96,15 @@ let msBuildExe =
]
defaultArg (sources |> List.choose id |> List.tryHead) "xbuild"
| false, _ ->
let getVSPathFromVSWhere ver =
ExecProcessAndReturnMessages(fun info ->
Copy link
Member

Choose a reason for hiding this comment

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

I think this throws when the VSWhere file doesn't exist. Is this available on all VS Versions? What if no Visual Studio is installed?

@matthid
Copy link
Member

matthid commented Jan 2, 2018

Good change, only one thing: Can you check what happens when no Visual Studio is installed or VSWhere is missing for some reason? Or investigate if VSWhere is always available.

Thank you.

@matthid matthid added the waiting for author Some information or action was requested and it needs to be addressed. Or a response from author label Jan 27, 2018
@matthid
Copy link
Member

matthid commented Jan 27, 2018

Additionally we need to update the new FAKE 5 modules as well (not only the Obsolete ones). They can be found in Fake.DotNet.MsBuild and Fake.Core.Environment.

@wallymathieu
Copy link
Member

You could also use the nuget package for vswhere so that you know that it exists regardless (but you would then need to take a dependency on that package).

@matthid
Copy link
Member

matthid commented Feb 25, 2018

I'd be fine with that dependency, considering that it will be local to the 'Fake.DotNet.MsBuild' module. we probably would disable that feature in fakelib in that scenario

@matthid
Copy link
Member

matthid commented May 4, 2018

@yuyoyuppe ping?

@yuyoyuppe
Copy link
Author

@matthid ah, sorry, completely forgot! :( I'll try fixing the mentioned issues this weekend.

@matthid
Copy link
Member

matthid commented Aug 4, 2018

Is this ready (it is red)?

@yuyoyuppe
Copy link
Author

@matthid I think there's a cleaner way to achieve the same result: just run fake from VS' native command tool prompt and its location will be detected, so imo this patch is unnecessary.

@wallymathieu
Copy link
Member

wallymathieu commented Aug 6, 2018

VS native command tool prompt is not available on all platforms. It's only available when you have installed VS for Windows (something that you might not want to do on a build server).

@matthid
Copy link
Member

matthid commented Aug 11, 2018

I think this would be viable as long as it still works when vswhere.exe is not found. Because as @wallymathieu said most people don't use the VS command line. Personally, I use the VS-Code bash terminal for example.

But if there is no interest in finishing this. Should we close the PR?

@matthid
Copy link
Member

matthid commented Sep 22, 2018

Looks like it has been superseded by #2077

@matthid matthid closed this Sep 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Some information or action was requested and it needs to be addressed. Or a response from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants