docs(ci): deduplicate doc images before packaging the artifact#4157
docs(ci): deduplicate doc images before packaging the artifact#4157grandixximo wants to merge 1 commit into
Conversation
When it works as intended without too much extra work,... why not. I trust you have weighed the options and went for the better one :-) I'll have a look, later. |
|
Thanks. FWIW I did spend a fair bit of time weighing the build-integrated alternative before settling on the post-build pass. This PR adds just one file and it is readable, but I agree the reason it has to exist is ugly: the build creates the duplicates and then this cleans them up. The real alternative is not only refactoring the Ruby resolver but also shifting the build from zip to tar, so the resolver can place symlinks and have them survive. That preserves the dedup all the way through (artifact, deb, fetch), but it is a larger blast radius and would need coordination with @hdiethelm. If we agree on that shape instead, the result is a more elegant structure, and I think genuinely better. Happy to go that way if you and @hdiethelm are on board. |
|
I commented in the wrong thread. I was meaning to use Python on the web server rather than bash / php |
|
Sorry, I just see this now. Hmm, does this 300MB hurt anyone? Otherwise, we could just leave it as it is. The best solution of course would be to build the docs in a way that common images are in a common folder and the html links are correct at build time, not just clean it up afterwards. But this might be unreasonable complex, I have no clue how this doc build works. If you want to go with symlinks: https://github.com/actions/upload-artifact can do zip or raw files. So I should be able to use tar to create an archive in CI and then upload this as raw file. Should I try that in #4150? |
|
I went for .tar.gz anyway, so you are free to do what you need. If the size is a real issue for the homepage server but nowhere else, an option would be to run your dedup scrip in CI only for the artifact but not in the makefile. You can easily add another step before the tar step. If it's CI only, the right place for the script is in That would be something like: |
c3832e4 to
f535a17
Compare
|
Went with your CI-only approach. The dedup is now On a full 8-language build it takes the tree from ~324MB to ~118MB (227MB of duplicate images down to 28.8MB), self-verifies that every image reference still resolves after the rewrite, and is idempotent. This PR is only the script. Could you add the invocation before the tar step in #4150? - name: Deduplicate doc images
run: python3 .github/scripts/dedup-docs.py docs/build/html --applyOne ordering note: that step needs the script on master first, else the CI build (your branch merged onto master) cannot find .github/scripts/dedup-docs.py and fails. So merge #4157 first, then rebase here and add the line. Or guard it with |
f535a17 to
1a9eddb
Compare
|
This working on different branches on the same topic is anoying. Best you rebase on top of my branch and do the CI change yourself. If yours get merged first, my changes are in and I close mine. If mine gets merged first, you can rebase to master. |
The HTML build copies every referenced image into each language tree, so the built doc tree is ~324MB with ~227MB of duplicate images. Add a CI step that collapses byte-identical images into a shared image/ tree and rewrites the refs before the tar step, dropping the published tarball's tree to ~118MB. The pass lives in .github/scripts/dedup-docs.py, runs only in CI, and leaves the build itself untouched. It self-verifies that every image reference still resolves and is idempotent.
1a9eddb to
2535f0d
Compare
|
@hdiethelm since #4150 merged, the tar step is on master, so I added the dedup call to the CI in this PR instead of asking you to wire it. It runs in the |
|
Deduplication after the fact is wasteful. First you create a lot of copies just to do a lot of work to get rid of them again. That is working backwards and fixing the symptoms of a "maybe-not-so-good" choice that caused the problem. The better approach, IMO, is not to create the duplicates in the first place. You already have to traverse all the links for the languages to match a translated image and use it instead of the generic/English version. I have a hard time understanding why this extra work after the fact is better than prevention? |
|
It will be a bigger PR, larger blast radius, I was settling for good enough with Hannes proposal... |
The HTML build copies every referenced image into each language tree, so the built doc tree is ~324MB with ~227MB of duplicate images. This collapses byte-identical images into a shared
image/tree and rewrites the refs, dropping the published tarball's tree to ~118MB.It runs as a CI step in the
htmldocsjob, after the build and before the tar, so the build itself stays untouched and only the packaged artifact is deduplicated. The pass lives in.github/scripts/dedup-docs.py(per @hdiethelm's suggestion to keep it out of the Makefile).Verified on a full 8-language build: 227MB of images collapse to 28.8MB (87% reclaimed), the pass self-verifies that every image reference still resolves after rewriting, is idempotent, and leaves the CSS and non-image refs untouched. Languages are read from
docs/po4a.cfg, the same source the doc build uses.