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

feat(base_image): add custom base_image argument #103

Merged
merged 3 commits into from
Dec 23, 2022

Conversation

LonghronShen
Copy link
Contributor

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

Copy link
Contributor

@martin-g martin-g left a comment

Choose a reason for hiding this comment

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

I'd revert the formatting changes and maybe explain in more detail what exactly base_image does.
But otherwise this is a really good addition!

@LonghronShen
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@uraimo
Copy link
Owner

uraimo commented Dec 23, 2022

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
Copy link
Contributor Author

LonghronShen commented Dec 23, 2022

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
Copy link
Owner

uraimo commented Dec 23, 2022

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
Copy link
Contributor Author

LonghronShen commented Dec 23, 2022

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 uraimo merged commit b346fda into uraimo:master Dec 23, 2022
@uraimo
Copy link
Owner

uraimo commented Dec 23, 2022

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

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.

3 participants