Fix install.environment not expanding some vars

The environment variables being fed into the substitution engine did not
include the build environment variables, such as
`$SPK_PKG_VERSION_MAJOR`. Users expect to be able to use these vars when
generating activation scripts.

Add some tests to verify expected substitution behavior.

`get_package_build_env` refactored into a `trait Package` method since
now this needs to be called that lives in the schema crate.

Signed-off-by: J Robert Ray <jrray@jrray.org>

crates/spk-build/src/build/binary.rs

@@ -23,7 +23,6 @@ use spk_schema::foundation::format::FormatIdent;
 use spk_schema::foundation::ident_build::Build;
 use spk_schema::foundation::ident_component::Component;
 use spk_schema::foundation::option_map::OptionMap;
-use spk_schema::foundation::version::VERSION_SEP;
 use spk_schema::ident::{PkgRequest, PreReleasePolicy, RangeIdent, RequestedBy, VersionIdent};
 use spk_schema::variant::Override;
 use spk_schema::{
@@ -638,7 +637,7 @@ where
         let mut cmd = cmd.into_std();
         cmd.envs(self.environment.drain());
         cmd.envs(options.as_ref().to_environment());
-        cmd.envs(get_package_build_env(package));
+        cmd.envs(package.get_build_env());
         cmd.env("PREFIX", &self.prefix);
         // force the base environment to be setup using bash, so that the
         // spfs startup and build environment are predictable and consistent
@@ -721,43 +720,6 @@ where
     }
 }
 
-/// Return the environment variables to be set for a build of the given package spec.
-pub fn get_package_build_env<P>(spec: &P) -> HashMap<String, String>
-where
-    P: Package,
-{
-    let mut env = HashMap::with_capacity(8);
-    env.insert("SPK_PKG".to_string(), spec.ident().to_string());
-    env.insert("SPK_PKG_NAME".to_string(), spec.name().to_string());
-    env.insert("SPK_PKG_VERSION".to_string(), spec.version().to_string());
-    env.insert(
-        "SPK_PKG_BUILD".to_string(),
-        spec.ident().build().to_string(),
-    );
-    env.insert(
-        "SPK_PKG_VERSION_MAJOR".to_string(),
-        spec.version().major().to_string(),
-    );
-    env.insert(
-        "SPK_PKG_VERSION_MINOR".to_string(),
-        spec.version().minor().to_string(),
-    );
-    env.insert(
-        "SPK_PKG_VERSION_PATCH".to_string(),
-        spec.version().patch().to_string(),
-    );
-    env.insert(
-        "SPK_PKG_VERSION_BASE".to_string(),
-        spec.version()
-            .parts
-            .iter()
-            .map(u32::to_string)
-            .collect::<Vec<_>>()
-            .join(VERSION_SEP),
-    );
-    env
-}
-
 /// Commit changes discovered in the runtime as a package.
 ///
 /// Only the changes also present in `filter` will be committed. It is

crates/spk-build/src/build/mod.rs

@@ -11,7 +11,6 @@ pub use binary::{
     build_spec_path,
     commit_component_layers,
     component_marker_path,
-    get_package_build_env,
     source_package_path,
     BinaryPackageBuilder,
     BuildError,

crates/spk-build/src/build/sources.rs

@@ -136,7 +136,7 @@ where
     std::fs::create_dir_all(source_dir)
         .map_err(|err| Error::DirectoryCreateError(source_dir.to_owned(), err))?;
 
-    let env = super::binary::get_package_build_env(spec);
+    let env = spec.get_build_env();
     for source in spec.sources().iter() {
         let target_dir = match source.subdir() {
             Some(subdir) => subdir.to_path(source_dir),

crates/spk-build/src/lib.rs

@@ -17,7 +17,6 @@ pub use build::{
     build_spec_path,
     commit_component_layers,
     component_marker_path,
-    get_package_build_env,
     source_package_path,
     validate_source_changeset,
     BinaryPackageBuilder,

crates/spk-cli/cmd-build/src/cmd_build_test/environment.rs

@@ -0,0 +1,99 @@
+// Copyright (c) Sony Pictures Imageworks, et al.
+// SPDX-License-Identifier: Apache-2.0
+// https://github.com/imageworks/spk
+
+use rstest::rstest;
+use spfs::storage::{LayerStorage, ManifestStorage, PayloadStorage};
+use spk_cli_common::BuildArtifact;
+use spk_schema::foundation::fixtures::*;
+use spk_schema::ident_component::Component;
+use spk_storage::fixtures::*;
+
+use crate::try_build_package;
+
+#[rstest]
+#[case::simple(
+    "- { set: TEST_VAR, value: \"${SPK_PKG_VERSION_MAJOR}\" }",
+    "export TEST_VAR=\"1\"\n"
+)]
+#[case::unexpanded_and_expanded(
+    "- { set: TEST_VAR, value: \"$$HOME/${SPK_PKG_VERSION_MAJOR}.${SPK_PKG_VERSION_MINOR}\" }",
+    "export TEST_VAR=\"$HOME/1.2\"\n"
+)]
+#[tokio::test]
+async fn basic_environment_generation(
+    tmpdir: tempfile::TempDir,
+    #[case] env_spec: &str,
+    #[case] expected: &str,
+) {
+    let rt = spfs_runtime().await;
+
+    let (_, result) = try_build_package!(
+        tmpdir,
+        "test.spk.yaml",
+        format!(
+            r#"
+pkg: test/1.2.3
+api: v0/package
+build:
+    script:
+        - true
+install:
+    environment:
+        {env_spec}
+        "#
+        ),
+    );
+
+    let mut result = result.expect("Expected build to succeed");
+
+    // Only care about binary builds (not source builds)
+    result
+        .created_builds
+        .artifacts
+        .retain(|(_, artifact)| matches!(artifact, BuildArtifact::Binary(_, _, _)));
+
+    assert_eq!(
+        result.created_builds.artifacts.len(),
+        1,
+        "Expected one build to be created"
+    );
+
+    // Check the generated activation script
+
+    let BuildArtifact::Binary(build, _, _) = &result.created_builds.artifacts[0].1 else {
+        panic!("Expected binary build");
+    };
+
+    let digest = *rt
+        .tmprepo
+        .read_components(build)
+        .await
+        .unwrap()
+        .get(&Component::Run)
+        .unwrap();
+
+    let spk_storage::RepositoryHandle::SPFS(repo) = &*rt.tmprepo else {
+        panic!("Expected SPFS repo");
+    };
+
+    let layer = repo.read_layer(digest).await.unwrap();
+
+    let manifest = repo
+        .read_manifest(
+            *layer
+                .manifest()
+                .expect("Layer should have a manifest in this test"),
+        )
+        .await
+        .unwrap()
+        .to_tracking_manifest();
+
+    let entry = manifest.get_path("etc/spfs/startup.d/spk_test.sh").unwrap();
+
+    let (mut payload, _filename) = repo.open_payload(entry.object).await.unwrap();
+    let mut writer: Vec<u8> = vec![];
+    tokio::io::copy(&mut payload, &mut writer).await.unwrap();
+    let contents = String::from_utf8(writer).unwrap();
+    assert_eq!(contents, expected);
+}

crates/spk-cli/cmd-build/src/cmd_build_test/mod.rs

@@ -2,9 +2,6 @@
 // SPDX-License-Identifier: Apache-2.0
 // https://github.com/imageworks/spk
 
-use std::fs::File;
-use std::io::Write;
-
 use clap::Parser;
 use rstest::rstest;
 use spfs::storage::prelude::*;
@@ -18,6 +15,7 @@ use spk_storage::fixtures::*;
 use super::Build;
 use crate::{build_package, try_build_package};
 
+mod environment;
 mod variant_filter;
 
 #[derive(Parser)]

crates/spk-cli/cmd-build/src/macros.rs

@@ -53,7 +53,8 @@ macro_rules! try_build_package {
         // Leak `filename` for convenience.
         let filename = Box::leak(Box::new($tmpdir.path().join($filename)));
         {
-            let mut file = File::create(&filename).unwrap();
+            let mut file = std::fs::File::create(&filename).unwrap();
+            use std::io::Write;
             file.write_all($recipe.as_bytes()).unwrap();
         }
 

crates/spk-schema/src/environ.rs

@@ -111,9 +111,12 @@ impl EnvOp {
 
     /// Returns the EnvOp object with expanded env var, if any
     pub fn to_expanded(&self, env_vars: &HashMap<String, String>) -> Self {
-        let value = self
-            .value()
-            .map(|val| shellexpand::env_with_context_no_errors(val, |s: &str| env_vars.get(s)));
+        let value = self.value().map(|val| {
+            shellexpand::env_with_context_no_errors(val, |s: &str| {
+                dbg!(env_vars);
+                env_vars.get(dbg!(s))
+            })
+        });
 
         match value {
             Some(val) => self.update_value(val.into_owned()),

crates/spk-schema/src/package.rs

@@ -3,9 +3,11 @@
 // https://github.com/imageworks/spk
 
 use std::borrow::Cow;
+use std::collections::HashMap;
 
 use spk_schema_foundation::ident_build::Build;
 use spk_schema_foundation::spec_ops::{Named, Versioned};
+use spk_schema_foundation::version::VERSION_SEP;
 use spk_schema_ident::BuildIdent;
 
 use super::RequirementsList;
@@ -61,6 +63,40 @@ pub trait Package:
     /// Identify the requirements for a build of this package.
     fn get_build_requirements(&self) -> crate::Result<Cow<'_, RequirementsList>>;
 
+    /// Return the environment variables to be set for a build of the given package spec.
+    fn get_build_env(&self) -> HashMap<String, String> {
+        let mut env = HashMap::with_capacity(8);
+        env.insert("SPK_PKG".to_string(), self.ident().to_string());
+        env.insert("SPK_PKG_NAME".to_string(), self.name().to_string());
+        env.insert("SPK_PKG_VERSION".to_string(), self.version().to_string());
+        env.insert(
+            "SPK_PKG_BUILD".to_string(),
+            self.ident().build().to_string(),
+        );
+        env.insert(
+            "SPK_PKG_VERSION_MAJOR".to_string(),
+            self.version().major().to_string(),
+        );
+        env.insert(
+            "SPK_PKG_VERSION_MINOR".to_string(),
+            self.version().minor().to_string(),
+        );
+        env.insert(
+            "SPK_PKG_VERSION_PATCH".to_string(),
+            self.version().patch().to_string(),
+        );
+        env.insert(
+            "SPK_PKG_VERSION_BASE".to_string(),
+            self.version()
+                .parts
+                .iter()
+                .map(u32::to_string)
+                .collect::<Vec<_>>()
+                .join(VERSION_SEP),
+        );
+        env
+    }
+
     /// Requests that must be met to use this package
     fn runtime_requirements(&self) -> Cow<'_, RequirementsList>;
 

crates/spk-schema/src/v0/spec.rs

@@ -569,16 +569,6 @@ impl Recipe for Spec<VersionIdent> {
 
         updated.meta.update_metadata(&config.metadata)?;
 
-        // Expand env variables from EnvOp.
-        let mut updated_ops = Vec::new();
-        let build_env_vars = build_env.env_vars();
-        for op in updated.install.environment.iter() {
-            updated_ops.push(op.to_expanded(&build_env_vars));
-        }
-
-        updated.install.environment.clear();
-        updated.install.environment.append(&mut updated_ops);
-
         let mut missing_build_requirements = HashMap::new();
         let mut missing_runtime_requirements: HashMap<OptNameBuf, (String, Option<String>)> =
             HashMap::new();
@@ -649,7 +639,18 @@ impl Recipe for Spec<VersionIdent> {
         // by `build_env`. The digest is expected to be based solely on the
         // input options and recipe.
         let digest = self.build_digest(variant.input_variant())?;
-        Ok(updated.map_ident(|i| i.into_build(Build::BuildId(digest))))
+        let mut build = updated.map_ident(|i| i.into_build(Build::BuildId(digest)));
+
+        // Expand env variables from EnvOp.
+        let mut updated_ops = Vec::new();
+        let mut build_env_vars = build_env.env_vars();
+        build_env_vars.extend(build.get_build_env());
+        for op in build.install.environment.iter() {
+            updated_ops.push(op.to_expanded(&build_env_vars));
+        }
+        build.install.environment = updated_ops;
+
+        Ok(build)
     }
 
     fn metadata(&self) -> &Meta {