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

Add experimental support for skipping Ruby install #130

Closed
wants to merge 2 commits into from

Conversation

schneems
Copy link
Contributor

@schneems schneems commented Mar 9, 2023

Allows user or another buildpack to set HEROKU_USE_SYSTEM_RUBY. This will:

  • Skip our logic for installing ruby
  • Trigger a which ruby command to run, if it returns non-zero exit the build will not proceed
  • Trigger a ruby -v command to run, if it returns non-zero exit the build will not proceed
  • Includes a warning message stating that turning this on effectively disables any guarantees of support

The main driver is allowing for experimentation of replacing a Ruby binary.

Allows user or another buildpack to set `HEROKU_USE_SYSTEM_RUBY`. This will:

- Skip our logic for installing ruby
- Trigger a `which ruby` command to run, if it returns non-zero exit the build will not proceed
- Trigger a `ruby -v` command to run, if it returns non-zero exit the build will not proceed
- Includes a warning message stating that turning this on effectively disables any guarantees of support

The main driver is allowing for experimentation of replacing a Ruby binary.
@jkutner
Copy link

jkutner commented Mar 10, 2023

Can you set:

[[stacks]]
id = "*"

## Allow `*` stacks

As stacks are going away, we can begin to remove the hard requirement that this buildpack only be run with known stacks. To clarify support I added stack detection warning messages.


## Refactor Ruby and Bundler version logic

I've learned a lot since I wrote this original code. I like the way Ed modeled https://github.com/heroku/buildpacks-python/blob/main/src/python_version.rs. 

This refactor allows us to represent the difference between a build where we are responsible for installing Ruby and one where we're not. We need to be able to bust layer caches based on these versions no matter what so I introduced a `RubyCacheKey` which is not guaranteed to also be an installable `RubyVersion`.

The logic in `commons/gemfile_lock.rs` was not great, and it contained structs that seemed like they would better live somewhere else.

Functions in `steps/` for bundler and ruby versions (download and install) were very small logically work better if grouped with their associated version logic.

## Updated section variables in main.rs

Update variables used for logging runtime duration to match the thing they're measuring. This reads better and helps understand what header is being closed by `done()` calls.
user::log_warning(
"Using System Ruby",
formatdoc! {"
Found environment variable {HEROKU_USE_SYSTEM_RUBY}={value:?}. When enabled, you are
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we're brainstorming an interface. One thought/question here is debug-ability.

I'm calling ruby -v and erroring if I didn't find a Ruby version on disk. To help debug if that fails I can use https://github.com/schneems/which_problem (perfect use case for that logic TBH). However there's no visibility into where the user was expecting the Ruby version to come from. I think they would want to know:

  • Did a user set HEROKU_USE_SYSTEM_RUBY or was it a buildpack (or host OS)?
    • We can detect if it came from platform env or in current env
  • If it came from buildpack/host-os how can we forward that location to the end user?
    • One idea would be to have another env var like HEROKU_USE_SYSTEM_RUBY_REASON and make it mandatory when not set via platform/user env. Presumably if it's set via another buildpack it would be something like HEROKU_USE_SYSTEM_RUBY_REASON="core-os-ruby buildpack is installing Ruby"
    • We could just use the value of HEROKU_USE_SYSTEM_RUBY, but I bet most buildpacks that choose to set this would just set it to 1 out of laziness. Though maybe not? We could get wild and enforce that it's semi-structured data like buildpack: <name-of-buildpack> or os: <name-of-os-expected-to-have-ruby>. I'm just trying to think of how we could give users more of a hint of where to look next when things go wrong other then "stuff failed"

It's useful to think about this now, as it would be a breaking change if we mandate something explicit/different in the future.

@schneems
Copy link
Contributor Author

@jkutner done. Also for completeness sake adding this link regarding stacks going away buildpacks/rfcs#219.

I refactored a bunch of logic too. I'm quite happy with the end results. If we end up not keeping this experiment I'll likely want to keep the refactor minus the HEROKU_USE_SYSTEM_RUBY logic.

@schneems schneems closed this Jul 6, 2023
@edmorley edmorley deleted the schneems/use-system-ruby branch August 2, 2023 12:22
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

Successfully merging this pull request may close these issues.

None yet

2 participants