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

Improvements to the CI Docker build #252

Merged
merged 1 commit into from
Sep 23, 2020
Merged

Improvements to the CI Docker build #252

merged 1 commit into from
Sep 23, 2020

Conversation

domenic
Copy link
Member

@domenic domenic commented Sep 23, 2020

This is a series of improvements that will help pave the way for a better CI build/deploy architecture in the future, potentially based on GitHub actions. In particular:

  • Stop grabbing pdfsizeopt using git from outside the container. Instead, get it as part of the Dockerfile along with other dependencies.

  • Avoid the multi-step PDF generation process, where we deploy to whatwg.org, point Prince at that, and then deploy the generated print.pdf to whatwg.org. Instead, we serve the built output inside the Docker container, use that to generate print.pdf, and deploy everything to whatwg.org in a single step.

  • Update Prince from v11.3 to v13.5.

  • Download only the .jar file for the validator, instead of the full package; we can let Docker handle installing Java.

This is a series of improvements that will help pave the way for a better CI build/deploy architecture in the future, potentially based on GitHub actions. In particular:

* Stop grabbing pdfsizeopt using git from outside the container. Instead, get it as part of the Dockerfile along with other dependencies.

* Avoid the multi-step PDF generation process, where we deploy to whatwg.org, point Prince at that, and then deploy the generated print.pdf to whatwg.org. Instead, we serve the built output inside the Docker container, use that to generate print.pdf, and deploy everything to whatwg.org in a single step.

* Update Prince from v11.3 to v13.5.

* Download only the .jar file for the validator, instead of the full package; we can let Docker handle installing Java.
Copy link
Contributor

@sideshowbarker sideshowbarker left a comment

Choose a reason for hiding this comment

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

Reviewed the code and this all looks fine to me, but I don’t know how to test it all locally, so I haven’t actually tested it

@sideshowbarker
Copy link
Contributor

sideshowbarker commented Sep 23, 2020

  • Download only the .jar file for the validator, instead of the full package; we can let Docker handle installing Java.

That’s fine, but for to record here: more precisely what the change in this branch is doing is that it’s moving away from using the standalone, self-contained binary of the checker that doesn’t require Java to be installed on the system (it embeds its own Java runtime) and instead switching to relying on the system having Java installed.

That’s more fragile than using the self-contained binary, because there can potentially be a mismatch between the Java version that generated the bytecode in the jar and the Java version on the system where it ends up being run.

But in practice that’s not a real risk for us right now — because the jar bytecode is produced to run in any Java 8 or later Java runtime, and Java 8 is currently the minimum Java version that Docker and any CI system are going to end up installing.

However, over the longer term, it’s possible that the checker code is going to end up using some Java features that are only available in Java 11, and then the bytecode in the jar will be produced to run under Java 11 as a minimum.

But if that were to happen accidentally, it’d be a big regression in the checker distribution and I would need to revert it quickly. However, I guess there is some small risk that before I got around to reverting it, it’d end up breaking all our builds. So that’s part of the reason why I had the build set up to use the self-contained binary — just to ensure a bit more robustness.

Anyway, all of that is just FYI. For the reasons outlined above, I think in practice it doesn’t make a real difference for us whether we use the jar distribution or the self-contained binary. So it’s right to just use whichever makes things easier.

@domenic
Copy link
Member Author

domenic commented Sep 23, 2020

Thanks for the review, and for the explanation. Yeah, it was definitely nice keeping things self-contained, but I found that downloading from GitHub was some of the slowest part of the build, so if we can utilize the caching layer for the JRE, I think that's worth it. We can always revisit if this ends up causing problems.

I tested this locally by checking out html-build and html as siblings, then running

IS_TEST_OF_HTML_BUILD_ITSELF=true html-build/ci-deploy/outside-container.sh

It seems to work well, but we'll see what happens on deploy!

@domenic domenic merged commit 6233161 into master Sep 23, 2020
@domenic domenic deleted the better-ci-docker branch September 23, 2020 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants