Code review changes

Cargo.toml

@@ -11,6 +11,7 @@ serde = "1"
 [dev-dependencies]
 libcnb-test = "=0.19.0"
 indoc = "2"
+toml = "0.8"
 
 [lints.rust]
 unreachable_pub = "warn"

src/debian.rs

@@ -3,7 +3,6 @@ use std::fmt::{Display, Formatter};
 use std::str::FromStr;
 
 #[derive(Debug, Eq, PartialEq, Hash, Clone, Serialize, Deserialize)]
-#[serde(transparent)]
 // https://www.debian.org/doc/debian-policy/ch-controlfields.html#source
 pub(crate) struct DebianPackageName(pub(crate) String);
 
@@ -60,8 +59,8 @@ pub(crate) enum DebianMultiarchName {
     X86_64_LINUX_GNU,
 }
 
-impl From<DebianArchitectureName> for DebianMultiarchName {
-    fn from(value: DebianArchitectureName) -> Self {
+impl From<&DebianArchitectureName> for DebianMultiarchName {
+    fn from(value: &DebianArchitectureName) -> Self {
         match value {
             DebianArchitectureName::AMD_64 => DebianMultiarchName::X86_64_LINUX_GNU,
         }
@@ -132,7 +131,7 @@ mod tests {
     #[test]
     fn converting_debian_architecture_name_to_multiarch_name() {
         assert_eq!(
-            DebianMultiarchName::from(DebianArchitectureName::AMD_64),
+            DebianMultiarchName::from(&DebianArchitectureName::AMD_64),
             DebianMultiarchName::X86_64_LINUX_GNU
         );
     }

src/layers/environment.rs

@@ -0,0 +1,141 @@
+use crate::debian::{DebianArchitectureName, DebianMultiarchName};
+use crate::AptBuildpack;
+use commons::output::build_log::SectionLogger;
+use libcnb::build::BuildContext;
+use libcnb::data::layer_content_metadata::LayerTypes;
+use libcnb::generic::GenericMetadata;
+use libcnb::layer::{Layer, LayerResult, LayerResultBuilder};
+use libcnb::layer_env::{LayerEnv, ModificationBehavior, Scope};
+use libcnb::Buildpack;
+use std::ffi::OsString;
+use std::path::Path;
+
+pub(crate) struct EnvironmentLayer<'a> {
+    pub(crate) debian_architecture_name: &'a DebianArchitectureName,
+    pub(crate) installed_packages_dir: &'a Path,
+    pub(crate) _section_logger: &'a dyn SectionLogger,
+}
+
+impl<'a> Layer for EnvironmentLayer<'a> {
+    type Buildpack = AptBuildpack;
+    type Metadata = GenericMetadata;
+
+    fn types(&self) -> LayerTypes {
+        LayerTypes {
+            build: true,
+            launch: true,
+            cache: false,
+        }
+    }
+
+    fn create(
+        &mut self,
+        _context: &BuildContext<Self::Buildpack>,
+        _layer_path: &Path,
+    ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {
+        LayerResultBuilder::new(GenericMetadata::default())
+            .env(configure_environment(
+                self.installed_packages_dir,
+                &DebianMultiarchName::from(self.debian_architecture_name),
+            ))
+            .build()
+    }
+}
+
+fn configure_environment(
+    packages_dir: &Path,
+    debian_multiarch_name: &DebianMultiarchName,
+) -> LayerEnv {
+    let mut env = LayerEnv::new();
+
+    let bin_paths = [
+        packages_dir.join("bin"),
+        packages_dir.join("usr/bin"),
+        packages_dir.join("usr/sbin"),
+    ];
+    prepend_to_env_var(&mut env, "PATH", &bin_paths);
+
+    // support multi-arch and legacy filesystem layouts for debian packages
+    // https://wiki.ubuntu.com/MultiarchSpec
+    let library_paths = [
+        packages_dir.join(format!("usr/lib/{debian_multiarch_name}")),
+        packages_dir.join("usr/lib"),
+        packages_dir.join(format!("lib/{debian_multiarch_name}")),
+        packages_dir.join("lib"),
+    ];
+    prepend_to_env_var(&mut env, "LD_LIBRARY_PATH", &library_paths);
+    prepend_to_env_var(&mut env, "LIBRARY_PATH", &library_paths);
+
+    let include_paths = [
+        packages_dir.join(format!("usr/include/{debian_multiarch_name}")),
+        packages_dir.join("usr/include"),
+    ];
+    prepend_to_env_var(&mut env, "INCLUDE_PATH", &include_paths);
+    prepend_to_env_var(&mut env, "CPATH", &include_paths);
+    prepend_to_env_var(&mut env, "CPPPATH", &include_paths);
+
+    let pkg_config_paths = [
+        packages_dir.join(format!("usr/lib/{debian_multiarch_name}/pkgconfig")),
+        packages_dir.join("usr/lib/pkgconfig"),
+    ];
+    prepend_to_env_var(&mut env, "PKG_CONFIG_PATH", &pkg_config_paths);
+
+    env
+}
+
+fn prepend_to_env_var<I, T>(env: &mut LayerEnv, name: &str, paths: I)
+where
+    I: IntoIterator<Item = T>,
+    T: Into<OsString>,
+{
+    let separator = ":";
+    env.insert(Scope::All, ModificationBehavior::Delimiter, name, separator);
+    env.insert(
+        Scope::All,
+        ModificationBehavior::Prepend,
+        name,
+        paths
+            .into_iter()
+            .map(Into::into)
+            .collect::<Vec<_>>()
+            .join(separator.as_ref()),
+    );
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::path::PathBuf;
+
+    #[test]
+    fn test_configure_environment() {
+        let debian_multiarch_name = DebianMultiarchName::X86_64_LINUX_GNU;
+        let layer_env = configure_environment(&PathBuf::from("/"), &debian_multiarch_name);
+        let env = layer_env.apply_to_empty(Scope::All);
+        assert_eq!(env.get("PATH").unwrap(), "/bin:/usr/bin:/usr/sbin");
+        assert_eq!(
+            env.get("LD_LIBRARY_PATH").unwrap(),
+            "/usr/lib/x86_64-linux-gnu:/usr/lib:/lib/x86_64-linux-gnu:/lib"
+        );
+        assert_eq!(
+            env.get("LIBRARY_PATH").unwrap(),
+            "/usr/lib/x86_64-linux-gnu:/usr/lib:/lib/x86_64-linux-gnu:/lib"
+        );
+        assert_eq!(
+            env.get("INCLUDE_PATH").unwrap(),
+            "/usr/include/x86_64-linux-gnu:/usr/include"
+        );
+        assert_eq!(
+            env.get("CPATH").unwrap(),
+            "/usr/include/x86_64-linux-gnu:/usr/include"
+        );
+        assert_eq!(
+            env.get("CPPPATH").unwrap(),
+            "/usr/include/x86_64-linux-gnu:/usr/include"
+        );
+        assert_eq!(
+            env.get("PKG_CONFIG_PATH").unwrap(),
+            "/usr/lib/x86_64-linux-gnu/pkgconfig:/usr/lib/pkgconfig"
+        );
+    }
+}

src/layers/installed_packages.rs

@@ -1,23 +1,16 @@
 use crate::aptfile::Aptfile;
-use crate::debian::{DebianArchitectureName, DebianMultiarchName};
-use crate::errors::AptBuildpackError;
 use crate::AptBuildpack;
 use commons::output::interface::SectionLogger;
 use commons::output::section_log::log_step;
 use libcnb::build::BuildContext;
 use libcnb::data::layer_content_metadata::LayerTypes;
 use libcnb::layer::{ExistingLayerStrategy, Layer, LayerData, LayerResult, LayerResultBuilder};
-use libcnb::layer_env::{LayerEnv, ModificationBehavior, Scope};
 use libcnb::Buildpack;
 use serde::{Deserialize, Serialize};
-use std::ffi::OsString;
-use std::path::{Path, PathBuf};
-use std::str::FromStr;
-use std::sync::atomic::{AtomicBool, Ordering};
+use std::path::Path;
 
 pub(crate) struct InstalledPackagesLayer<'a> {
     pub(crate) aptfile: &'a Aptfile,
-    pub(crate) cache_restored: &'a AtomicBool,
     pub(crate) _section_logger: &'a dyn SectionLogger,
 }
 
@@ -36,55 +29,15 @@ impl<'a> Layer for InstalledPackagesLayer<'a> {
     fn create(
         &mut self,
         context: &BuildContext<Self::Buildpack>,
-        layer_path: &Path,
+        _layer_path: &Path,
     ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {
-        log_step("Creating cache directory");
-
-        let debian_architecture = DebianArchitectureName::from_str(&context.target.arch)
-            .map_err(AptBuildpackError::ParseDebianArchitectureName)?;
-
-        let debian_multiarch_name = DebianMultiarchName::from(debian_architecture);
-
-        let mut env = LayerEnv::new();
-
-        let bin_paths = [
-            layer_path.join("bin"),
-            layer_path.join("usr/bin"),
-            layer_path.join("usr/sbin"),
-        ];
-        prepend_to_env_var(&mut env, "PATH", &bin_paths);
-
-        // support multi-arch and legacy filesystem layouts for debian packages
-        // https://wiki.ubuntu.com/MultiarchSpec
-        let library_paths = [
-            layer_path.join(format!("usr/lib/{debian_multiarch_name}")),
-            layer_path.join("usr/lib"),
-            layer_path.join(format!("lib/{debian_multiarch_name}")),
-            layer_path.join("lib"),
-        ];
-        prepend_to_env_var(&mut env, "LD_LIBRARY_PATH", &library_paths);
-        prepend_to_env_var(&mut env, "LIBRARY_PATH", &library_paths);
-
-        let include_paths = [
-            layer_path.join(format!("usr/include/{debian_multiarch_name}")),
-            layer_path.join("usr/include"),
-        ];
-        prepend_to_env_var(&mut env, "INCLUDE_PATH", &include_paths);
-        prepend_to_env_var(&mut env, "CPATH", &include_paths);
-        prepend_to_env_var(&mut env, "CPPPATH", &include_paths);
-
-        let pkg_config_paths = [
-            layer_path.join(format!("usr/lib/{debian_multiarch_name}/pkgconfig")),
-            layer_path.join("usr/lib/pkgconfig"),
-        ];
-        prepend_to_env_var(&mut env, "PKG_CONFIG_PATH", &pkg_config_paths);
+        log_step("Installing packages from Aptfile");
 
         LayerResultBuilder::new(InstalledPackagesMetadata::new(
             self.aptfile.clone(),
             context.target.os.clone(),
             context.target.arch.clone(),
         ))
-        .env(env)
         .build()
     }
 
@@ -100,8 +53,7 @@ impl<'a> Layer for InstalledPackagesLayer<'a> {
             context.target.arch.clone(),
         );
         if old_meta == new_meta {
-            log_step("Restoring installed packages");
-            self.cache_restored.store(true, Ordering::Relaxed);
+            log_step("Skipping installation, packages already in cache");
             Ok(ExistingLayerStrategy::Keep)
         } else {
             log_step(format!(
@@ -113,25 +65,6 @@ impl<'a> Layer for InstalledPackagesLayer<'a> {
     }
 }
 
-fn prepend_to_env_var<I, T>(env: &mut LayerEnv, name: &str, paths: I)
-where
-    I: IntoIterator<Item = T>,
-    T: Into<OsString>,
-{
-    let separator = ":";
-    env.insert(Scope::All, ModificationBehavior::Delimiter, name, separator);
-    env.insert(
-        Scope::All,
-        ModificationBehavior::Prepend,
-        name,
-        paths
-            .into_iter()
-            .map(Into::into)
-            .collect::<Vec<_>>()
-            .join(separator.as_ref()),
-    );
-}
-
 #[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)]
 pub(crate) struct InstalledPackagesMetadata {
     arch: String,
@@ -160,12 +93,6 @@ impl InstalledPackagesMetadata {
     }
 }
 
-#[derive(Debug, Eq, PartialEq)]
-pub(crate) enum InstalledPackagesState {
-    New(PathBuf),
-    Restored,
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -202,4 +129,25 @@ mod tests {
         ))
         .is_empty());
     }
+
+    #[test]
+    fn test_metadata_guard() {
+        let metadata = InstalledPackagesMetadata::new(
+            Aptfile::from_str("package-1").unwrap(),
+            "linux".to_string(),
+            "amd64".to_string(),
+        );
+        let actual = toml::to_string(&metadata).unwrap();
+        let expected = r#"
+arch = "amd64"
+os = "linux"
+
+[aptfile]
+packages = ["package-1"]
+"#
+        .trim();
+        assert_eq!(expected, actual.trim());
+        let from_toml: InstalledPackagesMetadata = toml::from_str(&actual).unwrap();
+        assert_eq!(metadata, from_toml);
+    }
 }

src/layers/mod.rs

@@ -1 +1,2 @@
+pub(crate) mod environment;
 pub(crate) mod installed_packages;