[runx] Use single path for all runx binaries #1544
Conversation
internal/impl/devbox.go
Outdated
| if err := os.RemoveAll(runxBinPath); err != nil { | ||
| return "", err | ||
| } | ||
| if err := fileutil.EnsureDirExists(runxBinPath, 0o755, false); err != nil { |
| paths = append(paths, p...) | ||
| for _, path := range paths { | ||
| // create symlink to all files in p | ||
| files, err := os.ReadDir(path) |
There was a problem hiding this comment.
Won't this symlink all files in the archive, including non-binaries? I'm wondering if this will create a bunch of conflicts. For example, if multiple packages have a README in them.
There was a problem hiding this comment.
Yeah it will, but mostly for files we don't care about. We can fix this later on. (Requires priorities similar to nix)
internal/impl/devbox.go
Outdated
| for _, file := range files { | ||
| src := filepath.Join(path, file.Name()) | ||
| dst := filepath.Join(runxBinPath, file.Name()) | ||
| if err := os.Symlink(src, dst); err != nil && errors.Is(err, os.ErrExist) { |
There was a problem hiding this comment.
Good catch, this should be !errors.Is(err, os.ErrExist)
Specifically I want to ignore already exist errors for the case of conflicts and race conditions.
internal/impl/devbox.go
Outdated
| if err := fileutil.EnsureDirExists(runxBinPath, 0o755, false); err != nil { | ||
| return "", err | ||
| } | ||
| packages := lo.Filter(d.InstallablePackages(), devpkg.IsRunX) |
There was a problem hiding this comment.
Doing devpkg.IsRunX in the loop would avoid cloning the slice.
There was a problem hiding this comment.
Will change. Though slice is just a pointer slice on stack, I would favor readability over avoiding the opperation.
The conflicts for sure are not a great experience, but I actually think this is almost ready for release and would not mind releasing with that issue. The main issue I'm not sure about is the |
Summary
This allows adding and removing runx binaries inside a devbox environment without having to shellenv again.
How was it tested?