Skip to content

Commit

Permalink
[bin wrappers] Fixes: don't export PATH, and eval the symlink of the …
Browse files Browse the repository at this point in the history
…binary (#1330)

## Summary

This PR tries to address some concerns with #1324.

1. We drop `export` from `PATH` in the bin-wrappers. This would have
modified
PATH for all child programs, which we need not do. 

2. When creating the symlink, we ensure the target value is passed
through `EvalSymlink`.
Previously, it was set as the result of `os.Executable`, which may still
have been
a symlink.

## How was it tested?

did a basic sanity test of opening a devbox shell in a golang project
  • Loading branch information
savil authored Jul 28, 2023
1 parent 654e973 commit 31d2c00
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 16 deletions.
2 changes: 1 addition & 1 deletion internal/boxcli/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func versionCmdFunc(cmd *cobra.Command, _ []string, flags versionFlags) error {
// Not doing for now, since users who have Devbox binary prior to this edit
// (before Devbox v0.5.9) will not invoke this flag in `devbox version update`.
// But we still want this to run for them.
if _, err := wrapnix.CreateDevboxSymlink(); err != nil {
if err := wrapnix.CreateDevboxSymlinkIfPossible(); err != nil {
return err
}
} else {
Expand Down
47 changes: 33 additions & 14 deletions internal/wrapnix/wrapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

"github.com/pkg/errors"
"go.jetpack.io/devbox/internal/cmdutil"
"go.jetpack.io/devbox/internal/debug"
"go.jetpack.io/devbox/internal/fileutil"
"go.jetpack.io/devbox/internal/nix"
"go.jetpack.io/devbox/internal/plugin"
Expand All @@ -31,6 +32,10 @@ type devboxer interface {
var wrapper string
var wrapperTemplate = template.Must(template.New("wrapper").Parse(wrapper))

// devboxSymlinkDir is the directory that has the symlink to the devbox binary,
// which is used by the bin-wrappers
var devboxSymlinkDir = xdg.CacheSubpath(filepath.Join("devbox", "bin", "current"))

// CreateWrappers creates wrappers for all the executables in nix paths
func CreateWrappers(ctx context.Context, devbox devboxer) error {
shellEnvHash, err := devbox.ShellEnvHash(ctx)
Expand All @@ -51,8 +56,7 @@ func CreateWrappers(ctx context.Context, devbox devboxer) error {
if err != nil {
return err
}
devboxSymlinkPath, err := CreateDevboxSymlink()
if err != nil {
if err := CreateDevboxSymlinkIfPossible(); err != nil {
return err
}

Expand All @@ -62,7 +66,7 @@ func CreateWrappers(ctx context.Context, devbox devboxer) error {
BashPath: bashPath,
Command: bin,
ShellEnvHash: shellEnvHash,
DevboxSymlinkDir: filepath.Dir(devboxSymlinkPath),
DevboxSymlinkDir: devboxSymlinkDir,
destPath: filepath.Join(destPath, filepath.Base(bin)),
}); err != nil {
return errors.WithStack(err)
Expand All @@ -72,7 +76,7 @@ func CreateWrappers(ctx context.Context, devbox devboxer) error {
return createSymlinksForSupportDirs(devbox.ProjectDir())
}

// CreateDevboxSymlink creates a symlink to the devbox binary
// CreateDevboxSymlinkIfPossible creates a symlink to the devbox binary.
//
// Needed because:
//
Expand All @@ -88,29 +92,44 @@ func CreateWrappers(ctx context.Context, devbox devboxer) error {
//
// So, the bin-wrappers need to use a symlink to the latest devbox binary. This
// symlink is updated when devbox is updated.
func CreateDevboxSymlink() (string, error) {
curDir := xdg.CacheSubpath(filepath.Join("devbox", "bin", "current"))
if err := fileutil.EnsureDirExists(curDir, 0755, false /*chmod*/); err != nil {
return "", err
func CreateDevboxSymlinkIfPossible() error {
// Get the symlink path; create the symlink directory if it doesn't exist.
if err := fileutil.EnsureDirExists(devboxSymlinkDir, 0755, false /*chmod*/); err != nil {
return err
}
currentDevboxSymlinkPath := filepath.Join(curDir, "devbox")
currentDevboxSymlinkPath := filepath.Join(devboxSymlinkDir, "devbox")

devboxBinaryPath, err := os.Executable()
// Get the path to the devbox binary.
execPath, err := os.Executable()
if err != nil {
return "", errors.WithStack(err)
return errors.WithStack(err)
}
devboxBinaryPath, evalSymlinkErr := filepath.EvalSymlinks(execPath)
// we check the error below, because we always want to remove the symlink

// We will always re-create this symlink to ensure correctness.
if err := os.Remove(currentDevboxSymlinkPath); err != nil && !errors.Is(err, os.ErrNotExist) {
return "", errors.WithStack(err)
return errors.WithStack(err)
}

if evalSymlinkErr != nil {
// This may return an error due to symlink loops. But we don't stop the
// process for this reason, so the bin-wrappers can still be created.
//
// Once the symlink loop is fixed, and the bin-wrappers
// will start working without needing to be re-created.
//
// nolint:nilerr
debug.Log("Error evaluating symlink: %v", evalSymlinkErr)
return nil
}

// Don't return error if error is os.ErrExist to protect against race conditions.
if err := os.Symlink(devboxBinaryPath, currentDevboxSymlinkPath); err != nil && !errors.Is(err, os.ErrExist) {
return "", errors.WithStack(err)
return errors.WithStack(err)
}

return currentDevboxSymlinkPath, nil
return nil
}

type createWrapperArgs struct {
Expand Down
2 changes: 1 addition & 1 deletion internal/wrapnix/wrapper.sh.tmpl
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!{{ .BashPath }}

export PATH="{{ .DevboxSymlinkDir }}:$PATH"
PATH="{{ .DevboxSymlinkDir }}:$PATH"

{{/*
# If env variable has never been set by devbox we set it, but also
Expand Down

0 comments on commit 31d2c00

Please sign in to comment.