Skip to content

[Bug fix] Ensure bin-wrappers use latest devbox binary to prevent false update notifications #1324

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

Merged
merged 3 commits into from
Jul 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions internal/boxcli/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@ import (
"runtime"

"github.com/spf13/cobra"
"go.jetpack.io/devbox/internal/wrapnix"

"go.jetpack.io/devbox/internal/build"
"go.jetpack.io/devbox/internal/envir"
"go.jetpack.io/devbox/internal/vercheck"
)

type versionFlags struct {
verbose bool
verbose bool
updateDevboxSymlink bool
}

func versionCmd() *cobra.Command {
Expand All @@ -33,6 +35,14 @@ func versionCmd() *cobra.Command {
command.Flags().BoolVarP(&flags.verbose, "verbose", "v", false, // value
"displays additional version information",
)
// Make this flag hidden because:
// This functionality doesn't strictly belong in this command, but we add it here
// since `devbox version update` calls `devbox version -v` to trigger an update.
command.Flags().BoolVarP(&flags.updateDevboxSymlink, "update-devbox-symlink", "u", false, // value
"update the devbox symlink to point to the current binary",
)
_ = command.Flags().MarkHidden("update-devbox-symlink")

command.AddCommand(selfUpdateCmd())
return command
}
Expand All @@ -52,16 +62,24 @@ func selfUpdateCmd() *cobra.Command {

func versionCmdFunc(cmd *cobra.Command, _ []string, flags versionFlags) error {
w := cmd.OutOrStdout()
v := getVersionInfo()
info := getVersionInfo()
if flags.verbose {
fmt.Fprintf(w, "Version: %v\n", v.Version)
fmt.Fprintf(w, "Platform: %v\n", v.Platform)
fmt.Fprintf(w, "Commit: %v\n", v.Commit)
fmt.Fprintf(w, "Commit Time: %v\n", v.CommitDate)
fmt.Fprintf(w, "Go Version: %v\n", v.GoVersion)
fmt.Fprintf(w, "Launcher: %v\n", v.LauncherVersion)
fmt.Fprintf(w, "Version: %v\n", info.Version)
fmt.Fprintf(w, "Platform: %v\n", info.Platform)
fmt.Fprintf(w, "Commit: %v\n", info.Commit)
fmt.Fprintf(w, "Commit Time: %v\n", info.CommitDate)
fmt.Fprintf(w, "Go Version: %v\n", info.GoVersion)
fmt.Fprintf(w, "Launcher: %v\n", info.LauncherVersion)

// TODO: in a subsequent PR, we should do this when flags.updateDevboxSymlink is true.
// 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 {
return err
}
} else {
fmt.Fprintf(w, "%v\n", v.Version)
fmt.Fprintf(w, "%v\n", info.Version)
}
return nil
}
Expand Down
12 changes: 2 additions & 10 deletions internal/cloud/openssh/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"text/template"

"github.com/pkg/errors"
"go.jetpack.io/devbox/internal/fileutil"
)

// These must match what's in sshConfigTmpl. We should eventually make the hosts
Expand Down Expand Up @@ -211,17 +212,8 @@ func containsDevboxInclude(r io.Reader) bool {
return false
}

// move to a file utility
func EnsureDirExists(path string, perm fs.FileMode, chmod bool) error {
if err := os.Mkdir(path, perm); err != nil && !errors.Is(err, fs.ErrExist) {
return errors.WithStack(err)
}
if chmod {
if err := os.Chmod(path, perm); err != nil {
return errors.WithStack(err)
}
}
return nil
return fileutil.EnsureDirExists(path, perm, chmod)
}

// returns path to ~/.config/devbox/ssh
Expand Down
15 changes: 15 additions & 0 deletions internal/fileutil/fileutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@
package fileutil

import (
"io/fs"
"os"
"strings"

"github.com/pkg/errors"
)

// TODO: publish as it's own shared package that other binaries can use.
Expand Down Expand Up @@ -55,3 +58,15 @@ func FileContains(path string, substring string) (bool, error) {
}
return strings.Contains(string(data), substring), nil
}

func EnsureDirExists(path string, perm fs.FileMode, chmod bool) error {
if err := os.MkdirAll(path, perm); err != nil && !errors.Is(err, fs.ErrExist) {
return errors.WithStack(err)
}
if chmod {
if err := os.Chmod(path, perm); err != nil {
return errors.WithStack(err)
}
}
return nil
}
4 changes: 4 additions & 0 deletions internal/impl/packages.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,10 @@ func (d *Devbox) ensurePackagesAreInstalled(ctx context.Context, mode installMod
// Ensure we clean out packages that are no longer needed.
d.lockfile.Tidy()

if err := wrapnix.CreateWrappers(ctx, d); err != nil {
return err
}

return d.lockfile.Save()
}

Expand Down
2 changes: 1 addition & 1 deletion internal/vercheck/vercheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ func triggerUpdate(stdErr io.Writer) (*updatedVersions, error) {
}

// TODO savil. Add a --json flag to devbox version and parse the output as JSON
cmd := exec.Command(exePath, "version", "-v")
cmd := exec.Command(exePath, "version", "-v", "--update-devbox-symlink")

buf := new(bytes.Buffer)
cmd.Stdout = io.MultiWriter(stdErr, buf)
Expand Down
51 changes: 46 additions & 5 deletions internal/wrapnix/wrapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ import (

"github.com/pkg/errors"
"go.jetpack.io/devbox/internal/cmdutil"
"go.jetpack.io/devbox/internal/fileutil"
"go.jetpack.io/devbox/internal/nix"
"go.jetpack.io/devbox/internal/plugin"
"go.jetpack.io/devbox/internal/xdg"
)

type devboxer interface {
Expand Down Expand Up @@ -49,9 +51,7 @@ func CreateWrappers(ctx context.Context, devbox devboxer) error {
if err != nil {
return err
}
// get absolute path of devbox binary that the launcher script invokes
// to avoid causing an infinite loop when coreutils gets installed
executablePath, err := os.Executable()
devboxSymlinkPath, err := CreateDevboxSymlink()
if err != nil {
return err
}
Expand All @@ -62,7 +62,7 @@ func CreateWrappers(ctx context.Context, devbox devboxer) error {
BashPath: bashPath,
Command: bin,
ShellEnvHash: shellEnvHash,
DevboxBinaryPath: executablePath,
DevboxSymlinkDir: filepath.Dir(devboxSymlinkPath),
destPath: filepath.Join(destPath, filepath.Base(bin)),
}); err != nil {
return errors.WithStack(err)
Expand All @@ -72,12 +72,53 @@ func CreateWrappers(ctx context.Context, devbox devboxer) error {
return createSymlinksForSupportDirs(devbox.ProjectDir())
}

// CreateDevboxSymlink creates a symlink to the devbox binary
//
// Needed because:
//
// 1. The bin-wrappers cannot invoke devbox via the Launcher. The Launcher script
// invokes some coreutils commands that may themselves be installed by devbox
// and so be bin-wrappers. This causes an infinite loop.
//
// So, the bin-wrappers need to directly invoke the devbox binary.
//
// 2. The devbox binary's path will change when devbox is updated. Hence
// using absolute paths to the devbox binaries in the bin-wrappers
// will result in bin-wrappers invoking older devbox binaries.
//
// 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
}
currentDevboxSymlinkPath := filepath.Join(curDir, "devbox")

devboxBinaryPath, err := os.Executable()
if err != nil {
return "", errors.WithStack(err)
}

// 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)
}

// 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 currentDevboxSymlinkPath, nil
}

type createWrapperArgs struct {
devboxer
BashPath string
Command string
ShellEnvHash string
DevboxBinaryPath string
DevboxSymlinkDir string
destPath string
}

Expand Down
6 changes: 4 additions & 2 deletions internal/wrapnix/wrapper.sh.tmpl
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#!{{ .BashPath }}

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

{{/*
# If env variable has never been set by devbox we set it, but also
# default to env value set by user. This means plugin env variables behave a bit
Expand All @@ -19,7 +21,7 @@ DO_NOT_TRACK=1 can be removed once we optimize segment to queue events.

if [[ "${{ .ShellEnvHashKey }}" != "{{ .ShellEnvHash }}" ]] && [[ -z "${{ .ShellEnvHashKey }}_GUARD" ]]; then
export {{ .ShellEnvHashKey }}_GUARD=true
eval "$(DO_NOT_TRACK=1 {{ .DevboxBinaryPath }} shellenv -c {{ .ProjectDir }})"
eval "$(DO_NOT_TRACK=1 devbox shellenv -c {{ .ProjectDir }})"
fi

{{/*
Expand All @@ -29,6 +31,6 @@ should be in PATH.

DO_NOT_TRACK=1 can be removed once we optimize segment to queue events.
*/ -}}
eval "$(DO_NOT_TRACK=1 {{ .DevboxBinaryPath }} shellenv only-path-without-wrappers)"
eval "$(DO_NOT_TRACK=1 devbox shellenv only-path-without-wrappers)"

exec {{ .Command }} "$@"