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

Retrieve the ProgramFilesX86 value by exclusively relying on System.Environment #705

Closed
wants to merge 1 commit into from

Conversation

bbenoist
Copy link
Contributor

Related documentation

⚠️ Warning

As I am a newcomer on FAKE, please note that I did not tested the code more than that:

  • I build it only on a 64-bit Windows using both build.cmd and FAKE.sln
  • I only ran it to build my own (and private) source code.

IMHO, it might be careful to test this PR on every platform targeted by FAKE.

@forki
Copy link
Member

forki commented Mar 24, 2015

did you encounter a bug or why do we need to change that?

@bbenoist
Copy link
Contributor Author

Nope. In fact, I had recently encountered the same problematic and resolved it by using those 4 lines based only on .Net 😅

As I find the solution to be more maintainable (moves the detection to the .Net framework) and concise than making guesses on environment variables, I just wanted to inform you about such an alternative...

@forki
Copy link
Member

forki commented Mar 24, 2015

I'm in favor of using the .NET stuff, but I don't remember why we have that hack. Maybe it's breaking...

@bbenoist
Copy link
Contributor Author

Here are some commits affecting this function (from oldest to latest):

  1. caa4733 Try to replace [ProgramFiles] and [ProgramFilesX86] in config file.
  2. fea8435 Move ProgramFilesX86 into EnvironmentHelper.fs
  3. b4e468e Using Path.Combine if possible.
  4. 0ef901d Fixed issue Fake.Deploy fail investigation #49, Fake.Deploy failures.
  5. 146945d Handle problems with ProgramFilesX86 on mono - Fix build of VSIX template fslaborg/zzarchive-FsLab#32

IMO, modifications of ProgramFilesX86 in 3 and 4 would have been unnecessary with an implementation relying on .Net. Only 5 seems to have interesting arguments against using the .Net way because it might throws exception on non-Windows platforms. However this can be managed with a proper try catch statement.

As I am not yet accustomed with Mono, there is something that I do not understand. What's the point of using ProgramFilesX86 outside of a Windows environment? Should'n it be avoided by throwing errors in such contexts?

@bbenoist
Copy link
Contributor Author

Environment.cs - .NET Reference Source and Environment.cs - Mono answered my questions but have different behaviors when the information is unavailable.
Microsoft's implementation throws an exception whereas Mono's returns an empty string.

😢

@bbenoist
Copy link
Contributor Author

cc @spouliot (79312b5)

@matthid
Copy link
Member

matthid commented May 28, 2017

All this stuff has changed anyway in the netcore version. I'll close this fow now. Feel free to send again against the new code.

@matthid matthid closed this May 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants