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

rbenv: use opt bin symlink in rehash script #75996

Closed
wants to merge 1 commit into from

Conversation

steinybot
Copy link
Contributor

@steinybot steinybot commented Apr 27, 2021

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

rbenv equivalent to #75715

@BrewTestBot BrewTestBot added the ruby Ruby use is a significant feature of the PR or issue label Apr 27, 2021
@carlocab
Copy link
Member

Thanks, @steinybot.

Am I understanding this correctly? This change is meant to fix rbenv's hardcoded paths getting invalidated whenever rbenv's version is bumped here in homebrew/core. In particular, it means that it works out the same for a user if they get this update now, or get it at the next version bump. Or am I misunderstanding something here?

@steinybot
Copy link
Contributor Author

This change is meant to fix rbenv's hardcoded paths getting invalidated whenever rbenv's version is bumped here in homebrew/core.

Yes this is correct.

In particular, it means that it works out the same for a user if they get this update now, or get it at the next version bump.

Yes in practice I think that is correct. The install will not fixing the paths directly, it fixes the rehash script which is what writes the paths. The user still has to run rehash to fix existing ruby installs (or if they install a new ruby version then those will have the correct paths). If this goes as a new revision then that is going to change the path from 1.1.2 to 1.1.2_1 which will break the path so they have to run rehash. If it goes in 1.1.3 then also have to run rehash.

@@ -25,6 +26,8 @@ def install
s.gsub! ":/usr/local/etc/rbenv.d", ":#{HOMEBREW_PREFIX}/etc/rbenv.d\\0" if HOMEBREW_PREFIX.to_s != "/usr/local"
end

inreplace "libexec/rbenv-rehash", "$(command -v rbenv)", opt_bin/"rbenv"
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but it feels brittle. If the expression command -v rbenv changes even slightly within rbenv source, this workaround silently breaks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two potential sources of breakage:

  1. The line we want to change stops using $(command -v rbenv).
  2. libexec/rbenv-rehash starts using $(command -v rbenv) somewhere else we may not want to change.

Both seem sufficiently unlikely to me, but you'd know more about this than I do, of course. libexec/rbenv-rehash hasn't changed in years, and the relevant line we want to change has been there for even longer.

I'm not sure 2 is such a problem either, as I think I'm comfortable with using opt_bin/"rbenv" everywhere rbenv wants to do command -v rbenv.

1 should not be a problem as long as it doesn't happen simultaneously with 2, as the inreplace will error out when it is unable to do a replacement. We can avoid that by tightening the inreplace with more context from the line we actually want to replace.

Copy link
Sponsor Contributor

@mislav mislav May 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for elaborating. I'm not opposed to this being merged. But I do not feel like the fix is ideal in the long term. The main problem is that Homebrew aggressively prunes old versions of software after upgrades. This breaks not just rbenv, but also ruby gems linked to removed versions of Homebrew software that got updated in the meantime. I have fixed my own Ruby development environment with export HOMEBREW_NO_INSTALL_CLEANUP=true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully such a change wouldn't break silently and would be picked up by the test.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Jun 7, 2021
@carlocab
Copy link
Member

Sorry for the delayed review on this -- I'm thinking we can drop the revision and merge this without bottles so we can apply this change to the next version bump.

Making this change with the revision now means discarding the high_sierra bottle, which seems a bit wasteful to me, since this isn't really meant to do anything until rbenv gets a version bump.

@carlocab carlocab added in progress Stale bot should stay away and removed stale No recent activity labels Jun 10, 2021
@jonchang
Copy link
Contributor

jonchang commented Sep 1, 2021

ping @carlocab, I've resolved the merge conflicts here and dropped the revision line.

@carlocab carlocab added CI-no-bottles Merge without publishing bottles and removed in progress Stale bot should stay away labels Sep 1, 2021
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@mislav
Copy link
Sponsor Contributor

mislav commented Sep 2, 2021

Thank you @steinybot @jonchang for fixing this and @carlocab for reviewing! ❤️

@mislav mislav mentioned this pull request Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-no-bottles Merge without publishing bottles ruby Ruby use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants