Skip to content
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

Simplify & introduce versioning #83

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
/containers/latest/qemu-*-static
/containers/register/Dockerfile
/containers/register/register.sh
/containers/x86_64_qemu-*/
/containers/qemu-*/
Copy link
Member

Choose a reason for hiding this comment

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

As I commented in big picture section, I prefer keeping x86_64 prefix, because we are trying support other host arch now. I like to see that this modification in .gitignore is reverted.

Copy link
Author

Choose a reason for hiding this comment

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

This dir is rm -rf-ed after each loop iteration: https://github.com/meeDamian/qemu-user-static/blob/simplify/build-docker.sh#L61, so frankly speaking a completely static name could be used for it. As a matter of fact, I think it might be even better, and simpler.

/releases
71 changes: 39 additions & 32 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,42 +3,49 @@ services: docker
language: bash
Copy link
Member

Choose a reason for hiding this comment

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

You changed your new modification with 2 spaces indent. But this .travis.yml YAMP file was managed with 4 spaces indent. Please change your modification in .travis.yml to 4 spaces.

addons:
apt:
config:
retries: true
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the lines? I added the lines referring ruby/ruby's .travis.yml.
https://github.com/ruby/ruby/blob/master/.travis.yml#L120

Copy link
Author

Choose a reason for hiding this comment

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

My IDE says this property is not allowed there. I wasn't able to find any documentation about it. Everything worked well without it. I'm happy to add it back if it actually does something, or is useful :).

Copy link
Member

Choose a reason for hiding this comment

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

Please back it.

When "My IDE says this property is not allowed there", removing it is not good choice.

As you may know Travis is open source and written in Ruby language. In Ruby project, the core maintainers communicate with a core member on Travis project in Ruby project. As a result, the feature that is not documented, only existed in the source code can be used for Ruby project as a possibility. Ruby project uses .travis.yml with very advanced way.

update: true
packages:
- jq
- rpm2cpio
- cpio
env:
global:
- VERSION=4.0.0-5
# See qemu-user-static's RPM spec file on Fedora to check the new version.
# https://src.fedoraproject.org/rpms/qemu/blob/master/f/qemu.spec
- DOCKER_SERVER=docker.io
# - DOCKER_SERVER=quay.io
# Git repository
- REPO=multiarch/qemu-user-static
# Container repository
- DOCKER_REPO=$DOCKER_SERVER/multiarch/qemu-user-static
# - DOCKER_REPO=$DOCKER_SERVER/your_username/qemu-user-static
Copy link
Member

Choose a reason for hiding this comment

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

Please keep VERSION (For adhoc pipline), DOCKER_SERVER, REPO, DOCKER_REPO.
In general, it's not only for this project, I think that a good strategy to send pull-request, is "1 pull-request, 1 feature".
You changed a lot including some features.

The reason why 1 pull-request, 1 feature strategy is better is you can proceed your work understanding project people's needs and intentions towards your final goal.
The output becomes "1 commit 1 feature". The merit of this is if the new commit has a issue, we can revert the commit later.

Though I understand the each feature in this PR is great.

I am thinking how we can proceed this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Everything was commented out, and defaults were used. I thought that in a case like this it's simpler to start with a cleaner slate, and then gradually add features, as they become needed at the time.

Also, not sure what "adhoc pipline" means, and how and what version to add 🤔

- PACKAGE_URI="https://kojipkgs.fedoraproject.org/packages/qemu/4.0.0/5.fc31/x86_64/qemu-user-static-4.0.0-5.fc31.x86_64.rpm"
- PACKAGE_FILENAME=$(basename "$PACKAGE_URI")
global:
# See qemu-user-static's RPM spec file on Fedora to check the new version.
# https://src.fedoraproject.org/rpms/qemu/blob/master/f/qemu.spec
# Container repository
- PACKAGE_URI="https://kojipkgs.fedoraproject.org/packages/qemu/4.0.0/5.fc31/x86_64/qemu-user-static-4.0.0-5.fc31.x86_64.rpm"
- PACKAGE_FILENAME=$(basename "$PACKAGE_URI")

stages:
- build
- test
- name: deploy
if: tag IS present


before_script:
- wget --content-disposition $PACKAGE_URI
- rpm2cpio $PACKAGE_FILENAME | cpio -dimv
script:
- wget --content-disposition ${PACKAGE_URI}
- rpm2cpio ${PACKAGE_FILENAME} | cpio -dimv
- ./generate_tarballs.sh
- |
if [[ $TRAVIS_BRANCH == 'master' && $TRAVIS_PULL_REQUEST == 'false' ]]; then
./publish.sh -v "$VERSION" -t "$GITHUB_TOKEN" -r "$REPO"
fi
- ./update.sh -v "$VERSION" -r "$REPO" -d "$DOCKER_REPO"
- docker images
- ./test.sh -d "$DOCKER_REPO"
after_success:
- |
if [[ $TRAVIS_BRANCH == 'master' && $TRAVIS_PULL_REQUEST == 'false' ]]; then
docker login -u="$DOCKER_USERNAME" -p="$DOCKER_PASSWORD" "$DOCKER_SERVER" && \
docker push "$DOCKER_REPO"
fi

jobs:
include:
- stage: build
name: "Build Docker image for amd64"
Copy link
Member

@junaruga junaruga Aug 16, 2019

Choose a reason for hiding this comment

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

Can you change "Build container images for x86_64"?
Because "x86_64" and "container" are already used in other parts and manual. "Docker" is product name.

script: ./build-docker.sh

- stage: test
name: "Test generated images"
script: ./test.sh

- name: "Upload qemu binaries to Github Releases"
stage: deploy
deploy:
skip_cleanup: true
provider: releases
api_key:
secure: Q2ueVCN6jsXOn9iaf8ucAMWsLH+KkYrleSKRbMkJPu+qVacVcYFynm6yMwM63koVA8ADN80+yl7JUvqWx+MZLfCwR+/MRyGC2J0AlCDTBuvK55ETT4Kggtwb/2j6XrjQLlV+9Qkhb0JVpyO1475CjnoT9aOpWsHEHBymddQC13IQMn27XV1LJhunLw7+7Pqb73Yrml8PvnRLFqfqaaLZMoEUjbCjayjmgJY1HqUA2dtQBUnlASDLo3H6epJaDD8qpZTU4850I6TFwavSpRXq786+uqLWfXki0H9EJFkPMgEluul09BRXNS4RABC2darrIluQfK0EOS+acW4AHkTiM5s5YiT402WrD5VVjRCV0GoLrUjnnAFj+jm5fCXykSVJ04/tOMrTMnIgusmUYDRLJ9JUhFb6LS5pzg1ib9pfBmkUfRfEqymFfmsV32sZUyI3+C3NNjOuBiePtQ0gx3vzy1jWwqf5y4IlvMjYyQE11J0PZBjrcZWSXaFlQ34Wl7jZgr+WXJ3a717LX+TbpVjTAuJdPv4vTDJ755avOAM3IgGLFONg7x/5+yroe5dtnHsS5g2Gy8ZYF+Kb0I14x+8Hgi/jRxdF3cKvOoO8MvnK6zeDB4EDolWyTXHwfmpkJOtW+LbvJTJffg5bhoeevpJWaebJqzkusY3/nevG8FYAv2Y=
file_glob: true
file: releases/*.tgz
on:
repo: ${TRAVIS_REPO_SLUG}
Copy link
Member

Choose a reason for hiding this comment

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

This line means the container image's URL that is used for docker push, right?
Not everyone's GitHub (owner_name/repo_name) including forked repository equals the image URL (owner_name/repo_name) on DockerHub.

The logic is to consider the cases to give users options.
In my opinion, DockerHub has a problem for the security than quay.io.

        - DOCKER_SERVER=docker.io
        # - DOCKER_SERVER=quay.io
        # Git repository
        - REPO=multiarch/qemu-user-static
        # Container repository
        - DOCKER_REPO=$DOCKER_SERVER/multiarch/qemu-user-static

My development container repository is here quay.io/junaruga/qemu-user-static.
https://quay.io/repository/junaruga/qemu-user-static?tab=info
The container server information is necessary.

When someone already have qemu-user-static repository, the forked repository from this repository is username/qemu-user-static-1.

Copy link
Author

Choose a reason for hiding this comment

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

That line (and entire deploy: section here) has nothing to do with Docker. This is a Travis-provided line that contains Github's SLUG ex. multiarch/qemu-user-static so that deploy knows where to upload the tarballs.

tags: true
92 changes: 92 additions & 0 deletions build-docker.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

I do not like the file name build-docker.sh.
The reason is

  1. shell scripts in this project is alphabet (small letter) + underscore as a separator.
  2. "docker" is product name. "containers" are already used as a output directory name for the purpose.

I prefer the file name build_containers.sh.

Copy link
Member

Choose a reason for hiding this comment

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

This project's shell scripts are 4 spaces indent mainly. See removed publish.sh and removed lines in test.sh.
So, keep 4 spaces indent on shell scripts (*.sh).

set -xe
Copy link
Member

Choose a reason for hiding this comment

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

If you use pipe | in the shell script, consider pipefail.

#!/usr/bin/env bash
set -xeo pipefail


#
## This script builds 3 types of Docker images (assuming version `VERSION`):
#
# 1. Image containing all qemu binaries. Built in `./containers/latest/`. Tagged with `:latest`, and `:VERSION`.
# 2. Register image. Built in `./containers/register/`. Tagged with `:register`, and `:register-VERSION`.
# 3. A series of images, each bundled with a single guest-architecture qemu binary. Built in `./containers/qemu-ARCH`.
# Tagged with `:ARCH`, and `:ARCH-VERSION`.


# Convert Travis-provided repo SLUG to lowercase - Docker's requirement for tags
SLUG="$(echo "${TRAVIS_REPO_SLUG}" | tr '[:upper:]' '[:lower:]')"
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use SLUG (Github's owner_name/repo_name) as docker repo. The reason is as I wrote at other part.
Please keep using DOCKER_REPO in the file instead.


#
Copy link
Member

Choose a reason for hiding this comment

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

You forget comment here?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if necessary, I'll just remove the line :)

BUILD_DIR="containers"

# list of all supported architectures for guest machines
guest_architectures="aarch64 aarch64_be alpha armeb arm cris hppa i386 m68k microblazeel microblaze mips64el mips64 mipsel mipsn32el mipsn32 mips nios2 or1k ppc64abi32 ppc64le ppc64 ppc riscv32 riscv64 s390x sh4eb sh4 sparc32plus sparc64 sparc tilegx trace-stap xtensaeb xtensa"

# NOTE: All qemu binaries have been downloaded, and unpacked to `./usr/bin/` in `before_script` already.

##
# Prepare & build image containing all qemu binaries (`:latest`)
##
cp -p ./usr/bin/qemu-*-static "${BUILD_DIR}/latest/"
docker build -t "${SLUG}:latest" "${BUILD_DIR}/latest"


##
# Prepare & build register image (`:register`)
##
cp -p ${BUILD_DIR}/latest/{Dockerfile,register.sh} "${BUILD_DIR}/register/"

# Register image does not need `qemu-*-static` binaries copied into it. This line removes the COPY directive from Dockerfile.
sed -i '/^COPY qemu/ s/^/#/' "${BUILD_DIR}/register/Dockerfile"
docker build -t "${SLUG}:register" "${BUILD_DIR}/register"


##
# Build images for individual target architectures (`:ARCH`)
##
for guest_arch in ${guest_architectures}; do
work_dir="${BUILD_DIR}/qemu-${guest_arch}"

mkdir -p "${work_dir}"

# copy a single binary to the image
cp -p "./usr/bin/qemu-${guest_arch}-static" "${work_dir}"

# create a minimal `Dockerfile` that only copies that specific binary into the image
cat > "${work_dir}/Dockerfile" -<<EOF
FROM scratch
COPY qemu-${guest_arch}-static /usr/bin/
EOF

docker build -t "${SLUG}:${guest_arch}" "${work_dir}"

# cleanup
rm -rf "${work_dir}"
done


# If git tag is provided, tag all images with VERSION, and push them to Docker Hub
if [[ -n "${TRAVIS_TAG}" ]]; then
if [[ -z "${DOCKER_USER}" ]] || [[ -z "${DOCKER_PASS}" ]]; then
echo "For deployment to Docker Hub to work both DOCKER_USER and DOCKER_PASS must be provided in Travis build settings."
exit 1
fi

# Login to Docker Hub
echo "${DOCKER_PASS}" | docker login -u="${DOCKER_USER}" --password-stdin

# Tag `:latest` with a specific qemu version, and push both
docker tag "${SLUG}:latest" "${SLUG}:${TRAVIS_TAG}"
docker push "${SLUG}:latest"
docker push "${SLUG}:${TRAVIS_TAG}"

# Tag `:register` with specific qemu version, and push both
docker tag "${SLUG}:register" "${SLUG}:register-${TRAVIS_TAG}"
docker push "${SLUG}:register"
docker push "${SLUG}:register-${TRAVIS_TAG}"

# For each architecture, create a versioned tag, and push all
for guest_arch in ${guest_architectures}; do
docker tag "${SLUG}:${guest_arch}" "${SLUG}:${guest_arch}-${TRAVIS_TAG}"

docker push "${SLUG}:${guest_arch}"
docker push "${SLUG}:${guest_arch}-${TRAVIS_TAG}"
done
fi
File renamed without changes.
20 changes: 13 additions & 7 deletions generate_tarballs.sh
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
#!/bin/bash -e
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

You need "-e" here, keep compatibility.

Copy link
Author

Choose a reason for hiding this comment

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

Adding -e to #!/usr/bin/env bash breaks everything. AFAIK using #!/usr/bin/env bash is recommended. I have no idea what -e does, I'm happy to go with anything you recommend :).

Copy link
Member

Choose a reason for hiding this comment

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

I do not object with #!/usr/bin/env bash.
It is certainly a good practice

  • In a case of people want to run a bash script on the environment where /bin/bash does not exist. But I wonder if there is the Linux or UNIX distribution that do not have it today. Because /bin/ is standard rule of Linux system. Ref: https://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard .
  • I a case of people want to run a shell script on the different path's bash that is source compiled bash.

The down side of #!/usr/bin/env bash is different path's bash is unintentionally executed.

I am not a original coder who wrote #!/bin/bash. But you should feel original coder's good intent for the code. At least you should not include the change of #!/usr/bin/env bash in this PR. Because it's not harmful.
For each change especially for non backward compatibility feature, we need discussion.

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea what -e does, I'm happy to go with anything you recommend :).

You do not know bash -e? It's quite important feature to make the bash script defensive. It's similar with set -e. man bash might explain you.

When you have no idea what something does, proceeding removing it is not good choice.


#
## This script compresses all qemu binaries, and prepares them for upload to Github Release later in the deploy step.

if [[ -z "${TRAVIS_TAG}" ]]; then
echo "TRAVIS_TAG not specified. Skipping tarball generation."
exit 0
fi

rm -rf releases
mkdir -p releases
# find . -regex './qemu-.*' -not -regex './qemu-system-.*' -exec cp {} releases \;
cp ./usr/bin/qemu-*-static releases/
cd releases/
for file in *; do
tar -czf $file.tar.gz $file;
cp $file.tar.gz x86_64_$file.tar.gz

for file in ./usr/bin/qemu-*-static; do
name="$(basename "${file}").tgz"
tar -cvzf "releases/${TRAVIS_TAG}-${name}" "${file}"
done
52 changes: 0 additions & 52 deletions publish.sh

This file was deleted.

28 changes: 7 additions & 21 deletions test.sh
Original file line number Diff line number Diff line change
@@ -1,29 +1,16 @@
#!/bin/bash
#!/usr/bin/env bash
set -xeo pipefail

# A POSIX variable
OPTIND=1 # Reset in case getopts has been used previously in the shell.

while getopts "d:" opt; do
case "$opt" in
d) DOCKER_REPO=$OPTARG
;;
esac
done

if [ "${DOCKER_REPO}" = "" ]; then
echo "DOCKER_REPO is required." 1>&2
exit 1
fi
# Convert Travis-provided repo SLUG to lowercase - Docker's requirement for tags
SLUG="$(echo "${TRAVIS_REPO_SLUG}" | tr '[:upper:]' '[:lower:]')"
Copy link
Member

Choose a reason for hiding this comment

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

TRAVIS_REPO_SLUG does not always container repository's URL. Please keep DOCKER_REPO in this file.


# Test cases

# ------------------------------------------------
# multiarch/qemu-user-static image

# It should register binfmt_misc entry with 'flags: F'
# by given "-p yes" option.
sudo docker run --rm --privileged ${DOCKER_REPO} --reset -p yes
sudo docker run --rm --privileged "${SLUG}" --reset -p yes
cat /proc/sys/fs/binfmt_misc/qemu-aarch64
grep -q '^flags: F$' /proc/sys/fs/binfmt_misc/qemu-aarch64

Expand Down Expand Up @@ -53,7 +40,7 @@ docker run --rm -t arm64v8/fedora uname -m

# It should register binfmt_misc entry with 'flags: '
# by given no "-p yes" option.
sudo docker run --rm --privileged ${DOCKER_REPO}:register --reset
sudo docker run --rm --privileged "${SLUG}:register" --reset
cat /proc/sys/fs/binfmt_misc/qemu-aarch64
grep -q '^flags: $' /proc/sys/fs/binfmt_misc/qemu-aarch64

Expand All @@ -62,13 +49,12 @@ grep -q '^flags: $' /proc/sys/fs/binfmt_misc/qemu-aarch64
# multiarch/qemu-user-static:$from_arch-$to_arch image

# /usr/bin/qemu-aarch64-static should be included.
docker run --rm -t ${DOCKER_REPO}:aarch64 /usr/bin/qemu-aarch64-static --version
docker run --rm -t ${DOCKER_REPO}:x86_64-aarch64 /usr/bin/qemu-aarch64-static --version
docker run --rm -t "${SLUG}:aarch64" /usr/bin/qemu-aarch64-static --version

# ------------------------------------------------
# Integration test
docker build --rm -t "test/integration/ubuntu" -<<EOF
FROM ${DOCKER_REPO}:x86_64-aarch64 as qemu
FROM ${SLUG}:aarch64 as qemu
FROM arm64v8/ubuntu
COPY --from=qemu /usr/bin/qemu-aarch64-static /usr/bin
EOF
Expand Down
68 changes: 0 additions & 68 deletions update.sh

This file was deleted.