Get-BinRoot defaulted to "C:\" instead of "C:\tools"

[bill-long]

Line 15 set $binRoot to C:\tools, but did not set $env:ChocolateyBinRoot. Thus, the expression on line 23 evaluates $false (because $env:ChocolateyBinRoot is still $null), so line 25 overwrites $binRoot with just "C:".

[Redsandro]

I'm on Linux so I only tested https://github.com/bill-long/chocolatey/pull/1 in my common sense emulator, but this should fix both old and new users. The Get-BinRoot is used in more packages than just OHM so it might be a good idea to implement this.

[bill-long]

Reopening this. I'm a GitHub noob, so I guess merging those commits into my branch closed this pull request?

[bill-long]

Am I supposed to submit a new pull request with the new code?

Edit: I think I figured it out. Those changes were merged into master. After merging master into get-binroot-fix, the changes are reflected in this pull request. Let me know if I screwed something up.

[Redsandro]

No that's not what's supposed to close it, not sure what did.
But anything you merge with your pull-request is automatically added to the pull-request so no need to submit a new one. :)

Edit: Ah I read the notification, not the edit. You figured it out. You can see all the commits to this pull request here: https://github.com/chocolatey/chocolatey/pull/421/commits

Edit: PS - if you haven't already, please see if the regex does what I advertised. I'm pretty sure but don't have a Windows machine to test right now. Ah, you already tested this in https://github.com/bill-long/chocolatey/pull/2#issuecomment-36654950 :)

[gep13]

I suspect that this pull request got closed because you wrote "Fixes #421" in the commit message. Due to the fact that when you fork a repo references to the issues remain pointing at the parent repo, it had the effect of closing the PR (and you were allowed to do this since you opened it in the first place).

[Redsandro]

@gep13 ah that's probable, I didn't know github was so attentive.

[gep13]

@Redsandro it has happened to me on another repo, and I tried to dig up the example, but I think it has gone away, since they "broke" the connection with the upstream fork, thus creating their own set of issues.

[ferventcoder]

I would like for folks to be able to set it to c:\

[Redsandro]

But setting it to c:\ is what causes these errors: #430

[Redsandro]

Either way, please merge @bill-long's initial commit ASAP, otherwise soon everyone will have c:\ as default.

https://github.com/bill-long/chocolatey/commit/d4806255e8d73567a64db0c3a8ad9196a13d226f

[bill-long]

Alright, since we intend to support being able to set it to C:, I am reverting @Redsandro's additions. This means we should probably open a new issue on the fact that if you DO set it to C:, Write-FileUpdateLog.ps1 produces a zillion failures.

[jberezanski]

@ferventcoder currently Chocolatey cannot handle $binRoot set to C:, because the installation tracking feature (Write-FileUpdateLog) relies on being able to scan the entire directory hierarchy under $binRoot, which will almost certainly fail (no access to other users' profile directories, for instance).

So, without major code changes, a Chocolatey installation with $binRoot = C:\ is a broken installation. Since the defective code that causes that is already out there, I would suggest implementing an automatic fix: treat C:\ the same way as an unset $env:ChocolateyBinRoot and reset to the default (C:\tools). That way users affected by this issue could get it fixed simply by upgrading Chocolatey.

[Redsandro]

Agreed. @ferventcoder @bill-long I suggest not reverting my additions. If you want to support c:\, first fix the issues at hand that make this impossible, and then revert my changes that fix current issues.

Don't deny gifted sneakers when at some point in the future you want to put your feet in boots you haven't bought yet. 😉

[bill-long]

Maybe Write-FileUpdateLog can be updated to just ignore directories it can't access?

In any case, I have reverted this to my original, narrow fix, which stops $binRoot from being unintentionally set to C:\ on new installations where the value is not set. As @Redsandro pointed out, we really should merge this narrow fix ASAP to stop future occurrences, as well as get all packages that directly contain Get-BinRoot.ps1 updated.

I created #434 to track the additional work that needs to happen after this narrow fix. Either we need to do something similar to @Redsandro's changes that I reverted, or we need to change Write-FileUpdateLog so that it works when bin root is the systemdrive root.

[bill-long]

Well, I do still have your changes in my master branch, so if @ferventcoder changes his mind, I can just merge them back in. :-)

[Redsandro]

Fixed two packages Redsandro/choco-packages@eb8bbcf

I'm on Linux, won't have access to Windows until sometime next week. Any volunteers I can make co-owner of OpenHardwareMonitor and RenameMaster, willing to cpack and cpush?

• Source:
  • OpenHardwareMonitor
  • https://github.com/Redsandro/chocolatey/tree/master/openhardwaremonitor
  • RenameMaster
  • https://github.com/Redsandro/chocolatey/tree/master/renamemaster

[bill-long]

Sure, add me and I'll update the packages. I'm "bilong" on chocolatey.org.

[Redsandro]

Thanks. Added you.

[Nilzor]

Great, @Redsandro . My concern stemmed from the fact that I see many popular packages do not heed the %chocolatey_bin_root% env variable, which I guess the wiki is partly to blame.

I have myself made a suggestion for update of the Creating Packages wiki page, emphasizing the Get-BinRoot helper instead of the environment variable:

Nilzor/chocolatey-wiki@33b63a8

[Redsandro]

Since when do we have a markdown branch for the wiki? Me (and others) just
use the editor after a bit of discussion.

This change seems 100% objective and accurate so I would vote to just add
it in the wiki which is now outdated indeed.

On Apr 16, 2014 8:40 AM, "Frode Nilsen" notifications@github.com wrote:

[Nilzor]

Yea I just did a clone of the wiki because I wasn't aware I had edit permission :) ...kinda new to this github thingy

[Redsandro]

I have applied your edit, and changed it slightly:

• The Ruby package is a bad example because it might confuse users into embedding the old and buggy BinRoot. Changed it to MinGW.
• The whole point of Get-BinRoot is to not install anything in the root if people forget to set BinRoot, so the C:\Ruby### example is obsolete and will only happen if the package maintainer specifically forces it to install there, or if C:\ is set as the BinRoot, which is not currently possible due to "You cannot call a method on a null-valued expression" introduced somewhere. #430 (And if it was up to me it woudln't ever be possible because that's just stupid (imo)).
• The default path is gonna be either %SystemDrive%:\Tools if https://github.com/bill-long/chocolatey/pull/1 is gonna be merged, or %ChocolateyInstall%\bin to prevent the unpopular thing called clutter, which is under consideration over here Doc generator #458

Feel free to verify and improve the documentation. :)

[ssteward54]

I'm still having the problem with the Chocolatey bin root being set to C:\ and causing all the errors when trying to update the log file. Stepping through the Get-BinRoot script, it looks like $env:ChocolateyBinRoot was never set, and keeps ending up set to C:\ in the code.

[ferventcoder]

@ssteward54 as a workaround, if you close and reopen the shell (and maybe reboot :( ) do you still see this behavior?

[Redsandro]

@ferventcoder ssteward54 is right. Because this issue is starting to bug me, I downloaded an image from Modern.IE and installed Chocolatey. Next, I installed sysinternals. And indeed. ChocolateyBinRoot is set to C:.

[Redsandro]

[Redsandro]

@ssteward54 please specify what package(s) cause this. I think maybe some package(s) might have the buggy binroot hardcoded.

[ssteward54]

It doesn't seem like a package caused the binRoot being set to C:. I just saw the errors at the end of my setup scripts and everything seemed to install ok. I looked through all the packages I used and none of them reference binRoot in the install files. (SVN, TortoiseSvn, custom java.jdk6 adapted from java.jdk, custom eclipse-javaee-kepler adpated from the juno version).

One thing I did realize, is the Windows 7 machine had user account access control turned on, I had turned it off, but didn't reboot. I wonder if this caused a problem setting the Environment variable? I was having some other problems accessing windows services and IIS without running as admin before rebooting. I'll be running the setup process again soon and will see if I can pinpoint the cause.

[ferventcoder]

Sorry, not sure where we are with this. Are you waiting on me to pull it in or is there work to do here?

[bill-long]

Still just waiting for this to be pulled. I created #434 for some additional work that needs to be considered, but this needs to be pulled regardless.

[ferventcoder]

okay, right on.

[ferventcoder]

I'm going to pull this over into stable.

[Redsandro]

Do we also need to address the issue where people now have environment $env:ChocolateyBinRoot set to c:\ because of this bug? It will continue to cause errors.

i.e. pull this one: https://github.com/bill-long/chocolatey/commit/25f7865719e9b101a9860d0a4fa6cad2a0ed3c9d

[ferventcoder]

I think some folks might want something like ruby in the c:\ directory for
Pik (although I think that Pik could be enhanced).

On Wed, May 28, 2014 at 1:55 PM, Sander AKA Redsandro <
notifications@github.com> wrote:

Do we also need to address the issue where people now have environment
$env:ChocolateyBinRoot set to c:?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/421#issuecomment-44449706
.

Rob
"Be passionate in all you do"

http://devlicio.us/blogs/rob_reynolds
http://ferventcoder.com
http://twitter.com/ferventcoder

[bill-long]

As noted in #434, if we're going to support setting bin root to C:, then we need to fix Write-FileUpdateLog. We should probably continue this discussion there.

[Redsandro]

And until then, merge the request I proposed. There's no need to have people with unusable broken installations just because we're contemplating future options when there's been a bugfix ready for 3 months now.

[ferventcoder]

@Redsandro is there a PR for that fix somewhere?

[Redsandro]

It was in this one, but @bill-long reverted that. @bill-long, could you
un-revert that? Can't do much at the moment, I'm on vacation in Berlin with
my tablet. :)
On May 31, 2014 2:50 PM, "Rob Reynolds" notifications@github.com wrote:

@Redsandro https://github.com/Redsandro is there a PR for that fix
somewhere?

—
Reply to this email directly or view it on GitHub
#421 (comment).

[ferventcoder]

No need. Already good -
Cherry picked

• d480625
• cd11749

[Redsandro]

Ah, never underestimate the power of git. :)
On May 31, 2014 3:02 PM, "Rob Reynolds" notifications@github.com wrote:

No need. Already good -
Cherry picked

• d480625
  d480625
• cd11749
  cd11749

—
Reply to this email directly or view it on GitHub
#421 (comment).

[ferventcoder]

Git, super powerful and very precise. 👍

[ferventcoder]

Thanks for the contributions folks! Much appreciated. :)

Sorry I've been (ahem) slow with the PRs lately.