feat: add build check to ManagedEnvironment::{push,push_new}

Justfile

@@ -34,20 +34,20 @@ cargo_test_invocation := "PKGDB_BIN=${PKGDB_BIN} cargo test --workspace"
 
 # Build the compilation database
 build-cdb:
-    @make -C pkgdb -j -s cdb
+    @make -C pkgdb -j 8 -s cdb
 
 # Build only pkgdb
 @build-pkgdb:
-    make -C pkgdb -j;
+    make -C pkgdb -j 8;
 
 # Build pkgdb with debug symbols
 @build-pkgdb-debug:
     # Note that you need to clean pkgdb first
-    make -C pkgdb -j -s DEBUG=1
+    make -C pkgdb -j 8 -s DEBUG=1
 
 # Clean the pkgdb build cache
 @clean-pkgdb:
-    make -C pkgdb -j -s clean
+    make -C pkgdb -j 8 -s clean
 
 # Build only flox
 @build-cli: build-pkgdb
@@ -61,7 +61,7 @@ build: build-cli
 
 # Run the pkgdb tests
 @test-pkgdb: build-pkgdb
-    make -C pkgdb -j tests;
+    make -C pkgdb -j 8 tests;
     make -C pkgdb check;
 
 # Run the end-to-end test suite
@@ -94,7 +94,7 @@ build: build-cli
 test-cli: impure-tests integ-tests functional-tests
 
 # Run the entire test suite, including impure unit tests
-test-all: test-pkgdb impure-tests integ-tests functional-tests 
+test-all: test-pkgdb impure-tests integ-tests functional-tests
 
 
 # ---------------------------------------------------------------------------- #

cli/flox-rust-sdk/src/models/environment/managed_environment.rs

@@ -127,6 +127,9 @@ pub enum ManagedEnvironmentError {
     #[error("could not link environment")]
     Link(#[source] CoreEnvironmentError),
 
+    #[error("could not build environment")]
+    Build(#[source] CoreEnvironmentError),
+
     #[error("could not read manifest")]
     ReadManifest(#[source] GenerationsError),
 
@@ -877,8 +880,17 @@ impl ManagedEnvironment {
     ) -> Result<Self, ManagedEnvironmentError> {
         // path of the original .flox directory
         let dot_flox_path = path_environment.path.clone();
+        let path_pointer = path_environment.pointer.clone();
+        let name = path_environment.name();
+
+        let mut core_environment = path_environment.into_core_environment();
+
+        // Ensure the environment builds before we push it
+        core_environment
+            .build(flox)
+            .map_err(ManagedEnvironmentError::Build)?;
 
-        let pointer = ManagedPointer::new(owner, path_environment.name(), &flox.floxhub);
+        let pointer = ManagedPointer::new(owner, name, &flox.floxhub);
 
         let checkedout_floxmeta_path = tempfile::tempdir_in(&flox.temp_dir).unwrap().into_path();
         let temp_floxmeta_path = tempfile::tempdir_in(&flox.temp_dir).unwrap().into_path();
@@ -898,7 +910,7 @@ impl ManagedEnvironment {
             checkedout_floxmeta_path,
             temp_floxmeta_path,
             remote_branch_name(&pointer),
-            &path_environment.pointer,
+            &path_pointer,
         )
         .map_err(ManagedEnvironmentError::InitializeFloxmeta)?;
 
@@ -909,10 +921,7 @@ impl ManagedEnvironment {
             .map_err(ManagedEnvironmentError::CreateFloxmetaDir)?;
 
         generations
-            .add_generation(
-                &mut path_environment.into_core_environment(),
-                "Add first generation".to_string(),
-            )
+            .add_generation(&mut core_environment, "Add first generation".to_string())
             .map_err(ManagedEnvironmentError::CommitGeneration)?;
 
         temp_floxmeta_git
@@ -949,10 +958,23 @@ impl ManagedEnvironment {
         Ok(env)
     }
 
-    pub fn push(&mut self, force: bool) -> Result<(), ManagedEnvironmentError> {
+    pub fn push(&mut self, flox: &Flox, force: bool) -> Result<(), ManagedEnvironmentError> {
         let project_branch = branch_name(&self.pointer, &self.path);
         let sync_branch = remote_branch_name(&self.pointer);
 
+        // Ensure the environment builds before we push it
+        // Usually we don't create generations unless they build,
+        // but that is not always the case.
+        // If a user pulls an environment that is broken on their system, we may
+        // create a "broken" generation.
+        // That generation could have a divergent manifest and lock,
+        // or it could fail to build.
+        // So we have to verify we don't have a "broken" generation before pushing.
+        {
+            let mut env = self.get_current_generation(flox)?;
+            env.build(flox).map_err(ManagedEnvironmentError::Build)?;
+        }
+
         // Fetch the remote branch into sync branch
         self.floxmeta
             .git

cli/flox-rust-sdk/src/models/environment/remote_environment.rs

@@ -133,7 +133,7 @@ impl Environment for RemoteEnvironment {
     ) -> Result<InstallationAttempt, EnvironmentError2> {
         let result = self.inner.install(packages, flox)?;
         self.inner
-            .push(false)
+            .push(flox, false)
             .map_err(RemoteEnvironmentError::UpdateUpstream)?;
         // TODO: clean up git branch for temporary environment
         Ok(result)
@@ -147,7 +147,7 @@ impl Environment for RemoteEnvironment {
     ) -> Result<String, EnvironmentError2> {
         let result = self.inner.uninstall(packages, flox)?;
         self.inner
-            .push(false)
+            .push(flox, false)
             .map_err(RemoteEnvironmentError::UpdateUpstream)?;
         Ok(result)
     }
@@ -159,7 +159,7 @@ impl Environment for RemoteEnvironment {
             return Ok(result);
         }
         self.inner
-            .push(false)
+            .push(flox, false)
             .map_err(RemoteEnvironmentError::UpdateUpstream)?;
         Ok(result)
     }
@@ -172,7 +172,7 @@ impl Environment for RemoteEnvironment {
     ) -> Result<UpdateResult, EnvironmentError2> {
         let result = self.inner.update(flox, inputs)?;
         self.inner
-            .push(false)
+            .push(flox, false)
             .map_err(RemoteEnvironmentError::UpdateUpstream)?;
         Ok(result)
     }
@@ -185,7 +185,7 @@ impl Environment for RemoteEnvironment {
     ) -> Result<UpgradeResult, EnvironmentError2> {
         let result = self.inner.upgrade(flox, groups_or_iids)?;
         self.inner
-            .push(false)
+            .push(flox, false)
             .map_err(RemoteEnvironmentError::UpdateUpstream)?;
         Ok(result)
     }

cli/flox/src/commands/environment.rs

@@ -1,4 +1,5 @@
 use std::borrow::Cow;
+use std::error::Error;
 use std::fmt::Display;
 use std::fs::{self, File};
 use std::io::{stdin, stdout, Write};
@@ -1131,7 +1132,7 @@ impl Push {
     ) -> Result<()> {
         let mut env = ManagedEnvironment::open(flox, managed_pointer.clone(), dir.join(DOT_FLOX))
             .context("Could not open environment")?;
-        env.push(force)
+        env.push(flox, force)
             .map_err(|err| Self::convert_error(err, managed_pointer, false))?;
 
         Ok(())
@@ -1165,6 +1166,15 @@ impl Push {
         let owner = &pointer.owner;
         let name = &pointer.name;
 
+        fn error_chain(mut e: &dyn Error) -> String {
+            let mut msg = e.to_string();
+            while let Some(source) = e.source() {
+                e = source;
+                msg.push_str(&format!(": {}", e));
+            }
+            msg
+        }
+
         let message = match err {
             ManagedEnvironmentError::AccessDenied => formatdoc! {"
                 ❌  You do not have permission to write to {owner}/{name}
@@ -1175,6 +1185,14 @@ impl Push {
                 To rename your environment: 'flox edit --name <new name>'
                 To pull and manually re-apply your changes: 'flox delete && flox pull -r {owner}/{name}'
             "}.into(),
+            ManagedEnvironmentError::Build(ref err) => formatdoc! {"
+                {err}
+
+                ❌  Unable to push environment with build errors.
+
+                Use 'flox edit' to resolve errors, test with 'flox activate', and 'flox push' again.",
+                err = error_chain(err)
+            }.into(),
             _ => None
         };
 

cli/flox/src/utils/openers.rs

@@ -34,7 +34,12 @@ impl Browser {
             "linux" => {
                 let path_var =
                     env::var("PATH").map_err(|_| "Could not read PATH variable".to_string())?;
-                let Some((path, _)) = first_in_path([OPENERS, BROWSER_OPENERS].concat(), env::split_paths(&path_var)) else { return Err("No opener found in PATH".to_string()) };
+                let Some((path, _)) = first_in_path(
+                    [OPENERS, BROWSER_OPENERS].concat(),
+                    env::split_paths(&path_var),
+                ) else {
+                    return Err("No opener found in PATH".to_string());
+                };
                 Self(path)
             },
             "macos" => Self(PathBuf::from("/usr/bin/open")),
@@ -75,6 +80,6 @@ where
     I::IntoIter: Clone,
 {
     path.into_iter()
-        .cartesian_product(candidates.into_iter())
+        .cartesian_product(candidates)
         .find(|(path, editor)| path.join(editor).exists())
 }

cli/tests/environment-managed.bats

@@ -319,6 +319,7 @@ EOF
 }
 
 @test "sanity check upgrade works for managed environments" {
+  _PKGDB_GA_REGISTRY_REF_OR_REV="${PKGDB_NIXPKGS_REV_OLD?}" \
   make_empty_remote_env
 
   _PKGDB_GA_REGISTRY_REF_OR_REV="${PKGDB_NIXPKGS_REV_OLD?}" \

cli/tests/environment-push.bats

@@ -180,3 +180,27 @@ function update_dummy_env() {
   assert_line "emacs"
   popd > /dev/null || return
 }
+
+# bats test_tags=push:broken
+@test "push: broken: if you attempt to flox push an environment that fails to build then the push should fail with a message." {
+  make_dummy_floxmeta "owner"
+
+  run "$FLOX_BIN" init
+
+  init_system=
+  # replace linux with darwin or darwin with linux
+  if [ -z "${NIX_SYSTEM##*-linux}" ]; then
+    init_system="${NIX_SYSTEM%%-linux}-darwin"
+  elif [ -z "${NIX_SYSTEM#*-darwin}" ]; then
+    init_system="${NIX_SYSTEM%%-darwin}-linux"
+  else
+    echo "unknown system: '$NIX_SYSTEM'"
+    exit 1
+  fi
+
+  tomlq --in-place -t ".options.systems=[\"$init_system\"]" .flox/env/manifest.toml
+
+  run "$FLOX_BIN" push --owner owner # dummy owner
+  assert_failure
+  assert_output --partial "Unable to push environment with build errors:"
+}

cli/tests/environment-remote.bats

@@ -222,6 +222,7 @@ EOF
 # ---------------------------------------------------------------------------- #
 
 @test "sanity check upgrade works for remote environments" {
+  _PKGDB_GA_REGISTRY_REF_OR_REV="${PKGDB_NIXPKGS_REV_OLD?}" \
   make_empty_remote_env
 
   _PKGDB_GA_REGISTRY_REF_OR_REV="${PKGDB_NIXPKGS_REV_OLD?}" \