Download and build podman-compose using docker/podman locally #1043
Conversation
iruzo
force-pushed
the
main
branch
3 times, most recently
from
9589e72
to
5142b5c
Compare
|
Thanks for the PR. It will take some time to review it. Ideally I would like to have this as part of release process so that the executables are added to release artifacts. |
|
Hey @p12tic, Thank you for being so fast to reply to my PR. Please, take your time to review the PR, there is no need to rush, thanks for just taking a look at it 😃 |
README.md
Outdated
| ### Generate binary using docker/podman locally | ||
| This script will download the repo, generate the binary using [this Dockerfile](https://github.com/containers/podman-compose/blob/main/Dockerfile.md) and let the binary in the directory where you called this script. | ||
| ```bash | ||
| sh -c "$(curl -sSL https://raw.githubusercontent.com/containers/podman-compose/main/scripts/download_podman-compose.sh)" |
There was a problem hiding this comment.
I think this should better named as build_podman-compose.sh, because we're not just downloading. Also add a mention that the script will call sudo in case of docker-based build because this is rather sensitive to some people.
There was a problem hiding this comment.
Hmmm, yeah, you are probably right, but since there is already a script called generate_binary_using_dockerfile.sh, having one with "build" could lead to confusion.
What do you think about naming it "build_and_download_podman-compose.sh" ?
There was a problem hiding this comment.
Makes sense. But maybe download_and_build_podman-compose.sh ? So that reading left to right the sequence of events is correct.
There was a problem hiding this comment.
I agree with you. I just pushed the changes 😄
| CONTAINER_TOOL=$(find_container_tool) | ||
|
|
||
| # Find the directory containing the dockerfile by traversing up the directory tree | ||
| find_dockerfile_dir() { |
There was a problem hiding this comment.
I think it would be simpler to get the path to the script using $(dirname "$0") and then take ../Dockerfile. Traversing whole directory tree is not needed.
There was a problem hiding this comment.
The reason for this is to be able to call the script from wherever you want without being worried to break the podman build command.
Thanks to this I can call the script from the download one while being outside the repository dir.
Also, if for some reason you want to move the scripts to other dir, you can be sure that they will not break.
There was a problem hiding this comment.
My suggestion would still achieve this, unless I'm missing something.
$(dirname "$0") would return the path to the directory this script is in. Because the repository structure is /Dockerfile and /scripts/generate_binary_using_dockerfile.sh, then $(dirname "$0")/../Dockerfile should always point to the Dockerfile regardless of where this script is called from.
Am I missing something why this wouldn't work?
I agree that moving the scripts somewhere could be a problem, but I don't think we will move the scripts in the foreseeable future.
There was a problem hiding this comment.
If you use $(dirname "$0")/../Dockerfile, this will work as long as your keep the script in a subdir at the same level as it is right now.
e.g. If you change the script to /root/scripts/docker-scripts/generate_binary_using_dockerfile.sh, the script will break.
But yeah, you are right that it should still work if you call it from any other directory. So, if you are fine with keeping the script always under /scripts, I can make the change knowing that it will never break.
Since I didn't know if you will ever move the script to any other dir, I decided to do the find on the directory tree. Now that you are suggesting this, I imagine the directory structure will always be the same, so I'm going to make the change and ping you once done 😄 .
| SELINUX=$(check_selinux) | ||
|
|
||
| # Build binary | ||
| $CONTAINER_TOOL image rm podman-compose |
There was a problem hiding this comment.
Use something like build-podman-compose so that it has less chances to conflict with container names already on the system.
There was a problem hiding this comment.
You are right!, I just pushed the changes.
|
I actually forgot about this PR because I somehow thought that I'm waiting for something from you. Thanks for the ping. I have reviewed the PR now, looks good except several comments that should be simple to address. |
|
@p12tic |
|
Thanks a lot for fixes, they look great. |
|
I rebased to squash fixup commits into the commit they are fixing. |
Reason for this PR
As a user of Fedora Silverblue I found that the easiest ways to install podman-compose are:
I just wanted to execute a single command and obtain a binary I can save on my home directory without modifying anything outside my $HOME.
Changes
Usage
sh -c "$(curl -sSL https://raw.githubusercontent.com/containers/podman-compose/main/scripts/download_podman-compose.sh)"Thanks in advance. Any suggestions are welcome.