Skip to content

Added checking for symlinked cmds #3818

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

plandem
Copy link
Contributor

@plandem plandem commented Aug 11, 2025

No description provided.

@afbjorklund
Copy link
Member

Why resolve the symlinks?

@plandem
Copy link
Contributor Author

plandem commented Aug 11, 2025

Why resolve the symlinks?

e.g. i dont want to install docker cli locally, but want to symlink docker.lima as docker. e.g. some tools (e.g. Tilt.dev) will call "docker_build" or "kubectl_build" or even "nerdctl_build", then it will work. with this checking I can symlink it and use as regular docker cli, e.g. LIMA_INSTANCE=lima_project_a docker, which will call that wrapper with proper limactl shell docker. without that "self guard", wrapper will go in infinite loop, because "command -v" will return link to wrapper.

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

I think the change is fine, but should have a comment to explain why the check is needed because stripping the .lima suffix from the script names is not the regular use case.

To prevent infinite looping, we need to self-guard lima cmds, when .lima suffixes were removed or symlinked without it (e.g. `ln -s docker.lima docker`)

Signed-off-by: Andrei Gaivoronskii <[email protected]>
@plandem
Copy link
Contributor Author

plandem commented Aug 12, 2025

I think the change is fine, but should have a comment to explain why the check is needed because stripping the .lima suffix from the script names is not the regular use case.

Added comments to commit message. Is it enough or better to comment in scripts?

@alexandear
Copy link
Member

Added comments to commit message. Is it enough or better to comment in scripts?

I vote for adding a comment explaining why we need this in three places:

  • PR description.
  • Commit message (make sure the first line of the commit does not end with a period .).
  • In the scripts themselves.

Make sure the title of the PR heading equals to the first line of the commit.

@afbjorklund
Copy link
Member

afbjorklund commented Aug 12, 2025

I missed that you were using symlinks, instead of shell aliases.

Maybe this also needs to be mentioned in the documentation?

https://lima-vm.io/docs/examples/#running-containers

you may make an alias (or a symlink) by yourself: alias nerdctl=nerdctl.lima

There are tabs for nerdctl / docker /podman / kubernetes, but no wrappers or symlinks.

cp -a cmd/lima _output/bin/lima
cp -a cmd/nerdctl.lima _output/bin/nerdctl.lima
cp -a cmd/apptainer.lima _output/bin/apptainer.lima
cp -a cmd/docker.lima _output/bin/docker.lima
cp -a cmd/podman.lima _output/bin/podman.lima
cp -a cmd/kubectl.lima _output/bin/kubectl.lima

Maybe I should have added something about apptainer (singularity / SIF), as well?

@plandem
Copy link
Contributor Author

plandem commented Aug 12, 2025

I missed that you were using symlinks, instead of shell aliases.

I needed it for Tilt.dev which spawns shells (non interactive, so can be some struggle with aliases). But I needed nerdctl and kubectl and you cant install nerdctl on OSX and use socket for containerd in lima, at least I had no success. if there would be an option to opt-out any local cli tools and use wrappers only, I personally would go this way.

Maybe this also needs to be mentioned in the documentation?

https://lima-vm.io/docs/examples/#running-containers

you may make an alias (or a symlink) by yourself: alias nerdctl=nerdctl.lima

like this? or something more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants