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

Leverage new version-name and version-origin hooks #13

Merged
merged 6 commits into from
Dec 19, 2016

Conversation

jasonkarns
Copy link
Contributor

version-name and version-origin are new hooks in rbenv 1.0 (see New Features under 1.0 release notes: https://github.com/rbenv/rbenv/releases/tag/v1.0.0)

The new version-name hook is invoked after rbenv-version-file lookup, giving plugins a chance to override the selected version.

At the point version-name hooks are run, RBENV_VERSION may already be set. If set by shell (RBENV_VERSION), or local file or global file, then RBENV_VERSION will be non-empty. (The final fallback to 'system' only occurs after version-name hooks have been run.) This PR now hooks into version-name to run rbenv-bundler-ruby-version. (As it currently does, rbenv-bundler-ruby-version must continue to check rbenv-local to determine whether or not to override the ruby version.) The hook then sets RBENV_VERSION if the Gemfile version is to be used. Otherwise, no modication is made and the already-selected version is used (or falls back to 'system').

The old logic in the which hook for verifying the selected ruby is installed is no longer necessary. This existance check is already performed by rbenv-version-name. This is a big improvement, especially because the previous existence check performed by the which hook was inconsistent with rbenv's own check; rbenv-bundler-ruby-version didn't handle the optional 'ruby-' prefix.

The new version-origin hook, in contrast to version-name, is run before the built-in version-origin logic. In order for a plugin to control the version-origin output, the hook needs to set RBENV_VERSION_ORIGIN.

This PR now leverages the version-origin hook. If rbenv-bundler-ruby-version is setting the ruby version from a Gemfile, then the hook sets RBENV_VERSION_ORIGIN to "Gemfile".

Taken together, these two hooks mean that rbenv-version correctly sets and reports the version that will be run.

$ head -2 Gemfile
source 'https://rubygems.org'
ruby '2.1.1'
$ rbenv version
2.1.1 (set by Gemfile)

This means that those using rbenv-version in their PS1 will report the correct ruby version and there is no longer a discrepancy between rbenv-version and rbenv-which.

Hooking into version-name and version-origin is less prone to error.
`which` naturally acquires the correct executable by virtue of
version-name and version-origin already being altered.
@aripollak
Copy link
Owner

Can we still keep backwards compatibility for 0.4.0?

@jasonkarns
Copy link
Contributor Author

Possibly. Though I think it would require version checking within the which hook (so that the lookup logic doesn't occur in the which hook under 1.0)

I'm not sure if the effort would be worthwhile. Those who use this plugin who are keeping it up to date, would likely be using git-update manually or the nodenv-update plugin. In both cases, it is unlikely they haven't already upgraded nodenv to 1.0.

@aripollak
Copy link
Owner

Those who use this plugin who are keeping it up to date, would likely be using git-update manually or the nodenv-update plugin. In both cases, it is unlikely they haven't already upgraded nodenv to 1.0.

I don't think that's a safe assumption, especially since rbenv doesn't change that often (0.4.0 to 1.0 took a few years). I'm using the package from Debian, which hasn't been updated to 1.0. I'm fine with deprecating 0.4.0 support at some point, but I think it's too soon for that.

@jasonkarns
Copy link
Contributor Author

Personally I think it would be sufficient to make it a new major version (breaking change) and note that it's only compatible with rbenv 1.0. I still maintain that the majority of people likely install rbenv and the plugins the same way (whether all through homebrew or all through git, etc). Especially since this is primarily a dev tool plugin, as opposed to something that runs on production boxes, clearly stating on the README the version compatibility would make resolution trivial.

If this were merely a refactor to take advantage of the new hooks, I wouldn't expect the version to be released so fast. But the way the 0.4.0 compatible hook works is fundamentally a hack that doesn't work consistently with the rbenv suite. Fixing that is as nearly as urgent as a bug fix.

@aripollak
Copy link
Owner

I don't consider this an urgent fix since it's working as expected, so I'd like to at least wait to merge this until rbenv 1.0 is in Debian.

@aripollak aripollak merged commit 2827cc7 into aripollak:master Dec 19, 2016
aripollak added a commit that referenced this pull request Dec 19, 2016
@aripollak
Copy link
Owner

Sorry for the long delay on this. Thanks for the PR!

@jasonkarns jasonkarns deleted the version-hooks branch December 22, 2016 13:32
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.

2 participants