codecheckers / codecheckers/codecheck

Use up-tree lib_dir (rmarkdown 2.32) instead of rewriting lib paths after render

Open
#88 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
R
Stars
12
Forks
5
PR merge metrics
No merged PRs in 30d

Description

## Summary

rmarkdown 2.32 (merged, not yet on CRAN) contains two upstream fixes that let us delete a substantial amount of workaround code in the rendering pipeline:

- **[rstudio/rmarkdown#2632](https://github.com/rstudio/rmarkdown/pull/2632)** — `html_document_base()` no longer errors when `lib_dir` points outside the output directory. An up-tree `lib_dir: "../../libs"` now works and dependencies are referenced with up-tree relative hrefs instead of failing with *"The path <file> does not appear to be a descendant of <dir>"*. See the [maintainer's comment](https://github.com/rstudio/rmarkdown/issues/1859#issuecomment-5431523808) on #1859.
- **[rstudio/rmarkdown#1632](https://github.com/rstudio/rmarkdown/issues/1632)** (also in the 2.32 NEWS) — `render()` in a fork cluster no longer deletes sibling renders' `rmarkdown-str*.html` temp files from the shared `tempdir()`.

The first is the reason the "descendant" error forced our render-locally-then-rewrite approach. The second is the cause of the flaky parallel renders we currently sweep up after.

## What we do today

Every page renders its HTML dependencies into a page-local `libs/`, and we then rewrite the hrefs and delete that directory:

- `R/utils_render_register_htmls.R:23` — `generate_html_document_yml()` hardcodes `lib_dir: libs` (with `self_contained: false`)
- `R/utils_render_register_htmls.R:288` — `edit_html_lib_paths()` computes the depth to `docs/` and regex-rewrites `="(../)*libs/` in the rendered HTML
- `R/utils_render_register_htmls.R:276-281` and `R/utils_render_cert_htmls.R:321-324` — call it, then `unlink()` the `libs/` directory that was just written

On top of that, `R/utils_render_cert_htmls.R:232-256` sweeps up after failed parallel renders: stray `libs/` directories and stray `index_header.html` / `index_prefix.html` / `index_postfix.html` / `html_document.yml` / `temp.md` files. The inline comment names the cause precisely — *"forked processes share /tmp, causing occasional pandoc temp file conflicts"* — which is rmarkdown#1632.

`register/Makefile:49` (`make clean`) additionally has to `find docs/certs -type d -name "libs" -exec rm -rf {} +`.

## Proposed approach

1. Compute the relative depth from the page's output directory to `docs/libs` in `generate_html_document_yml()` and emit `lib_dir: <../ × depth>libs` instead of the fixed `lib_dir: libs`. The depth logic already exists inside `edit_html_lib_paths()` and can move there.
2. Delete `edit_html_lib_paths()` and its two call sites, together with the two `unlink(file.path(output_dir, "libs"))` calls.
3. Remove the parallel-render cleanup block in `R/utils_render_cert_htmls.R:232-256` once a full parallel render is confirmed clean under 2.32.
4. Drop the `libs` removal from `make clean` in the register repository.

Path resolution is unambiguous for us: `temp.md` is written into `output_dir` before rendering, so the input directory and the output directory are the same and a relative `lib_dir` resolves identically either way.

## Open question: concurrent writes into the shared `docs/libs`

Today each parallel worker owns a private `libs/` directory. Afterwards they all copy into the same one, and htmltools' dependency copy is not locked, so two workers can write the same file at the same time. Mitigation options, cheapest first:

- render one certificate serially to populate `docs/libs`, then fan out
- pre-seed `docs/libs` before the parallel section, reusing the machinery in `R/utils_external_libs.R` (`external_library_specs()`, `libs_are_current()`)

This needs to be settled before the cleanup block in step 3 is removed, otherwise we trade one race for another.

## Dependency and Dockerfile

rmarkdown 2.32 is **not on CRAN yet** (CRAN is at 2.31). Until it is released:

- `DESCRIPTION` needs `rmarkdown (>= 2.32)` in `Imports` once we rely on the new behaviour, plus a `Remotes: rstudio/rmarkdown` entry while it is unreleased
- the register's `Dockerfile` installs the package with `remotes::install_github("codecheckers/codecheck")` and `ENV R_REMOTES_UPGRADE="never"`; it must additionally install the development rmarkdown, e.g. an explicit `remotes::install_github("rstudio/rmarkdown")` step before the codecheck install, so the image does not silently fall back to the CRAN 2.31 and fail every render with the descendant error

Both the `Remotes:` entry and the extra Dockerfile step should be removed again once 2.32 reaches CRAN.

## Suggested order of work

Prototype against the GitHub build of rmarkdown and time a full parallel render before committing to it; merge when 2.32 ships.

Contributor guide

Open the contributing guide

Research direction

Start by testing the GitHub build of rmarkdown 2.32 and timing a full parallel render. Read R/utils_render_register_htmls.R, R/utils_render_cert_htmls.R, R/utils_external_libs.R, DESCRIPTION, the register Dockerfile, and register/Makefile. Done means the up-tree docs/libs approach works without path rewriting or cleanup races, dependencies are correctly pinned, and parallel renders complete cleanly.

Written by the indexing model from the issue text.

Assessment

Tech stack
docker, r
Domain
build-system, tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.