uraimo / run-on-arch-action

A Github Action that executes jobs/commands on non-x86 cpu architectures (ARMv6, ARMv7, aarch64, s390x, ppc64le, riscv64) via QEMU
BSD 3-Clause "New" or "Revised" License
665 stars 146 forks source link

feat(base_image): add custom base_image argument #103

Closed LonghronShen closed 1 year ago

LonghronShen commented 1 year ago

Address the issue #25 It would be more flexible to use a custom base image.

LonghronShen commented 1 year ago

Thank you for your like! I have reverted the formatting.

Currently, we can't use a custom Dockerfile but the Dockerfiles in this repo. But when you have a look at these Dockerfiles, the most meaningful thing is just the FROM line, which defines the base image of the build environment. The arch-related things are handled by the buildkit and qemu automatically, but not here. In most cases, we can simplely use the predefined Dockerfiles in this repo and set the install script for the action. But in some rare cases, we want to use some special base images. For example, FROM busybox, or some older distros. So adding a custom base_image settings would be a more flexible option for us.

The core part of the PR is:

  const arch = core.getInput('arch', {
    required: true
  });
  const distro = core.getInput('distro', {
    required: true
  });
  const base_image = core.getInput('base_image', {
    required: false
  });

  // If bad arch/distro passed, fail fast before installing all the qemu stuff
  const dockerFile = path.join(
    __dirname, '..', 'Dockerfiles', `Dockerfile.${arch}.${distro}`);

  // If a custom base image is given, then dynamically create its Dockerfile.
  if (base_image) {
    let lines = [];
    lines.push(`FROM ${base_image}`);
    lines.push("COPY ./run-on-arch-install.sh /root/run-on-arch-install.sh");
    lines.push("RUN chmod +x /root/run-on-arch-install.sh && /root/run-on-arch-install.sh");
    console.log(`Writing custom Dockerfile to: ${dockerFile} ...`);
    fs.writeFileSync(dockerFile, lines.join("\n"));
  }

  if (!fs.existsSync(dockerFile)) {
    throw new Error(`run-on-arch: ${dockerFile} does not exist.`);
  }

The basic idea here is to add a base_image input parameter and only if it is set, the script would generate a Dockerfile based on the base image you given with the name from arch and distro. This will give the end user a chance to replace an existing predefined Dockerfile or add a new combination by themself.

martin-g commented 1 year ago

Hi @LonghronShen ! I didn't mean to explain the purpose of base_image here as a PR comment. I meant to add more info either in the README or at https://github.com/uraimo/run-on-arch-action/pull/103/files#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R43 Currently one has to read the implementation or the linked issue to understand the idea. (Or at least I needed to read them to get it! :-) )

LonghronShen commented 1 year ago

Oh, sorry for misunderstanding. Let me add more description there.

uraimo commented 1 year ago

Hi, yes, I would put an example of a valid base_image value in the readme (maybe with a link to docket hub to make it even more clear) and that could be enough. I will change it a little when I will refactor how Dockerfiles are handled (removing them) introducing a "none" value for arch and distro but for now it can stay as it is in this PR.

LonghronShen commented 1 year ago

I have added some documentation there. Yeah, I think in fact, we don't even need to have these dockerfiles stay in the repo. All the dockerfiles could be built on the fly.

By the way, with this PR merged, some issues related to native x86_64 build within the same matrix should also be addressed, because they can freely use native x86_64 images now, or mips64el, or others that qemu and docker can handle.

uraimo commented 1 year ago

Yeah, the idea behind the Dockerfiles was to provide basic and tested defaults that you could just use without searching for one on docker hub, but it has now become a nightmare to manages. The default will stay but in a simpler default_images file easier to maintain for me. But yes, with the refactoring this code will just become one of the two possible ways to initialize the Dockerfile built on startup. For x86_64 the plan was to also give a way to skip the embedded dockerfile altogether, but I'm still not sure if there are really use cases for that.

LonghronShen commented 1 year ago

As for native x86_64 support, there are two options:

  1. Fully skip the docker-in-docker build way for x86_64 case Pros: better performance, save build time. Cons: inconsistant workflow, x86_64 would be a real exception, and some build bugs cannot be reproduced in the non-dind way (I really came across such bug before).

  2. Keep the current workflow even for x86_64 Pro: all these archs use the same workflow, and easy to maintain. Con: dind may take more time for x86_64 case. Note: in this case, qemu will not affect the actual build process because the base image is also x86_64 and qemu will do nothing. So as for build performance itself, it is just fine.

I prefer the option 2.

uraimo commented 1 year ago

Included in 2.5.0, arch and distro should now be none when using base_image.