Skip to content

Conversation

mikeland73
Copy link
Contributor

@mikeland73 mikeland73 commented Feb 8, 2023

Summary

This is stacked on #599

This improves error handling when adding and removing packages. It also uses a new devbox only profile.

  • Uses new global profile stored in ~/.config/devbox/nix/profile
  • Only removes packages if they exist in devbox.json
  • Only adds packages if all packages exist. This mimics behavior of other tools (e.g. apt-get)
  • If only some packages exist it will still remove but show an error. (This is mostly for convenience since it's easy to forget if packages were already removed)

How was it tested?

image

@mikeland73 mikeland73 requested review from gcurtis and loreto February 8, 2023 23:35
Base automatically changed from landau/global to main February 8, 2023 23:48
Copy link
Collaborator

@gcurtis gcurtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a caveat to this is that users can't edit their global devbox.json directly. I'm thinking of the scenario where a user removes a package from devbox.json, but then it's never uninstalled because devbox thinks it didn't install it.

@mikeland73
Copy link
Contributor Author

mikeland73 commented Feb 9, 2023

I think a caveat to this is that users can't edit their global devbox.json directly. I'm thinking of the scenario where a user removes a package from devbox.json, but then it's never uninstalled because devbox thinks it didn't install it.

@gcurtis good call. I wonder if we should use jsonc and put a comment telling people not to edit manually. We could also add a hash or description:

{
"description": "This is a devbox home config file. It does not support all fields from a regular devbox.json. Please do not edit manually",
}

@loreto
Copy link
Contributor

loreto commented Feb 9, 2023

I think a caveat to this is that users can't edit their global devbox.json directly. I'm thinking of the scenario where a user removes a package from devbox.json, but then it's never uninstalled because devbox thinks it didn't install it.

@gcurtis good call. I wonder if we should use jsonc and put a comment telling people not to edit manually. We could also add a hash or description:

{
"description": "This is a devbox home config file. It does not support all fields from a regular devbox.json. Please do not edit manually",
}

Wait, I'm not sure I followed everything. Why can't users edit manually? I think they should be able to.

@mikeland73
Copy link
Contributor Author

@loreto as is, if they remove manually the package won't actually get removed. If they add manually it will get added to shells, but not outside of shell.

@loreto
Copy link
Contributor

loreto commented Feb 9, 2023

Yeah, but why? If we have a devbox.json we can recreate the profile from scratch every time based on what the devbox.json says. Is it because we're using the default nix profile (and we want to let you modify that profile through nix directly as well?). If that's the case, should we just have a different default profile for devbox global (that is separate from the default nix one)

@mikeland73
Copy link
Contributor Author

@loreto using a different profile wont be in path. I was really trying to stick to using the default one.

The issue is that if someone modifies devbox.json and removes a package, we can't be sure if the same package in the profile was added by nix or devbox

@gcurtis
Copy link
Collaborator

gcurtis commented Feb 9, 2023

Why are we trying to use the default Nix profile and PATH? Is it to avoid messing with the user's ~/.zshrc?

If I create the following ~/devbox.json:

{
  "packages": [
    "go",
    "python3"
  ],
  "shell": {
    "init_hook": [
      "unset DEVBOX_SHELL_ENABLED",
      "export DEVBOX_HOME_ENABLED=1"
    ]
  },
  "nixpkgs": {
    "commit": "f80ac848e3d6f0c12c52758c0f25c10c97ca3b62"
  }
}

And then append this to my ~/.zshrc:

if [ "${DEVBOX_SHELL_ENABLED:-0}" = 0 -a "${DEVBOX_HOME_ENABLED:-0}" = 0 ]; then
        exec devbox shell -c ~/devbox.json
fi

I get a global devbox shell when I launch my terminal, and when I do devbox shell I get the project's shell with its packages preferred in my PATH.

There would be some work to make this nicer (maybe silence all output when starting a global shell), but it means that the global shell uses the same exact logic as a project shell (that is, it uses print-dev-env).

@loreto
Copy link
Contributor

loreto commented Feb 9, 2023

I don't mind that it's not in the path either, I think it's reasonable to require users to add a line to their rc files if they want to use devbox global (as you have to do for brew, or for nix itself).

I think I prefer to that if it means:

  1. We can encapsulate nix and not "leak" it for devbox global purposes. (i.e. you source a path that looks like a devbox path)
  2. We can guarantee we can blow up and recreate the profile every time to match the devbox.json

@nbish11
Copy link

nbish11 commented Feb 9, 2023

Why not add a devbox.lock file? Users can still manually edit devbox.json, but devbox itself will read from devbox.lock. If a package has been removed from devbox.json but still exists in devbox.lock, then devbox will remove it. This allows manual editing of packages in addition to using the command line interface. It is similar to how other package managers like Composer and npm work.

@loreto
Copy link
Contributor

loreto commented Feb 9, 2023

Why not add a devbox.lock file? Users can still manually edit devbox.json, but devbox itself will read from devbox.lock. If a package has been removed from devbox.json but still exists in devbox.lock, then devbox will remove it. This allows manual editing of packages in addition to using the command line interface. It is similar to how other package managers like Composer and npm work.

@nbish11 Agreed, I think a lock file is something we should add as an additional improvement (although it would probably be a follow up to this PR).

@mikeland73 mikeland73 changed the title [global] Improve error handling when adding/removing global packages [global] Use non default profile and improve error handling when adding/removing global packages Feb 10, 2023
@mikeland73 mikeland73 merged commit f1b0c6f into main Feb 10, 2023
@mikeland73 mikeland73 deleted the landau/global-2 branch February 10, 2023 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants