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

ysndr · 2024-01-23T17:01:24Z

implements #825

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

mkenigs · 2024-01-23T17:35:05Z

cli/flox/src/commands/environment.rs

@@ -1198,6 +1199,11 @@ 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(_) => formatdoc! {"
+                ❌  Unable to push environment with compatibility errors on your system ({system}).


nonblocking product nit @ghudgins @jennymahmoudi: I don't think I would know as a user what compatibility errors means. And because we're suppressing the underlying error, I don't actually know what to do next. I can't start with flox edit; I'd actually have to flox activate to see the error, and then do flox activate && flox edit && flox push.

Also compatibility errors makes it sound like it's a system specific issue, but this is the same error I get if I do:

flox init # edit the manifest and misspell a package name flox push

I agree with the concerns reg. "compatibility".
The way its implemented now its ensuring that the environment can build on the current system.
I don't know if it becomes much clearer with the error present

(-v will show it FYI)

the AC calls to show both errors

And When the env fails to build Then I see the failure from the build And I see a message about push

ok to add the error in a somewhat raw form now (and later/soon use the prettyfied version)?

yes - we'll redo the error in a future ticket #602

Also compatibility errors makes it sound like it's a system specific issue, but this is the same error I get if I do:

flox init # edit the manifest and misspell a package name flox push

Is this still a valid concern?
Can we have separate error messages for flox push due to system compatibility issues vs misspelled package names?

Yeah it's still a concern. At the moment I don't think it would be very easy to have a different message, but we could come back to that when we re-work our errors. Until then I think it would be simplest to say Unable to push environment that doesn't build or whatever word we're currently using for build

Ok I think it's fine to continue to use build since that's already being propagated through in the other error message.

Unable to push environment with build errors.

mkenigs · 2024-01-23T19:43:53Z

cli/flox/src/commands/environment.rs

+            ManagedEnvironmentError::Build(_) => formatdoc! {"
+                ❌  Unable to push environment with compatibility errors on your system ({system}).
+
+                {err}


The actual error still isn't getting printed:

ERROR: ❌ Unable to push environment with compatibility errors on your system (aarch64-darwin). could not build environment Use 'flox edit' to resolve errors, test with 'flox activate', and 'flox push' again.

Well it is just not the helpful bits...
Fixing this tomorrow
Just wondering if we should display the entire chain or not just the root cause (esp. for these pkgdb ones).
Leaning towards the latter

I bet we'll come back and change it anyways when we rework errors, so either's probably fine for now

Update cli/flox-rust-sdk/src/models/environment/managed_environment.rs Co-authored-by: Matthew Kenigsberg <matthew@floxdev.com>

ysndr · 2024-01-24T12:51:57Z

https://github.com/flox/flox/actions/runs/7639418966/job/20812508062?pr=832#step:6:188
and
https://github.com/flox/flox/actions/runs/7639418966/job/20812508062?pr=832#step:6:315

look like regressions form then upgrade to nixpkgs 23.11

ghudgins · 2024-01-24T15:34:40Z

looks good from my end. I see it now building

ghudgins · 2024-01-24T15:35:21Z

the string isn't matching up yet

mkenigs · 2024-01-24T15:56:37Z

look like regressions form then upgrade to nixpkgs 23.11

I think it's actually a regression from this PR. Prior to this PR, the first time the environment was built was at the line:

  _PKGDB_GA_REGISTRY_REF_OR_REV="${PKGDB_NIXPKGS_REV_OLD?}" \
    "$FLOX_BIN" install hello

where _PKGDB_GA_REGISTRY_REF_OR_REV was set explicitly. Now that the environment is built during a push, the environment is built during the make_empty_remote_env call, at which point _PKGDB_GA_REGISTRY_REF_OR_REV is set to whatever it is throughout the test suite. I think something like:

 _PKGDB_GA_REGISTRY_REF_OR_REV="${PKGDB_NIXPKGS_REV_OLD?}" \
  make_empty_remote_env

should fix it.

Or you can invert all the OLD and NEW in the test since the suite-wide setting of _PKGDB_GA_REGISTRY_REF_OR_REV is to NEW

ysndr · 2024-01-24T16:08:02Z

thanks, yes that makes sense, added the _PKGDB_GA_REGISTRY_REF_OR_REV="${PKGDB_NIXPKGS_REV_OLD?}" \ bit

ysndr · 2024-01-24T16:12:27Z

@ghudgins moved the build error to the top (i guess that's what you were referring to?

ghudgins · 2024-01-24T16:16:46Z

when I ran it, it didn't match the AC. I would want to see the error from the build and then an error underneath about the push with this text.

 <<Build error goes here>>
 
 ERROR: ❌  Unable to push environment with compatibility errors on your system (aarch64-darwin). 
 
 Use `flox edit` to resolve errors, test with 'flox activate', and 'flox push' again.

ysndr · 2024-01-24T16:37:57Z

I thought we had decided on "Unable to push environment with build errors."?

the build error is now above the other stuff

there is some formatting/indentation weirdness but that's going to be fixed with the bigger logging/error polish

jennymahmoudi · 2024-01-24T16:53:29Z

@ysndr you're right. i just updated the AC on #825

cli/flox/src/commands/environment.rs

cli/tests/environment-push.bats

aakropotkin

LGTM

ysndr requested review from zmitchell and mkenigs January 23, 2024 17:01

ysndr self-assigned this Jan 23, 2024

mkenigs approved these changes Jan 23, 2024

View reviewed changes

mkenigs enabled auto-merge January 23, 2024 17:39

mkenigs disabled auto-merge January 23, 2024 17:46

mkenigs requested changes Jan 23, 2024

View reviewed changes

ysndr and others added 2 commits January 24, 2024 12:19

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

0ed2b0d

Update cli/flox-rust-sdk/src/models/environment/managed_environment.rs Co-authored-by: Matthew Kenigsberg <matthew@floxdev.com>

fix: handle build error

46d02c3

ysndr force-pushed the feat/push/prevent-non-building branch from 10e2681 to 151c705 Compare January 24, 2024 11:19

ysndr added 2 commits January 24, 2024 15:47

test: add a test for failing build during push

2b2c2c1

chore: fix new clippy/fmt issues

3dad968

ysndr force-pushed the feat/push/prevent-non-building branch from 151c705 to 3dad968 Compare January 24, 2024 14:47

ci: limit make parallelism in justfile

70f2496

fix: resolve regression du to build at push time

03e428a

ysndr added 2 commits January 24, 2024 17:11

fixup! fix: handle build error

ce5f835

fixup! fix: handle build error

6b511d5

ysndr requested a review from mkenigs January 24, 2024 16:12

mkenigs reviewed Jan 24, 2024

View reviewed changes

cli/flox/src/commands/environment.rs Outdated Show resolved Hide resolved

Update cli/flox/src/commands/environment.rs

f9c6a8a

mkenigs approved these changes Jan 24, 2024

View reviewed changes

mkenigs reviewed Jan 24, 2024

View reviewed changes

cli/tests/environment-push.bats Outdated Show resolved Hide resolved

Update cli/tests/environment-push.bats

5f2bed7

mkenigs approved these changes Jan 24, 2024

View reviewed changes

mkenigs enabled auto-merge January 24, 2024 23:53

zmitchell approved these changes Jan 25, 2024

View reviewed changes

mkenigs added this pull request to the merge queue Jan 25, 2024

aakropotkin approved these changes Jan 25, 2024

View reviewed changes

bryanhonof approved these changes Jan 25, 2024

View reviewed changes

Merged via the queue into main with commit 3d046ad Jan 25, 2024
15 checks passed

mkenigs deleted the feat/push/prevent-non-building branch January 25, 2024 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ysndr commented Jan 23, 2024

mkenigs Jan 23, 2024

ysndr Jan 23, 2024

ghudgins Jan 23, 2024

ysndr Jan 23, 2024

ghudgins Jan 23, 2024

jennymahmoudi Jan 23, 2024

mkenigs Jan 23, 2024

jennymahmoudi Jan 23, 2024

mkenigs Jan 23, 2024

ysndr Jan 23, 2024

mkenigs Jan 23, 2024

ysndr commented Jan 24, 2024

ghudgins commented Jan 24, 2024

ghudgins commented Jan 24, 2024

mkenigs commented Jan 24, 2024 •

edited

ysndr commented Jan 24, 2024

ysndr commented Jan 24, 2024

ghudgins commented Jan 24, 2024

ysndr commented Jan 24, 2024

jennymahmoudi commented Jan 24, 2024

aakropotkin left a comment

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

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

Conversation

ysndr commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysndr commented Jan 24, 2024

ghudgins commented Jan 24, 2024

ghudgins commented Jan 24, 2024

mkenigs commented Jan 24, 2024 • edited

ysndr commented Jan 24, 2024

ysndr commented Jan 24, 2024

ghudgins commented Jan 24, 2024

ysndr commented Jan 24, 2024

jennymahmoudi commented Jan 24, 2024

aakropotkin left a comment

Choose a reason for hiding this comment

mkenigs commented Jan 24, 2024 •

edited