Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[shellenv] filter out buildInput paths from PATH #1091

Merged
merged 5 commits into from
Jun 9, 2023

Conversation

savil
Copy link
Collaborator

@savil savil commented Jun 2, 2023

Summary

problem
Doing devbox rm <package> within a devbox shell will usually still have the package's binary accessible in PATH. This is confusing and unexpected.

cause

In shellenv's PATH, we now have 3 sources of paths:

  1. .devbox/virtenv/.wrappers/bin
  2. .devbox/nix/profile/default/bin
  3. /nix/store/<hash>-<package>-<version>/bin

When we do devbox rm <package>, the first two (wrappers and profile bins) are removed, but the /nix/store remains in PATH.

The user would have to do eval $(devbox shellenv) to remove it. But that is not a good UX.

background context

Heres’ why we need/don’t need each one:

  1. bin-wrapper dir: is our path for binaries. Binaries here are wrapped to ensure the correct environment is sourced. “Support files” are also sym linked here to ensure that binaries that require relative dependencies work as well (e.g. mariadb)
  2. profile bin path: This is a fallback hack for certain edge cases (e.g. curl). If a package uses propagateBuildInputs then we may not correctly create a bin wrapper for it. Currently we use $buildInputs to determine binaries that need to be wrapped, but this does not contain propagated inputs, only explicit inputs.
  3. nix store paths: these are included by nix from nix print-dev-env. We've never removed them, but need to ensure nothing breaks.

Thanks to @mikeland86 for explaining this. I've copy-pasted this from a chat conversation.

this PR
This PR removes from PATH the subset of /nix/store paths that belong to the buildInputs, during shellenv.

Fixes #1060

TODO:

  • do more extensive testing

How was it tested?

> devbox add stress
> devbox shell
(devbox)> which stress
# printed wrappers/bin path
(devbox)> devbox rm stress
(devbox)> which stress
# empty
# BEFORE: this would print the /nix/store path
  • ensure testscripts pass.

Sorry, something went wrong.

Copy link
Collaborator Author

savil commented Jun 2, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@savil savil requested a review from mikeland73 June 2, 2023 23:33
internal/impl/devbox.go Show resolved Hide resolved
@savil savil force-pushed the savil/drop-nix-store-path branch from 65e3518 to c1385e1 Compare June 6, 2023 23:59
// packages around if they are used somewhere else or if the user hasn't triggered
// garbage collection.
nixEnvPath = filterPathList(nixEnvPath, func(path string) bool {
return !strings.HasPrefix(path, "/nix/store")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A problem with filtering all the /nix/store paths is that we also remove many utilities that are automatically included by pkgs.mkShell in the generated flake.nix.

As a result, basic functionality breaks, like invoking mkdir.

@mikeland86 thoughts on fixing this?

A (bad) idea is to generate print-dev-env PATH without any packages in the flake.nix. Keep the /nix/store/ paths from this, while removing any new /nix/store paths added from print-dev-env using the generated flake.nix. BUT this would involve calling print-dev-env twice, which is slow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you filter out $BuildInputs only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, this works I think. PTAL!

@savil savil changed the title [shellenv] filter out /nix/store paths from PATH [shellenv] filter out buildInput paths from PATH Jun 9, 2023
@savil savil force-pushed the savil/drop-nix-store-path branch from 96c855d to 3dd9cbf Compare June 9, 2023 18:16
@savil savil requested a review from mikeland73 June 9, 2023 18:21
@savil savil force-pushed the savil/drop-nix-store-path branch 2 times, most recently from a3312f6 to 319b0ab Compare June 9, 2023 21:35
@savil savil force-pushed the savil/drop-nix-store-path branch from 319b0ab to 03443a6 Compare June 9, 2023 21:36
@savil
Copy link
Collaborator Author

savil commented Jun 9, 2023

@mikeland86 going to land, since the change is as we'd discussed.

@savil savil merged commit 194a32d into main Jun 9, 2023
@savil savil deleted the savil/drop-nix-store-path branch June 9, 2023 22:17
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.

[Bug]: devbox global rm doesn't remove packages
2 participants