ChefDK should ONLY include git on windows

[chilicheech]

ChefDK is embedding git on the OSX version even though the changelog says: Include git on windows. referring to PR #814 ..

As a user of ChefDK on OSX I run brew install git and make my customizations to it. I have chef shell-init in my .bashrc so that ruby, rake, bundler, foodcritic, and others are available in my path and I don't have to prefix those commands with chef exec.

Please remove the embedded git from the OSX and Linux installers and keep it only on Windows as was the intention of PR #814 ..

[coderanger]

If we want to ship it on mac/linux it should be in the embedded/bin/ only and not linked out to bin/. That will make it available for internal tools without overriding system-level git.

[freimer]

Would it make sense to include a shim that just calls the system git if it is an acceptable version, or the embedded git if the system git is not acceptable or not found? That way if someone doesn't use the ChefDK on a platform it won't mess anyone else up, while still providing an "override" for those that have the embedded bin in their path and don't have git or have such an old one that causes problems. Make sense?

To be clear, the shim would be in the embedded bin directory also, so it would only use the system git if the PATH wasn't set to use the embedded bin directory, and even if it was would prefer the system git if it exists and is "acceptable."

[tyler-ball]

@chilicheech To help me understand your issues, what customizations have you made to git that are not respected by the embedded git we included in the ChefDK? Most settings I know of are controlled by configuration files and should be respected by whatever binary is called.

@coderanger You are correct that embedded/bin is where we put the git binary but chef shell-init adds embedded/bin to your path.

[coderanger]

@tyler-ball It is super unexpected that installing ChefDK would take over what version of git I'm running. On Windows it is an understandable convenience, but for Linux and OS X it is more likely to be a hindrance than a help.

[lamont-granquist]

@tyler-ball we should take embedded/bin out of the PATH -- see #313 and #682

[onlyhavecans]

I would be fine with this is chef shell-init didn't force everything to be higher priority than /usr/local/bin
Maybe the quick(er) fix would be to have chef shell-init add itself to the END of the path instead of the beginning like a good neighbor?

@tyler-ball embedded/bin/git it is compiled without interactive mode on OS X which is workflow breaking for some people (read me).

> git add --patch
fatal: git was built without support for git-add--interactive (NO_PERL=YesPlease).

[lamont-granquist]

we need it first in the path so that we pick up our ruby first. the point of #313 is to move the ruby to /opt/chef/bin and put that first, then eliminate embedded/bin entirely. then if users don't want that, they should not use chef shell-init to setup env vars, and should use a switcher like rvm/rbenv/chruby to manage selecting /opt/chef/bin. any other solution is just going to be rearranging deckchairs.

[johnbellone]

Why not just have the chef shim have a custom shebang similar to /usr/bin/env that sets up the PATH specifically for that?

[TonyLovesDevOps]

I too was surprised by this on Ubuntu Linux when I installed the latest ChefDK and ran into:

$ git add -p
fatal: git was built without support for git-add--interactive (NO_PERL=YesPlease).

I worked around this by adding an alias to my .zshrc by specifying alias git=/usr/bin/git later in the file than where I run eval "$(chef shell-init zsh)".

[lamont-granquist]

#745 and #358 are other examples of how having embedded/bin in the PATH and picking up pkg-config and related stuff and linking against embedded/bin breaks things.

we need to not have embedded/bin in the PATH and need to move ruby and gem installs to /opt/chefdk/bin.

[eedwardsdisco]

Thanks for the workaround, @TonyFlint

I was also very surprised by this behavior. I'd like to see this changed so I have control over my git version.

[charlesjohnson]

A heads-up: We're working on fixing this, but it didn't get done in time to make the June ChefDK 0.15.x release. The fix will be included in the July release.

[tyler-ball]

Scenarios we want to still work if we go with @lamont-granquist's solution:

1. If you use rvm, rbenv, etc. to manage your rubies you still need to be able to add the ChefDK Ruby to their list of rubies
2. If you run gem install foo without chef shell-init should install binstubs to /opt/chefdk/bin instead of /opt/chefdk/embedded/bin (because that won't be on your path)
   • To do this, do we need to make sure Ruby is installed to /opt/chefdk/bin instead of /opt/chefdk/embedded/bin? Or can we symlink the binaries?
3. Appbundling. Right now we appbundle chef-client, so I don't think you can chef exec gem update chef-client and have that work. Do we want to support that? Or do we rely on something like appbundle_updater?
   • Example of my path after running chef shell-init: /opt/chefdk/bin:/Users/tball/.chefdk/gem/ruby/2.1.0/bin:/opt/chefdk/embedded/bin:/Users/tball/.rvm/gems/ruby-2.3.1/bin:/Users/tball/.rvm/gems/ruby-2.3.1@global/bin:/Users/tball/.rvm/rubies/ruby-2.3.1/bin:/Users/tball/google-cloud-sdk/bin:/usr/local/sbin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/tball/.rvm/bin. One solution would be to move ~/.chefdk/gem/ruby/2.1.0/bin to the front of the path.

[charlesjohnson]

1. Can you do that now? I know chruby plays nice, but I thought RVM + ChefDK == deadly poison. Basically I'd say "Preserve existing functionality, don't worry about creating new."
2. I'm okay with binstubs going to /opt/chefdk/bin instead. Adding those binstubs to $PATH is the intended outcome of chef gem install anyway.
3. I'm not particularly interested in supporting this. Thoughts from others?

[lamont-granquist]

@charlesjohnson RVM + ChefDK is not remotely deadly poison any more than rbenv or chruby.

The whole point is to fix the bad design decisions that initially split up the ruby+binstubs which confused rvm/rbenv/chruby needlessly.

And I really think we need to think through the behavior of installing gems over the top of chef-dk and make it behave as close as possible to vanilla ruby, or at least minimize the amount of surprise. The entire root of this problem I think is that we're trying to engineer chef-dk to be 'different' which just leads to frustrated users, and us explaining how people can't do what they're trying to do because $REASONS. That is all incredibly poor UX. If we don't do this now we just kick the can down the road.

And I'm really throwing down that we absolutely need to support the use case of slurping up /opt/chefdk/bin into rvm/rbenv/chruby just like it was a hand-built ruby install. The binary needs to be in there and the binstubs need to be in there so that switching just works. Since those users will not be running chef shell-init they won't be setting up the custom $HOME/.chefdk and we do need to spend a little time thinking through the use case of what happens when they gem install into that ruby.

And we have always stated that using chef-dk as your ruby was supposed to be OPT-IN, it never was supposed to be the ONLY way to use chef-dk, but that alternative has always been broken which is what led to #313. We need to fix that bug, and we need to fix the embedded/bin bug, and the two things are very related and therefore need to be engineered together, otherwise we'll just wind up doing a bad job and having to fix this twice.

[tyler-ball]

Running rvm mount /opt/chefdk/embedded today will exit successfully but it gives you a weird environment. It sets up gemsets in ~/.rvm and does not appear to see the installed gems - it only lists the gems that come pre-installed into the Ruby installation (like json). The chef command still works unless I set APPBUNDLER_ALLOW_RVM = true. If I do gem install chef then it installs it to ~/.chefdk/gems. So I don't think rvm mount works correctly today.

[tyler-ball]

I need to setup a VM to test what gem install foo does. OSX has a system ruby pre-installed so if you don't run chef shell-init then it tries to install gems into the system ruby.

[martinisoft]

I'm an RVM user myself and would prefer to deal with the breaking change before 1.0 of ChefDK with the suggestions from @lamont-granquist to keep ruby environments from clashing with each other. I'd rather the chef universe be separate from my other environments so when installing gems into ChefDK they stay there and don't get mixed in with other environments, creating headaches for my team. Much of my team uses some kind of ruby environment switcher so if it could be compatible that would be much nicer overall.

Do post your results of testing @tyler-ball because it may be helpful. I know another long-standing rubygems bug with Gemfiles is still out there and not resolved yet either. This bug can easily be triggered by ChefDK's current setup too if you happen to mix in a Gemfile with your cookbooks or Chef repo.

[lamont-granquist]

@tyler-ball correct, rvm mount is busted because of the way we split things up.

some other things i thought about late yesterday: really what we're doing here is just engineering ruby-switching to work correctly. and what we're offering is either BYORS (bring-your-own-ruby-switcher: rvm, rbenv, chruby) or else use ours (chef shell-init) and really that is all that chef shell-init IS -- it is just a very slim ruby switcher that has two switches -- off or use-chefdk-for-your-ruby.

the chef shell-init was always supposed to be a beginner-ruby-API for users who only want to use chef and don't already care about using ruby. it was never supposed to be one-size-fits-all that we forced on everyone. using switchers right now is broken. the only halfway pleasant way i've gotten around it is to use chef-dk for my system ruby on mac (because system ruby on mac was kinda lolwut anyway for a long time) and then use 'rvm use system' to make rvm go away to use chef-dk. that's a poor experience, and we should fix this bug.

i'm actually not sure we should worry too much about what happens to appbundled binaries when users do something like:

rvm mount /opt/chefdk/bin/ruby -n chefdk
rvm use chefdk
rvmsudo gem install chef

if /opt/chef/bin/knife gets overwritten with a non-appbundle'd binary and users take a perf hit and we have to point them at 'appbundle-updater' to solve that problem i think that's as far as we need to worry about that -- people can do the trivial thing to update the gems and it 'works' although they may suffer some consequences in perf for stepping outside of the set of gems that we ship -- then there's still the appbundle-updater path to fix that.

so, i think it will all just work without being too surprising for anyone if we go down this road.

[lamont-granquist]

and yeah, lets get this done while we still have the leading zero in the version number to hide behind.

[charlesjohnson]

While I'm convinced the difference between 'embedded/bin' and 'bin' is a valid issue, it's also grown too big for a comment thread on an unrelated issue. We need a dedicated issue to scope this and understand the ramifications.

Meanwhile, I need to re-focus this issue onto "Let's not put git into the path on non-windows hosts when $(which git) returns 0."

[lamont-granquist]

Yes, but you have git in the path because we want to put ruby in the path, so that tightly couples the two issues together. Trying to isolate them is going to lead to bad engineering, or duplicated effort down the road. What we don't need to do is something like move everything other than ruby out of /opt/chefdk/embedded/bin to some other directory, and then later wind up moving ruby to /opt/chefdk/bin and now there's nothing in /opt/chefdk/embedded/bin which will be hugely confusing end state caused by only solving the problem in front of us.

I understand you're trying to decouple tasks and project manage, but I'm convinced that will lead to substantially worse outcomes and much more work and much more confused users than just dealing with the holistic problem up front.

[charlesjohnson]

Related: https://github.com/chef/chef-dk/issues/313

[tyler-ball]

This fix will go out in the 0.16 release of ChefDK