[bin wrappers] Fixes: don't export PATH, and eval the symlink of the binary #1330
Conversation
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
savil
requested review from
mikeland73 and
gcurtis
and removed request for
mikeland73
internal/wrapnix/wrapper.go
Outdated
| if err != nil { | ||
| return "", errors.WithStack(err) | ||
| } | ||
| devboxBinaryPath, err := filepath.EvalSymlinks(execPath) |
There was a problem hiding this comment.
let me harden this, so if an error is returned, then we do NOT error here. And then we skip modifying the PATH in the bin-wrappers.
There was a problem hiding this comment.
I think if this errors, you can simply delete the symlink and not recreate it. The rest can be the same.
internal/wrapnix/wrapper.go
Outdated
| if err != nil { | ||
| return "", errors.WithStack(err) | ||
| } | ||
| devboxBinaryPath, err := filepath.EvalSymlinks(execPath) |
There was a problem hiding this comment.
This will fail if existing symlink is already a loop. I think in that case we should just remove the symlink and not create a new one.
There was a problem hiding this comment.
I didn't remove the symlink: i'm a little concerned that if it was previously created and was working fine, then by removing the symlink we may break some bin-wrappers (that are from a different project) that have the symlinkDir in their PATH
There was a problem hiding this comment.
I think we should always add the symlink dir to path even if the sym link was not created. The reason is that those binwrappers will work correctly when we are able to create the sym link.
Note that adding an empty dir to PATH does no harm.
Otherwise, we end up with a bunch of bin wrappers that need to be recreated later.
internal/wrapnix/wrapper.go
Outdated
| // This may return an error due to symlink loops. In that case, we | ||
| // return an empty string, and the bin-wrapper should handle it accordingly. | ||
| // nolint:nilerr | ||
| return "", nil |
internal/wrapnix/wrapper.go
Outdated
| @@ -89,22 +94,33 @@ 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) { | |||
There was a problem hiding this comment.
change definition of this function to return sym link dir.
There was a problem hiding this comment.
I'd prefer not doing that, since I'd expect a function named CreateDevboxSymlink to return the path to the symlink, rather than its directory. Changing the name to CreateDevboxSymlinkAndGetDir feels verbose. I think its okay for the handling logic to do filepath.Dir
There was a problem hiding this comment.
but yeah, let me know if you feel strongly, and I can make the change.
hm fair point |
There was a problem hiding this comment.
OK, promise last comments.
Change CreateDevboxSymlink to return error only. Rename it to CreateDevboxSymlinkIfPossible. it returns nil if evalSymlinkErr != nil
create package variable
var devboxSymlinkDir = xdg.CacheSubpath(filepath.Join("devbox", "bin", "current"))
use that variable in CreateDevboxSymlinkIfPossible and also in CreateWrappers
|
@mikeland73 cool, PTAL |
Suspect IssuesThis pull request has been deployed and Sentry has observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Summary
This PR tries to address some concerns with #1324.
We drop
exportfromPATHin the bin-wrappers. This would have modifiedPATH for all child programs, which we need not do.
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 beena symlink.
How was it tested?
did a basic sanity test of opening a devbox shell in a golang project