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

Please deprecate this whole project #32

Closed
mislav opened this issue Jan 22, 2013 · 15 comments
Closed

Please deprecate this whole project #32

mislav opened this issue Jan 22, 2013 · 15 comments

Comments

@mislav
Copy link

mislav commented Jan 22, 2013

This plugin seems to be popular, presumably because people think they will need it if they use rbenv and Bundler. Yes, it can free users from having to type bundle exec or bin/* to reach the executables for their project, but the tradeoff (never mentioned in the documentation) is that the rehash process is slow, and a detrimental effect to people's whole experience with rbenv in general.

Edit: more issues popping up, documenting here for posterity

I want to be able to simply write rake or rspec in my Ruby project just as much as the next person, without bundle exec or the like, but I firmly believe this plugins approach is flawed which is evident by the number of problems people with it, poor rbenv rehash performance and the sheer amount of code necessary for it to work.

I've updated our wiki to warn against this plugin, and suggested that people simply use bundle install --binstubs and use bin/* for their executables or add ./bin to their PATH. (Yes, this approach has potential security flaws which we're discussing, but at least it works consistently and in a simple manner.)

I would like that this project deprecates itself, by putting up a notice in the documentation about the negative consequences of using the plugin, and by pushing an update that prints out a similar warning to STDERR any time someone does rbenv rehash or invokes a rbenv-bundler-generated binstub. The project's documentation could also suggest alternative solutions for avoiding bundle exec, such as Bundler's --binstubs.

@carsomyr
Copy link
Owner

@mislav Thanks for the feedback, and sorry for the trouble I've caused with the false positive issue reports! I think it's a good idea to warn people about the consequences, if they feel that they need the plugin to work with Bundler. As for the issues users have observed, some of them have been closed outright, and there's still some work to be done with regards to not executing the plugin hook twice (I've noticed that rbenv somehow wants to call it twice, and that sucks if the plugin is going to try to call Ruby).

If you want to see the plugin deprecated, you may want to contact the Homebrew maintainers to remove the formula. I've notice that under Homebrew, rbenv seems to execute plugin hooks even more redundantly, leading to even worse performance. I am CC'ing @MikeMcQuaid on this to get his thoughts. I will meanwhile be updating the README to reflect the best practices espoused by @sstephenson and yourself.

@mislav
Copy link
Author

mislav commented Jan 23, 2013

Wow, thanks for the positive response. I actually expected you to tell me to "go jump in a lake" and close this issue as wontfix :]

Yes removing it from Homebrew might be a good idea, since people might install things from there automatically just because they are returned from brew search rbenv.

@carsomyr
Copy link
Owner

@mislav Nah, I try to balance the will of other devs with the will of the people. The README change is going up soon. As for multiple hook execution, is there any way for rbenv to remember the hooks it has executed? Homebrew uses symlinks, and with each additional symlink, rbenv rehash gets slower, if you know what I mean. I don't believe the other plugins ever run into this issue because they don't have to do crazy amounts of bookkeeping.

@carsomyr
Copy link
Owner

@mislav @MikeMcQuaid Another alternative yet is to print out a large post-install banner warning of the consequences and telling them to not annoy rbenv maintainers. This would encourage people to experiment at their own risk and report issues to me.

@carsomyr
Copy link
Owner

@mislav Ok, I added a warning label to the README.

@carsomyr
Copy link
Owner

@mislav @MikeMcQuaid I'm closing this issue, as there's not much more to do on my end. Let's continue the discussion here, though.

@MikeMcQuaid
Copy link

Passing --binstubs is not a drop-in replacement; it changes the contents of ./bundle/config which can cause problems in large, shared projects (where coworkers don't want that setting in their file). Additionally there's the potential security issue you pointed out and the fact that your approach doesn't work in subdirectories.

That said, I get that it's annoying to have a plugin causing problems with your project and causing false bug reports; we have to deal with similar silliness in Homebrew.

I think the best solution for everyone would be for the two of you to work together to improve rbenv-bundler to try and deal with some of the issues mentioned and I'll add to the caveats for the Homebrew formula to report any rbenv issues to rbenv-bundler instead. I'm not going to remove the formula because I use it, like it and haven't experienced any of the mentioned problems. If @carsomyr completely deletes the project then I'll do so (or perhaps start my own fork).

I'm totally up for further discussion on this though.

@MikeMcQuaid
Copy link

I added the following warning to Homebrew:

rbenv-bundler may cause problems with rbenv including significant slowdown of shell initialisation and rehashing.

Please report any issues with rbenv after installing this plugin here: https://github.com/carsomyr/rbenv-bundler/issues/new

@MikeMcQuaid
Copy link

My mistake; it only changes the contents of ./bundle/config if I haven't edited ~/.bundle/config (I learnt something new today). I'll try playing around with just using binstubs for a day or two.

@carsomyr
Copy link
Owner

@MikeMcQuaid I think that warning message will go a long way. If you like the plugin, would you be so kind so to help me triage suspected redundant hook execution? All you need to do is to insert the following code.

diff --git a/libexec/rbenv-rehash b/libexec/rbenv-rehash
index b05a4c7..26dd207 100755
--- a/libexec/rbenv-rehash
+++ b/libexec/rbenv-rehash
@@ -141,6 +141,7 @@ cd "$OLDPWD"

 # Allow plugins to register shims.
 for script in $(rbenv-hooks rehash); do
+  echo "$script" >> ~/test
   source "$script"
 done

What does the test file show after you rehash? If it's multiple entries, that means rbenv-bundler is being run multiple times, and that it's possible improve rehash times by at least 2x, if you get what I mean.

@MikeMcQuaid
Copy link

I'll get on this at some point, pretty snowed under at the moment. Bookmarked though.

@MikeMcQuaid
Copy link

@carsomyr I'm afraid I'm just using binstubs fairly happily now instead. Sorry!

@carsomyr carsomyr reopened this May 30, 2013
@carsomyr
Copy link
Owner

@mislav @MikeMcQuaid Good news! The plugin no longer slows down shell initialization. Would it be possible to remove the warnings from the rbenv wiki and the Homebrew formula?

@mislav
Copy link
Author

mislav commented May 30, 2013

Most of my gripes weren't about slow shell initialization, but about edge-case bugs and slow rehash in general, and people thinking they are bugs in rbenv. That's why we get so many bug reports on our project (some linked above).

I find binstubs superior since they require significantly less code to solve the same problem, so I won't take down the heads-up on our wiki.

@carsomyr
Copy link
Owner

@mislav Oh, ok. I'll consider this resolved then.

simeonwillbanks pushed a commit to simeonwillbanks/homebrew that referenced this issue Oct 30, 2013
aiwilliams added a commit to aiwilliams/tilde that referenced this issue Sep 30, 2015
david-a-wheeler added a commit to coreinfrastructure/best-practices-badge that referenced this issue Oct 27, 2015
  - Explain why we don't need bin/ or bundle exec more precisely,
    mainly, we are specifically using rbenv-bundler.
  - The rbenv developers want to have rbenv-bundler deprecated, but
    their solution is a hideous mistake-ridden user interface
    (e.g., where the longer correct "bin/rails" is subtly different
    than the incorrect but easier-to-type and widely-documented "rails").
    See carsomyr/rbenv-bundler#32
    for the sorry tale.  In any case, since there are some arguments,
    it's important do document *why* we don't need the prefixes.
    Another solution for us is rvm, but its install approach is to curl in a
    bash script to install random stuff, and it overrides the universe;
    both are significant negatives to its approach.
    It seems better to document a simple single on-ramp for new developers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants