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

Proposal: Aligning the build/run protocol among platforms #36

Closed
nya3jp opened this issue Jul 4, 2020 · 16 comments
Closed

Proposal: Aligning the build/run protocol among platforms #36

nya3jp opened this issue Jul 4, 2020 · 16 comments
Labels
enhancement New feature or request

Comments

@nya3jp
Copy link
Contributor

nya3jp commented Jul 4, 2020

As initially proposed in #11, some platforms now support customizing the build/run process with participant-supplied shell scripts (e.g. #21). But other platforms don't.

To avoid confusing participants, I propose to align the build/run protocol among all platforms. In other words, all Dockerfile in this repository should be identical except for the base image reference (e.g. icfpcontest2020/python).

I imagine that such Dockerfile would be like this:

FROM icfpcontest2020/${platform}-build AS build  # This line varies by platforms

RUN mkdir -p /app
WORKDIR /build
COPY . .
RUN ./build.sh  # This script should build the solution and install it to /app

FROM icfpcontest2020/${platform}-run  # This line varies by platforms
WORKDIR /app
COPY --from=build /app .
ENTRYPOINT ["./run.sh"]

Also, if we're fine with reusing build-time images as run-time images, Dockerfile can be simplied to something like the following. Actually this looks better to me, unless it has big implication to the evaluation infrastructure, because the difference of build-time dependencies and run-time dependencies is often tricky and hard to debug.

FROM icfpcontest2020/${platform}  # This is the only line that varies by platforms

RUN mkdir -p /app
WORKDIR /build
COPY . .
RUN ./build.sh  # This script should build the solution

WORKDIR /app
ENTRYPOINT ["./run.sh"]
@jeremysawicki
Copy link

Good idea.

One suggestion: At build time you use two paths off of the root (/build and /app). I did the same in my C++ Dockerfile (/source and /build) but now I am regretting it. It makes it inconvenient to test the build script without docker. Instead we could use two subdirectories, e.g. /app/build and /app/run. If the build script does things like "cp some_file ../run" it will work outside of docker as well. Also, files that only need to exist in the run directory, like run.sh or a pre-built binary, can be placed there in the repository and not need to be copied by the build script at all.

For example:

FROM icfpcontest2020/${platform}-build AS build  # This line varies by platforms
WORKDIR /app
COPY . .
RUN mkdir -p /app/run
WORKDIR /app/build
RUN ./build.sh  # This script should build the solution and install it to /app/run

FROM icfpcontest2020/${platform}-run  # This line varies by platforms
WORKDIR /app/run
COPY --from=build /app/run .
ENTRYPOINT ["./run.sh"]

And with a single image:

FROM icfpcontest2020/${platform}  # This is the only line that varies by platforms
WORKDIR /app
COPY . .
RUN mkdir -p /app/run
WORKDIR /app/build
RUN ./build.sh  # This script should build the solution

WORKDIR /app/run
ENTRYPOINT ["./run.sh"]

@beevee beevee added the enhancement New feature or request label Jul 4, 2020
@beevee
Copy link
Member

beevee commented Jul 4, 2020

Brilliant idea! Can I ask any one of you to rewrite a single platform of your choice to @jeremysawicki's second option (single image)? We'll verify that everything works fine: that it's comfortable for you as the developers and doesn't break our submission system. Then I'll try to align all the other platforms.

@jeremysawicki
Copy link

So is there no longer a concern about keeping run images small, as you previously expressed (#8 (comment))? Or you've decided to sacrifice size for simplicity?

Sure, I can work on updating C++.

@beevee
Copy link
Member

beevee commented Jul 4, 2020

Simplicity is a strong argument, yes. I think we can handle a limited amount of large base images in our submission system.

@nya3jp
Copy link
Contributor Author

nya3jp commented Jul 4, 2020

I really like @jeremysawicki's proposal. I agree that it's great to do everything under the checkout directory to simplify local verification process. And given that @beevee is fine with the second approach to reuse build images as run images, resulting Dockerfile would be a lot simpler.

I have very minor revision proposal: what do you think about placing build.sh at the top-level directory? This is because build.sh is an important entry point and always provided statically in the source repository (participants can't generate it dynamically). We can keep run.sh in a subdirectory because it might be generated by build.sh; separating source files and generated files is generally a good practice.

So Dockerfile would be as simple as:

FROM icfpcontest2020/${platform}  # This is the only line that varies by platforms

WORKDIR /app
COPY . .
RUN ./build.sh

WORKDIR /app/run
ENTRYPOINT ["./run.sh"]

And finally, it's great if @jeremysawicki could work on applying this idea to the cpp platform. Currently it is the only platform having separated build/run images, so it would be the most interesting one to try converting to the new approach.

@nya3jp
Copy link
Contributor Author

nya3jp commented Jul 5, 2020

On second thought, I feel like moving run.sh to the top-level directory as well. It's also an important entry point so having it in the top-level directory would make it more prominent. In the case that a participant wants to dynamically generate a run script, they can just exec a generated script from run.sh.

In the end, Dockerfile is much simpler:

FROM icfpcontest2020/${platform}  # This is the only line that varies by platforms

WORKDIR /app
COPY . .
RUN ./build.sh
ENTRYPOINT ["./run.sh"]

@jeremysawicki
Copy link

Yes, I was about to agree with you about build.sh and to suggest the same for run.sh. The main reason for a run directory was to identify which files should be copied to the run image, which is not needed if we use a single image. In the event that some platform needs separate build and run images, we can just copy everything over. So I think your Dockerfile is good.

Here is an example of how build.sh and run.sh might look:

#!/bin/sh
cd app
make
#!/bin/sh
cd app
exec ./app "$@"

If someone wants to put output files in a separate directory, they can. If someone wants to put their source code at the top level and avoid the need to cd app, they can. We don't need to mandate anything other than the entry points.

We do have to choose how to structure the starter kits, though. Should we put the code at the top level or in a subdirectory? If a subdirectory, should it be called app or src or something else? I guess the current starter kits use app so it is simplest to stick with that.

@beevee
Copy link
Member

beevee commented Jul 5, 2020

I strongly prefer leaving this application in a subdirectory (/app). A team can store lots of auxiliary stuff in this repo, and I’d like to keep it separate from the application.

I’m also not sure about make. Less people are familiar with make than with simple bash scripts. Make can complicate things for beginners.

@jeremysawicki
Copy link

Regarding the app subdirectory, I'm not quite sure what you are proposing:

  • Keep the source code in an app subdirectory in the starter kits, but teams can do whatever they want?
  • Enforce that teams keep their code in an app subdirectory by only copying app, build.sh, and run.sh into the container?
  • Also keep build.sh and run.sh in the app subdirectory, and only copy app to the container?

It is starting to sound like a good idea to isolate everything in the app subdirectory. It is a one line change to the Dockerfile:

FROM icfpcontest2020/${platform}  # This is the only line that varies by platforms

WORKDIR /app
COPY app .
RUN ./build.sh
ENTRYPOINT ["./run.sh"]

Also it makes cd app unnecessary in build.sh and run.sh.

Regarding make, that was just an example of something teams might do. The starter kits can do the simplest thing possible, like calling g++ directly in starterkit-cpp.

@beevee
Copy link
Member

beevee commented Jul 5, 2020

Regarding the app subdirectory, I'm not quite sure what you are proposing

I imagine that team's repository may contain multiple applications. One of them being the submission, other some helpful auxilliary apps for development. These applications may have some common libraries. Resulting directory structure can look like this:

/lib ← required to build the submission, but also required to build helper apps
/app ← submission
/helper_app_1 ← not required for submission to build or run
/helper_app_2
...

That's why I think we should copy everything and let teams do whatever they want. But keep /app in starterkits as a default place to put the submission application to make them consistent and predictable.

@beevee
Copy link
Member

beevee commented Jul 5, 2020

But copying everything will probably make submissions quite large, since teams can and will store various massive binary files in their repos... Now I'm confused.

@beevee
Copy link
Member

beevee commented Jul 5, 2020

OK, let's go with a single image and copy everything.

@spaceorc
Copy link
Member

spaceorc commented Jul 6, 2020

I made some changes to dockerfiles and starterkit-cpp

  • renamed root directory from app -> solution - it was strange to have /app/app directory, now we have /solution/app
  • moved build output to build directory and added this directory to .gitignore

Please @nya3jp and @jeremysawicki review this changes, I'm not a cpp developer at all )

@nya3jp
Copy link
Contributor Author

nya3jp commented Jul 6, 2020

@jeremysawicki: Thanks for updating the cpp platform!

@spaceorc: Renaming the root directory to /solution sounds fine to me. Typically build.sh and run.sh operate on relative paths, so the root directory path doesn't matter anyway. I'm not sure about the build directory, I believe @jeremysawicki knows best.

I will make PRs for some platforms I'm familiar with.

@jeremysawicki
Copy link

@spaceorc: I don't have a strong opinion about either of these changes.

* renamed root directory from `app` -> `solution` - it was strange to have `/app/app` directory, now we have `/solution/app`

Yes, I thought /app/app was bit strange as well, so this is good.

* moved build output to `build` directory and added this directory to `.gitignore`

Personally I would not have made this change as it adds a small incremental complexity, but I don't think it matters much either way. I think most teams will use their own build system that they are comfortable with.

@beevee
Copy link
Member

beevee commented Jul 6, 2020

Done all the remaining platforms.

@beevee beevee closed this as completed Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

4 participants