-
Notifications
You must be signed in to change notification settings - Fork 357
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
Issue 530: Fix container engine detection when podman-docker installed #538
Conversation
If both |
Why? If anything, I'd assume the opposite, since I assume there are very few users who would have both
If
From within a container, there are several (potentially unreliable) ways containers/podman#3586 . |
What is needed to get this merged? Currently this issue is preventing me from using cross at all and I would like to help. |
I think this change make sense, I don't use podman but the reasoning to use it when both docker and podman is installed makes sense to me. Right now we're locked by CI (#609), and we're in the process of a migration, see rust-embedded/wg#590 |
The CI issues have been fixed so this could be rebased and if it makes sense to the other maintainers, merged. I'm personally of the opinion defaulting to podman over docker would be a good idea as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed the merge issues, and I'd personally approve of these changes since podman defaults to less permissions and doesn't require a daemon. I'll let another maintainer be the final judge, however, since although podman aims to be entirely compatible with docker, this is a change in the default cross behavior.
@reitermarkus you previosuly mentioned this shouldn't be done. has your position changed? |
I think it makes sense to be the default on Linux. I think on macOS and Windows |
I've updated it to do this by default. I've used the check to default to podman first if the OS is Linux, rather than use podman on anything other than Windows or macOS, since podman can't run natively on anything other than Linux anyway I believe (FreeBSD jails or Dragonfly zones aren't identical) even if it gets support on the BSDs or similar (also, I doubt Docker will ever support FreeBSD). |
Uh I tried quashing multiple commits into one, but I think I failed spectacularly, which was not what I intended based on the documentation. Also, I've confirmed this works on docker for Windows, podman for Windows, podman-docker on WSL, podman on WSL, and docker & podman on Linux. |
Currently, docker detection fails under a few cases: 1. If `podman_docker` is installed, so the filename is docker but the actual executable is podman. 2. If the executable has a suffix, such as `.exe` on Windows, because we check if the executable `ends_with(DOCKER)`. The only reliable way to fix both these issues, IE, if the actual engine is docker and not an alias, and if the executable does not contain a suffix, is to query the container engine. This might not be ideal for performance reasons, but is the only reliable way to fix these issues. Closes cross-rs#530. Closes cross-rs#538.
755: Fix docker detection. r=Emilgardis a=Alexhuszagh Currently, docker detection fails under a few cases: 1. If `podman_docker` is installed, so the filename is docker but the actual executable is podman. 2. If the executable has a suffix, such as `.exe` on Windows, because we check if the executable `ends_with(DOCKER)`. The only reliable way to fix both these issues, IE, if the actual engine is docker and not an alias, and if the executable does not contain a suffix, is to query the container engine. This might not be ideal for performance reasons, but is the only reliable way to fix these issues. Closes #530. Closes #538. Co-authored-by: Alex Huszagh <ahuszagh@gmail.com>
When using
podman
withpodman-docker
installed,cross
will fail to build with a permission error.If using
cross
from master, this can be worked around by specifyingCROSS_CONTAINER_ENGINE=podman
, but this does not work for the latest released version (v0.2.1).Even though this has a workaround, this should be fixed such that we do not need to specify
CROSS_CONTAINER_ENGINE
. Simply defaulting topodman
first fixes it.Resolves #530