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

[Stacked] Schneems/download bundler struct layer #326

Closed
wants to merge 14 commits into from
Closed
240 changes: 114 additions & 126 deletions buildpacks/ruby/src/layers/bundle_download_layer.rs
Original file line number Diff line number Diff line change
@@ -1,149 +1,137 @@
//! # Install the bundler gem
//!
//! ## Layer dir: Install bundler to disk
//!
//! Installs a copy of `bundler` to the `<layer-dir>` with a bundler executable in
//! `<layer-dir>/bin`.
use crate::RubyBuildpack;
use crate::RubyBuildpackError;
use bullet_stream::state::SubBullet;
use bullet_stream::{style, Print};
use commons::gemfile_lock::ResolvedBundlerVersion;
use commons::output::{
fmt,
section_log::{log_step, log_step_timed, SectionLogger},
};
use fun_run::{self, CommandWithName};
use libcnb::build::BuildContext;
use libcnb::data::layer_content_metadata::LayerTypes;
#[allow(deprecated)]
use libcnb::layer::{ExistingLayerStrategy, Layer, LayerData, LayerResult, LayerResultBuilder};
use libcnb::data::layer_name;
use libcnb::layer::{
CachedLayerDefinition, EmptyLayerCause, InvalidMetadataAction, LayerState, RestoredLayerAction,
};
use libcnb::layer_env::{LayerEnv, ModificationBehavior, Scope};
use libcnb::Env;
use serde::{Deserialize, Serialize};
use std::path::Path;
use std::io::Stdout;
use std::process::Command;

#[derive(Deserialize, Serialize, Debug, Clone)]
pub(crate) struct BundleDownloadLayerMetadata {
pub(crate) version: ResolvedBundlerVersion,
}

/// # Install the bundler gem
///
/// ## Layer dir: Install bundler to disk
///
/// Installs a copy of `bundler` to the `<layer-dir>` with a bundler executable in
/// `<layer-dir>/bin`. Must run before [`crate.steps.bundle_install`].
pub(crate) struct BundleDownloadLayer<'a> {
pub(crate) env: Env,
pub(crate) metadata: BundleDownloadLayerMetadata,
pub(crate) _section_logger: &'a dyn SectionLogger,
}

#[allow(deprecated)]
impl<'a> Layer for BundleDownloadLayer<'a> {
type Buildpack = RubyBuildpack;
type Metadata = BundleDownloadLayerMetadata;

fn types(&self) -> LayerTypes {
LayerTypes {
pub(crate) fn handle(
context: &libcnb::build::BuildContext<RubyBuildpack>,
mut bullet: Print<SubBullet<Stdout>>,
env: &Env,
metadata: Metadata,
) -> libcnb::Result<(Print<SubBullet<Stdout>>, LayerEnv), RubyBuildpackError> {
// TODO switch logging to bullet stream
let layer_ref = context.cached_layer(
layer_name!("bundler"),
CachedLayerDefinition {
build: true,
launch: true,
cache: true,
}
}

fn create(
&mut self,
_context: &BuildContext<Self::Buildpack>,
layer_path: &Path,
) -> Result<LayerResult<Self::Metadata>, RubyBuildpackError> {
let bin_dir = layer_path.join("bin");
let gem_path = layer_path;

let mut cmd = Command::new("gem");
cmd.args([
"install",
"bundler",
"--version", // Specify exact version to install
&self.metadata.version.to_string(),
])
.env_clear()
.envs(&self.env);

// Format `gem install --version <version>` without other content for display
let short_name = fun_run::display(&mut cmd);

// Arguments we don't need in the output
cmd.args([
"--install-dir", // Directory where bundler's contents will live
&layer_path.to_string_lossy(),
"--bindir", // Directory where `bundle` executable lives
&bin_dir.to_string_lossy(),
"--force", // Overwrite if it already exists
"--no-document", // Don't install ri or rdoc documentation, which takes extra time
"--env-shebang", // Start the `bundle` executable with `#! /usr/bin/env ruby`
]);

log_step_timed(format!("Running {}", fmt::command(short_name)), || {
cmd.named_output().map_err(|error| {
fun_run::map_which_problem(error, cmd.mut_cmd(), self.env.get("PATH").cloned())
})
})
.map_err(RubyBuildpackError::GemInstallBundlerCommandError)?;

LayerResultBuilder::new(self.metadata.clone())
.env(
LayerEnv::new()
.chainable_insert(Scope::All, ModificationBehavior::Delimiter, "PATH", ":")
.chainable_insert(
Scope::All,
ModificationBehavior::Prepend,
"PATH", // Ensure this path comes before default bundler that ships with ruby, don't rely on the lifecycle
bin_dir,
invalid_metadata_action: &|_| {
(
InvalidMetadataAction::DeleteLayer,
"invalid metadata".to_string(),
)
},
restored_layer_action: &|old: &Metadata, _| {
if let Some(cause) = metadata_diff(old, &metadata) {
(RestoredLayerAction::DeleteLayer, cause)
} else {
(
RestoredLayerAction::KeepLayer,
"using cached version".to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense to update this to specifically mention "Bundler" here. This is not necessary as part of this PR (it could also be a separate PR if you like), but all the other layers specify what's being reused (e.g. metrics agent, Ruby version, gems:

- Metrics agent
  - Using cached metrics agent
- Ruby version `3.1.3` from `default`
  - Using cached Ruby version
- Bundler version `2.4.17` from `Gemfile.lock`
  - Using cached version
- Bundle install
  - Loading cached gems
Suggested change
"using cached version".to_string(),
"using cached Bundler version".to_string(),

)
.chainable_insert(Scope::All, ModificationBehavior::Delimiter, "GEM_PATH", ":")
.chainable_insert(
Scope::All,
ModificationBehavior::Prepend,
"GEM_PATH", // Bundler is a gem too, allow it to be required
gem_path,
),
)
.build()
}

fn existing_layer_strategy(
&mut self,
_context: &BuildContext<Self::Buildpack>,
layer_data: &LayerData<Self::Metadata>,
) -> Result<ExistingLayerStrategy, RubyBuildpackError> {
let old = &layer_data.content_metadata.metadata;
let now = self.metadata.clone();
match cache_state(old.clone(), now) {
State::NothingChanged(_version) => {
log_step("Using cached version");

Ok(ExistingLayerStrategy::Keep)
}
},
},
)?;

let bin_dir = layer_ref.path().join("bin");
let gem_path = layer_ref.path();
match &layer_ref.state {
LayerState::Restored { cause: _ } => {
bullet = bullet.sub_bullet("Using cached version");
}
LayerState::Empty { cause } => {
match cause {
EmptyLayerCause::NewlyCreated => {}
EmptyLayerCause::InvalidMetadataAction { cause }
| EmptyLayerCause::RestoredLayerAction { cause } => {
bullet = bullet.sub_bullet(format!("Clearing cache {cause}"));
Copy link
Contributor

Choose a reason for hiding this comment

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

The log output was changed in this PR, to include more details when the bundler version changed, but that also made the log output a bit awkward:

Clearing cache Bundler version (`2.4.5` to `2.4.17`)

The cause also has differing casing (Bundler version vs invalid metadata).

Suggested change
bullet = bullet.sub_bullet(format!("Clearing cache {cause}"));
bullet = bullet.sub_bullet(format!("Clearing cache {}", style::details(cause)));

This (combined with the edit below, fixing the bundler version change cause) is one way this could be addressed. Might make sense to consider a different phrasing though as these changes result in:

Clearing cache (bundler version changed (`2.4.5` to `2.4.17`))

}
}
State::BundlerVersionChanged(_old, _now) => {
log_step(format!(
"Clearing cache {}",
fmt::details("bundler version changed")
));

Ok(ExistingLayerStrategy::Recreate)
}
let mut cmd = Command::new("gem");
cmd.args(["install", "bundler"]);
cmd.args(["--version", &metadata.version.to_string()])
.env_clear()
.envs(env);

// Format `gem install --version <version>` without other content for display
let short_name = fun_run::display(&mut cmd);

// Directory where bundler's contents will live
cmd.args(["--install-dir", &format!("{}", layer_ref.path().display())]);
// Directory where `bundle` executable lives
cmd.args(["--bindir", &format!("{}", bin_dir.display())]);
cmd.args([
"--force", // Overwrite if it already exists
"--no-document", // Don't install ri or rdoc documentation, which takes extra time
"--env-shebang", // Start the `bundle` executable with `#! /usr/bin/env ruby`
]);

let timer = bullet.start_timer(format!("Running {}", style::command(short_name)));
cmd.named_output()
.map_err(|error| {
fun_run::map_which_problem(error, cmd.mut_cmd(), env.get("PATH").cloned())
})
.map_err(RubyBuildpackError::GemInstallBundlerCommandError)?;
bullet = timer.done();
}
}
layer_ref.write_env(
LayerEnv::new()
.chainable_insert(Scope::All, ModificationBehavior::Delimiter, "PATH", ":")
.chainable_insert(
Scope::All,
ModificationBehavior::Prepend,
"PATH", // Ensure this path comes before default bundler that ships with ruby, don't rely on the lifecycle
bin_dir,
)
.chainable_insert(Scope::All, ModificationBehavior::Delimiter, "GEM_PATH", ":")
.chainable_insert(
Scope::All,
ModificationBehavior::Prepend,
"GEM_PATH", // Bundler is a gem too, allow it to be required
gem_path,
),
)?;
layer_ref.write_metadata(metadata)?;

Ok((bullet, layer_ref.read_env()?))
}

// [derive(Debug)]
enum State {
NothingChanged(ResolvedBundlerVersion),
BundlerVersionChanged(ResolvedBundlerVersion, ResolvedBundlerVersion),
#[derive(Deserialize, Serialize, Debug, Clone)]
pub(crate) struct Metadata {
pub(crate) version: ResolvedBundlerVersion,
}

fn cache_state(old: BundleDownloadLayerMetadata, now: BundleDownloadLayerMetadata) -> State {
let BundleDownloadLayerMetadata { version } = now; // Ensure all properties are checked
fn metadata_diff(old: &Metadata, metadata: &Metadata) -> Option<String> {
let Metadata { version } = old;

if old.version == version {
State::NothingChanged(version)
if version == &metadata.version {
None
} else {
State::BundlerVersionChanged(old.version, version)
Some(format!(
"Bundler version ({old} to {now})",
Copy link
Contributor

Choose a reason for hiding this comment

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

Use lower-case (like the "invalid metadata" cause) https://github.com/heroku/buildpacks-ruby/pull/326/files#diff-cd4ee747be3abfb0ab939cdf3087fa781357327a5eabb3d870f73cc4625b7160R38 and consistent with the output prior to this PR - unless this is intentional/desired ("Bundler" could work with the edit I made above, wrapping the cause in parenthesis). Also added "changed" for context.

Suggested change
"Bundler version ({old} to {now})",
"bundler version changed ({old} to {now})",

old = style::value(version.to_string()),
now = style::value(metadata.version.to_string())
))
}
}

Expand All @@ -152,10 +140,10 @@ mod test {
use super::*;

/// If this test fails due to a change you'll need to implement
/// `migrate_incompatible_metadata` for the Layer trait
/// `invalid_metadata_action`
#[test]
fn metadata_guard() {
let metadata = BundleDownloadLayerMetadata {
let metadata = Metadata {
version: ResolvedBundlerVersion(String::from("2.3.6")),
};

Expand Down
Loading
Loading