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 1.2.0 #86043

Closed
wants to merge 1 commit into from
Closed

rbenv 1.2.0 #86043

wants to merge 1 commit into from

Conversation

mislav
Copy link
Contributor

@mislav mislav commented Sep 28, 2021

The workaround was meant to hardcode the path to rbenv executable to prevent rbenv itself detecting the path to its executable and finding it under /usr/local/Cellar/rbenv/VERSION/libexec/..., which is a path we cannot rely on in the long term due to Homebrew's automatic cleanup.

However, the workaround is brittle:

  • It's currently failing CI jobs, even those unrelated to rbenv;
  • If the hardcoded path that's the result of opt_bin/"rbenv" makes it into a bottle, it's not guaranteed to match the opt/bin path on someone else's system. Such bottles would not be portable.

Reverts #75996 /cc @steinybot

I might try to come up with a permanent solution for this from within rbenv itself.

@BrewTestBot BrewTestBot added the ruby Ruby use is a significant feature of the PR or issue label Sep 28, 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.

This isn't necessary -- the failure is because the test block assumes that the bottles have been built with the workaround, but they haven't.

Formula/rbenv.rb Outdated
@@ -58,7 +56,6 @@ def install

# Test rehash.
system "rbenv", "rehash"
refute_match "Cellar", (rbenv_root/"shims/foo").read
Copy link
Member

Choose a reason for hiding this comment

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

We can just comment this out for now and CI jobs will pass.

@carlocab carlocab added the CI-no-bottles Merge without publishing bottles label Sep 28, 2021
@mislav
Copy link
Contributor Author

mislav commented Sep 28, 2021

@carlocab See my comment above about bottles. Let's say that we rebuild the rbenv bottle—wouldn't it be dangerous to hardcode the local opt/bin directory in a bottle? What if that bottle gets restored on a system with a different Homebrew prefix?

@mislav mislav mentioned this pull request Sep 28, 2021
@carlocab
Copy link
Member

What if that bottle gets restored on a system with a different Homebrew prefix?

Keg relocation will take care of that. opt_bin isn't actually recorded as /usr/local/opt/rbenv/bin in the bottle. Instead, it is replaced with @HOMEBREW_PREFIX@/opt/rbenv/bin upon bottling, with @HOMEBREW_PREFIX@ substituted for the correct value upon pouring.

See https://github.com/Homebrew/brew/blob/3.2.13/Library/Homebrew/keg_relocate.rb.

@mislav
Copy link
Contributor Author

mislav commented Sep 28, 2021

@carlocab Ah, thanks for explaining!

Then this doesn't have to be reverted. As for the failing test; could we just keep the original test and bump up the revision of the formula to force re-bottling?

@Bo98
Copy link
Member

Bo98 commented Sep 28, 2021

As for the failing test; could we just keep the original test and bump up the revision of the formula to force re-bottling?

Yeah this should have received new bottles IMO.

I get the argument that the issue will only happen on the next version bump, but equally it is affecting people setting up their environments now for when the bump happens in the future so we're just increasing the number of people affected by delaying it.

@mislav mislav changed the title Revert rbenv rehash workaround rbenv 1.2.0 Sep 29, 2021
@mislav
Copy link
Contributor Author

mislav commented Sep 29, 2021

I've modified this PR to upgrade to rbenv 1.2.0 which isn't susceptible to the rehash problem across Homebrew upgrades. rbenv/rbenv#1350

Please re-review!

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.

Thanks @mislav!

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@carlocab
Copy link
Member

Oops. Forgot to remove the CI-no-bottles label. Should be fixed now.

@github-actions github-actions bot added the outdated PR was locked due to age label Oct 31, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-no-bottles Merge without publishing bottles outdated PR was locked due to age 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.

4 participants