Code review changes

Cargo.toml

@@ -4,9 +4,9 @@ edition = "2021"
 rust-version = "1.76"
 
 [dependencies]
+commons = { git = "https://github.com/heroku/buildpacks-ruby", branch = "main" }
 libcnb = "=0.18.0"
 regex-lite = "0.1"
-serde = "1"
 
 [dev-dependencies]
 libcnb-test = "=0.18.0"

src/aptfile.rs

@@ -1,68 +1,44 @@
-use serde::{Deserialize, Serialize};
 use std::collections::HashSet;
-use std::ffi::OsStr;
-use std::ops::Deref;
 use std::str::FromStr;
 use std::sync::OnceLock;
 
-#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)]
+#[derive(Debug, Eq, PartialEq)]
 pub(crate) struct Aptfile {
-    pub(crate) packages: HashSet<DebianPackage>,
+    packages: HashSet<DebianPackageName>,
 }
 
-#[derive(Debug)]
-pub(crate) struct ParseAptfileError(ParseDebianPackageError);
+#[derive(Debug, PartialEq)]
+pub(crate) struct ParseAptfileError(ParseDebianPackageNameError);
 
 impl FromStr for Aptfile {
     type Err = ParseAptfileError;
 
     fn from_str(value: &str) -> Result<Self, Self::Err> {
         value
             .lines()
-            .filter_map(|mut line| {
-                line = line.trim();
-                if line.starts_with('#') || line.is_empty() {
-                    None
-                } else {
-                    Some(line)
-                }
-            })
-            .map(DebianPackage::from_str)
+            .map(str::trim)
+            .filter(|line| !line.starts_with('#') && !line.is_empty())
+            .map(DebianPackageName::from_str)
             .collect::<Result<HashSet<_>, _>>()
             .map_err(ParseAptfileError)
             .map(|packages| Aptfile { packages })
     }
 }
 
-#[derive(Debug, Clone, Eq, PartialEq, Hash, Serialize, Deserialize)]
-#[serde(transparent)]
-pub(crate) struct DebianPackage(String);
-
-impl Deref for DebianPackage {
-    type Target = String;
-
-    fn deref(&self) -> &Self::Target {
-        &self.0
-    }
-}
-
-impl AsRef<OsStr> for DebianPackage {
-    fn as_ref(&self) -> &OsStr {
-        OsStr::new(&self.0)
-    }
-}
+#[derive(Debug, Eq, PartialEq, Hash)]
+pub(crate) struct DebianPackageName(String);
 
 #[derive(Debug, PartialEq)]
-pub(crate) struct ParseDebianPackageError(String);
+pub(crate) struct ParseDebianPackageNameError(String);
 
-impl FromStr for DebianPackage {
-    type Err = ParseDebianPackageError;
+impl FromStr for DebianPackageName {
+    type Err = ParseDebianPackageNameError;
 
     fn from_str(value: &str) -> Result<Self, Self::Err> {
         if debian_package_name_regex().is_match(value) {
-            Ok(DebianPackage(value.to_string()))
+            Ok(DebianPackageName(value.to_string()))
         } else {
-            Err(ParseDebianPackageError(value.to_string()))
+            Err(ParseDebianPackageNameError(value.to_string()))
         }
     }
 }
@@ -85,42 +61,75 @@ mod tests {
     use indoc::indoc;
 
     #[test]
-    fn parse_valid_debian_package() {
-        let debian_package = DebianPackage::from_str("package-name").unwrap();
-        assert_eq!(*debian_package, "package-name".to_string());
+    fn parse_valid_debian_package_name() {
+        let valid_names = [
+            "a0",             // min length, starting with number
+            "0a",             // min length, starting with letter
+            "g++",            // alphanumeric to start followed by non-alphanumeric characters
+            "libevent-2.1-6", // just a mix of allowed characters
+            "a0+.-",          // all the allowed characters
+        ];
+        for valid_name in valid_names {
+            assert_eq!(
+                DebianPackageName::from_str(valid_name).unwrap(),
+                DebianPackageName(valid_name.to_string())
+            );
+        }
     }
     #[test]
-    fn parse_invalid_debian_package() {
+    fn parse_invalid_debian_package_name() {
         let invalid_names = [
-            "a",    // too short
-            "+a",   // can't start with non-alphanumeric character
-            "ab_c", // can't contain invalid characters
-            "aBc",  // uppercase is not allowed
+            "a",               // too short
+            "+a",              // can't start with non-alphanumeric character
+            "ab_c",            // can't contain invalid characters
+            "aBc",             // uppercase is not allowed
+            "package=1.2.3-1", // versioning is not allowed, package name only
         ];
         for invalid_name in invalid_names {
             assert_eq!(
-                DebianPackage::from_str(invalid_name).unwrap_err(),
-                ParseDebianPackageError(invalid_name.to_string())
+                DebianPackageName::from_str(invalid_name).unwrap_err(),
+                ParseDebianPackageNameError(invalid_name.to_string())
             );
         }
     }
 
     #[test]
     fn parse_aptfile() {
         let aptfile = Aptfile::from_str(indoc! { "
-            # comment line
+           # comment line
+               # comment line with leading whitespace
 
             package-name-1
             package-name-2
-            
+
+            # Package name has leading and trailing whitespace
+               package-name-3  \t
+            # Duplicates are allowed (at least for now)
+            package-name-1
+
         " })
         .unwrap();
         assert_eq!(
             aptfile.packages,
             HashSet::from([
-                DebianPackage::from_str("package-name-1").unwrap(),
-                DebianPackage::from_str("package-name-2").unwrap(),
+                DebianPackageName("package-name-1".to_string()),
+                DebianPackageName("package-name-2".to_string()),
+                DebianPackageName("package-name-3".to_string()),
             ])
         );
     }
+
+    #[test]
+    fn parse_invalid_aptfile() {
+        let error = Aptfile::from_str(indoc! { "
+           invalid package name!
+        " })
+        .unwrap_err();
+        assert_eq!(
+            error,
+            ParseAptfileError(ParseDebianPackageNameError(
+                "invalid package name!".to_string()
+            ))
+        );
+    }
 }

src/errors.rs

@@ -1,13 +1,15 @@
+use crate::aptfile::ParseAptfileError;
+
 #[derive(Debug)]
 #[allow(clippy::enum_variant_names)]
 pub(crate) enum AptBuildpackError {
     DetectAptfile(std::io::Error),
     ReadAptfile(std::io::Error),
-    ParseAptfile,
+    ParseAptfile(ParseAptfileError),
 }
 
 impl From<AptBuildpackError> for libcnb::Error<AptBuildpackError> {
     fn from(value: AptBuildpackError) -> Self {
-        libcnb::Error::BuildpackError(value)
+        Self::BuildpackError(value)
     }
 }

src/main.rs

@@ -1,10 +1,12 @@
 use crate::aptfile::Aptfile;
 use crate::errors::AptBuildpackError;
+use commons::output::build_log::{BuildLog, Logger};
 use libcnb::build::{BuildContext, BuildResult, BuildResultBuilder};
 use libcnb::detect::{DetectContext, DetectResult, DetectResultBuilder};
 use libcnb::generic::{GenericMetadata, GenericPlatform};
 use libcnb::{buildpack_main, Buildpack};
 use std::fs;
+use std::io::stdout;
 
 #[cfg(test)]
 use libcnb_test as _;
@@ -24,15 +26,19 @@ impl Buildpack for AptBuildpack {
     type Error = AptBuildpackError;
 
     fn detect(&self, context: DetectContext<Self>) -> libcnb::Result<DetectResult, Self::Error> {
-        let exists = context
+        let aptfile_exists = context
             .app_dir
             .join(APTFILE_PATH)
             .try_exists()
             .map_err(AptBuildpackError::DetectAptfile)?;
 
-        if exists {
+        if aptfile_exists {
             DetectResultBuilder::pass().build()
         } else {
+            BuildLog::new(stdout())
+                .without_buildpack_name()
+                .announce()
+                .warning("No Aptfile found.");
             DetectResultBuilder::fail().build()
         }
     }
@@ -41,7 +47,7 @@ impl Buildpack for AptBuildpack {
         let _aptfile: Aptfile = fs::read_to_string(context.app_dir.join(APTFILE_PATH))
             .map_err(AptBuildpackError::ReadAptfile)?
             .parse()
-            .map_err(|_| AptBuildpackError::ParseAptfile)?;
+            .map_err(AptBuildpackError::ParseAptfile)?;
 
         BuildResultBuilder::new().build()
     }

tests/integration_test.rs

@@ -3,3 +3,76 @@
 
 // Required due to: https://github.com/rust-lang/rust/issues/95513
 #![allow(unused_crate_dependencies)]
+
+use libcnb::data::buildpack_id;
+use libcnb_test::{
+    assert_contains, BuildConfig, BuildpackReference, PackResult, TestContext, TestRunner,
+};
+use std::path::PathBuf;
+
+#[test]
+#[ignore = "integration test"]
+fn test_successful_detection() {
+    apt_integration_test_with_config(
+        "./fixtures/basic",
+        |config| {
+            config.expected_pack_result(PackResult::Success);
+        },
+        |_| {},
+    );
+}
+
+#[test]
+#[ignore = "integration test"]
+fn test_failed_detection() {
+    apt_integration_test_with_config(
+        "./fixtures/no_aptfile",
+        |config| {
+            config.expected_pack_result(PackResult::Failure);
+        },
+        |ctx| {
+            assert_contains!(ctx.pack_stdout, "No Aptfile found.");
+        },
+    );
+}
+
+const DEFAULT_BUILDER: &str = "heroku/builder:22";
+
+fn get_integration_test_builder() -> String {
+    std::env::var("INTEGRATION_TEST_CNB_BUILDER").unwrap_or(DEFAULT_BUILDER.to_string())
+}
+
+fn apt_integration_test_with_config(
+    fixture: &str,
+    with_config: fn(&mut BuildConfig),
+    test_body: fn(TestContext),
+) {
+    integration_test_with_config(
+        fixture,
+        with_config,
+        test_body,
+        &[BuildpackReference::WorkspaceBuildpack(buildpack_id!(
+            "heroku/apt"
+        ))],
+    );
+}
+
+fn integration_test_with_config(
+    fixture: &str,
+    with_config: fn(&mut BuildConfig),
+    test_body: fn(TestContext),
+    buildpacks: &[BuildpackReference],
+) {
+    let cargo_manifest_dir = std::env::var("CARGO_MANIFEST_DIR")
+        .map(PathBuf::from)
+        .expect("The CARGO_MANIFEST_DIR should be automatically set by Cargo when running tests but it was not");
+
+    let builder = get_integration_test_builder();
+    let app_dir = cargo_manifest_dir.join("tests").join(fixture);
+
+    let mut build_config = BuildConfig::new(builder, app_dir);
+    build_config.buildpacks(buildpacks);
+    with_config(&mut build_config);
+
+    TestRunner::default().build(build_config, test_body);
+}