docs: implementation plans for R-minor-sensitive builds
Split into two sequenced plans: bincraft 4.2.0 enhancements (per-minor index + classifier-driven process_cran_updates) and the pipeline integration (full + iterative). Spec revised to classify at the orchestration layer instead of a build_binary_package "auto" mode.
This commit is contained in:
parent
59c3e0960c
commit
d328f261d2
GPG key ID:
3C6318841EF78925
1 changed files with 962 additions and 14 deletions
|
|
@ -0,0 +1,470 @@
|
|||
# bincraft R-minor Enhancements Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Teach bincraft to (a) build/serve per-R-minor package indexes and (b) drive `process_cran_updates()` from the ABI classifier so only risky packages are built per R minor.
|
||||
|
||||
**Architecture:** Two additive, low-risk changes to the bincraft R package. `upload_package_index()` gains an `r_minor` argument so a per-minor slot (`…/contrib/<x.y>/`) gets its own `PACKAGES*`. `process_cran_updates()` gains `r_minor_detection`/`r_minor_sensitive_only` and classifies each candidate via `needs_per_minor_recompile()` (already in the package), passing a concrete `is_r_minor_sensitive` logical to the unchanged `build_binary_package()`.
|
||||
|
||||
**Tech Stack:** R package, `testthat` (3e) + `mockery` for tests, `roxygen2` for docs, `s3fs`/`cranlike` for the index. Repo: `https://codefloe.com/rpkgs/bincraft` (this plan is executed in a clone of that repo, NOT in build-cran-binaries).
|
||||
|
||||
**Pre-req:** PR #49 (ABI classifier) is already merged on `main`; `abi_classify()`, `needs_per_minor_recompile()`, `abi_risky_linking_deps()` are exported.
|
||||
|
||||
---
|
||||
|
||||
### Task 1: `package_index_remote_dir()` pure helper
|
||||
|
||||
**Files:**
|
||||
- Modify: `R/package_index.R` (top of file, before `add_to_package_index`)
|
||||
- Test: `tests/testthat/test-package_index.R` (new)
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
```r
|
||||
# tests/testthat/test-package_index.R
|
||||
test_that("package_index_remote_dir builds the generic slot when r_minor is NULL", {
|
||||
expect_identical(
|
||||
package_index_remote_dir("bucket", "amd64", "alpine323"),
|
||||
file.path("bucket", "amd64", "alpine323", "latest", "src", "contrib")
|
||||
)
|
||||
})
|
||||
|
||||
test_that("package_index_remote_dir appends the minor slot when r_minor is set", {
|
||||
expect_identical(
|
||||
package_index_remote_dir("bucket", "amd64", "alpine323", r_minor = "4.4"),
|
||||
file.path("bucket", "amd64", "alpine323", "latest", "src", "contrib", "4.4")
|
||||
)
|
||||
})
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `R -q -e 'devtools::load_all("."); testthat::test_file("tests/testthat/test-package_index.R")'`
|
||||
Expected: FAIL — `could not find function "package_index_remote_dir"`.
|
||||
|
||||
- [ ] **Step 3: Add the helper**
|
||||
|
||||
```r
|
||||
# R/package_index.R — add near the top, above add_to_package_index()
|
||||
#' Build the S3 remote contrib dir for a package index
|
||||
#'
|
||||
#' @param r_minor Optional `"major.minor"` string (e.g. `"4.4"`). When non-NULL
|
||||
#' the path points at the per-minor slot.
|
||||
#' @keywords internal
|
||||
package_index_remote_dir <- function(s3_bucket, arch, codename, r_minor = NULL) {
|
||||
base <- file.path(s3_bucket, arch, codename, "latest", "src", "contrib")
|
||||
if (is.null(r_minor)) {
|
||||
base
|
||||
} else {
|
||||
file.path(base, r_minor)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run test to verify it passes**
|
||||
|
||||
Run: `R -q -e 'devtools::load_all("."); testthat::test_file("tests/testthat/test-package_index.R")'`
|
||||
Expected: PASS (2/2).
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add R/package_index.R tests/testthat/test-package_index.R
|
||||
git commit -m "feat(index): add package_index_remote_dir helper for per-minor slots"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 2: per-minor support in `upload_package_index()`
|
||||
|
||||
**Files:**
|
||||
- Modify: `R/package_index.R` (`upload_package_index`, signature + the `remote_bin_dir` block at lines ~117-124; `add_to_package_index` for consistency)
|
||||
|
||||
- [ ] **Step 1: Add `r_minor` to the signature and use the helper**
|
||||
|
||||
In `upload_package_index()`, add `r_minor = NULL` to the argument list (after `arch = NULL`). Replace the inline `remote_bin_dir <- file.path(s3_bucket, arch, codename, "latest", "src", "contrib")` block with:
|
||||
|
||||
```r
|
||||
remote_bin_dir <- package_index_remote_dir(s3_bucket, arch, codename, r_minor)
|
||||
```
|
||||
|
||||
Do the same replacement in `add_to_package_index()` (it has the identical inline construction); add `r_minor = NULL` to its signature too.
|
||||
|
||||
- [ ] **Step 2: Add the roxygen param**
|
||||
|
||||
Above `upload_package_index` and `add_to_package_index`, add:
|
||||
|
||||
```r
|
||||
#' @param r_minor Optional `"major.minor"` string. When set, the index is
|
||||
#' written/read under the per-minor slot `…/contrib/<r_minor>/` instead of the
|
||||
#' generic `…/contrib/` slot.
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Regenerate docs**
|
||||
|
||||
Run: `R -q -e 'devtools::document()'`
|
||||
Expected: updated `man/upload_package_index.Rd`, `man/add_to_package_index.Rd`, no errors.
|
||||
|
||||
- [ ] **Step 4: Verify package still loads and existing tests pass**
|
||||
|
||||
Run: `R -q -e 'devtools::load_all("."); testthat::test_dir("tests/testthat")'`
|
||||
Expected: PASS — no regressions; the new path test from Task 1 still green.
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add R/package_index.R man/
|
||||
git commit -m "feat(index): upload/update PACKAGES for a per-minor slot via r_minor"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 3: `classify_r_minor_sensitive()` internal helper
|
||||
|
||||
**Files:**
|
||||
- Modify: `R/process_cran_updates.R` (add helper near the top)
|
||||
- Test: `tests/testthat/test-process_cran_updates.R` (new)
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
```r
|
||||
# tests/testthat/test-process_cran_updates.R
|
||||
test_that("classify_r_minor_sensitive returns TRUE for a risky package", {
|
||||
skip_if_not_installed("mockery")
|
||||
mockery::stub(classify_r_minor_sensitive, "clone_repository", function(pkg, tag, url, dest) {
|
||||
dir.create(dest, recursive = TRUE, showWarnings = FALSE)
|
||||
writeLines(
|
||||
c("Package: dummy", "Version: 1.0", "NeedsCompilation: yes", "LinkingTo: Rcpp"),
|
||||
file.path(dest, "DESCRIPTION")
|
||||
)
|
||||
dir.create(file.path(dest, "src"))
|
||||
writeLines("// Rcpp glue", file.path(dest, "src", "x.cpp"))
|
||||
})
|
||||
expect_true(classify_r_minor_sensitive("dummy", "1.0", "https://github.com/cran"))
|
||||
})
|
||||
|
||||
test_that("classify_r_minor_sensitive returns FALSE for a pure-r package", {
|
||||
skip_if_not_installed("mockery")
|
||||
mockery::stub(classify_r_minor_sensitive, "clone_repository", function(pkg, tag, url, dest) {
|
||||
dir.create(dest, recursive = TRUE, showWarnings = FALSE)
|
||||
writeLines(c("Package: dummy", "Version: 1.0", "NeedsCompilation: no"),
|
||||
file.path(dest, "DESCRIPTION"))
|
||||
})
|
||||
expect_false(classify_r_minor_sensitive("dummy", "1.0", "https://github.com/cran"))
|
||||
})
|
||||
|
||||
test_that("classify_r_minor_sensitive fails safe to TRUE on clone error", {
|
||||
skip_if_not_installed("mockery")
|
||||
mockery::stub(classify_r_minor_sensitive, "clone_repository", function(...) stop("boom"))
|
||||
expect_true(classify_r_minor_sensitive("dummy", "1.0", "https://github.com/cran"))
|
||||
})
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `R -q -e 'devtools::load_all("."); testthat::test_file("tests/testthat/test-process_cran_updates.R")'`
|
||||
Expected: FAIL — `could not find function "classify_r_minor_sensitive"`.
|
||||
|
||||
- [ ] **Step 3: Add the helper**
|
||||
|
||||
```r
|
||||
# R/process_cran_updates.R — add above process_cran_updates()
|
||||
#' Classify a single CRAN package for R-minor sensitivity
|
||||
#'
|
||||
#' Clones the package source to a temp dir and runs
|
||||
#' [needs_per_minor_recompile()]. Fails safe to `TRUE` (build per minor) if the
|
||||
#' clone or classification errors, so a possibly-ABI-fragile binary is never
|
||||
#' served from the cross-minor generic slot by mistake.
|
||||
#' @keywords internal
|
||||
classify_r_minor_sensitive <- function(
|
||||
package_name,
|
||||
tag,
|
||||
source_org_url = "https://github.com/cran",
|
||||
local_clone_dir = tempdir()
|
||||
) {
|
||||
dest <- file.path(
|
||||
local_clone_dir,
|
||||
sprintf("classify_%s_%s", package_name, tag)
|
||||
)
|
||||
on.exit(unlink(dest, recursive = TRUE, force = TRUE), add = TRUE)
|
||||
tryCatch(
|
||||
{
|
||||
clone_repository(package_name, tag, source_org_url, dest)
|
||||
isTRUE(as.logical(needs_per_minor_recompile(dest)))
|
||||
},
|
||||
error = function(e) {
|
||||
log_warn(sprintf(
|
||||
"{.fun classify_r_minor_sensitive}: failed for {.pkg %s} {.field %s}: %s. Treating as r-minor-sensitive.", # nolint
|
||||
package_name,
|
||||
tag,
|
||||
conditionMessage(e)
|
||||
))
|
||||
TRUE
|
||||
}
|
||||
)
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run test to verify it passes**
|
||||
|
||||
Run: `R -q -e 'devtools::load_all("."); testthat::test_file("tests/testthat/test-process_cran_updates.R")'`
|
||||
Expected: PASS (3/3).
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add R/process_cran_updates.R tests/testthat/test-process_cran_updates.R
|
||||
git commit -m "feat(updates): add classify_r_minor_sensitive helper"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 4: wire classifier detection into `process_cran_updates()`
|
||||
|
||||
**Files:**
|
||||
- Modify: `R/process_cran_updates.R` (`process_cran_updates` signature + the build loop at lines ~262-289)
|
||||
- Modify: `tests/testthat/test-process_cran_updates.R` (add routing tests)
|
||||
|
||||
- [ ] **Step 1: Write the failing tests**
|
||||
|
||||
```r
|
||||
# append to tests/testthat/test-process_cran_updates.R
|
||||
test_that("classifier mode passes per-package is_r_minor_sensitive to build", {
|
||||
skip_if_not_installed("mockery")
|
||||
recorded <- list()
|
||||
mockery::stub(process_cran_updates, "get_updated_cran_packages",
|
||||
function(...) data.frame(name = c("riskypkg", "purepkg"),
|
||||
version = c("1.0", "2.0"), stringsAsFactors = FALSE))
|
||||
mockery::stub(process_cran_updates, "get_new_cran_packages",
|
||||
function(...) data.frame(name = character(), version = character()))
|
||||
mockery::stub(process_cran_updates, "tools::CRAN_package_db",
|
||||
function(...) data.frame(Package = character(), OS_type = character()))
|
||||
mockery::stub(process_cran_updates, "classify_r_minor_sensitive",
|
||||
function(name, tag, ...) name == "riskypkg")
|
||||
mockery::stub(process_cran_updates, "build_binary_package",
|
||||
function(name, tag, ..., is_r_minor_sensitive) {
|
||||
recorded[[name]] <<- is_r_minor_sensitive
|
||||
invisible(TRUE)
|
||||
})
|
||||
|
||||
process_cran_updates(
|
||||
platform = "alpine-323", process_removed = FALSE,
|
||||
r_minor_detection = "classifier",
|
||||
s3_endpoint = "x", s3_region = "x", s3_bucket = "x"
|
||||
)
|
||||
|
||||
expect_true(recorded[["riskypkg"]])
|
||||
expect_false(recorded[["purepkg"]])
|
||||
})
|
||||
|
||||
test_that("r_minor_sensitive_only drops non-risky candidates", {
|
||||
skip_if_not_installed("mockery")
|
||||
built <- character()
|
||||
mockery::stub(process_cran_updates, "get_updated_cran_packages",
|
||||
function(...) data.frame(name = c("riskypkg", "purepkg"),
|
||||
version = c("1.0", "2.0"), stringsAsFactors = FALSE))
|
||||
mockery::stub(process_cran_updates, "get_new_cran_packages",
|
||||
function(...) data.frame(name = character(), version = character()))
|
||||
mockery::stub(process_cran_updates, "tools::CRAN_package_db",
|
||||
function(...) data.frame(Package = character(), OS_type = character()))
|
||||
mockery::stub(process_cran_updates, "classify_r_minor_sensitive",
|
||||
function(name, tag, ...) name == "riskypkg")
|
||||
mockery::stub(process_cran_updates, "build_binary_package",
|
||||
function(name, tag, ..., is_r_minor_sensitive) { built <<- c(built, name); invisible(TRUE) })
|
||||
|
||||
process_cran_updates(
|
||||
platform = "alpine-323", process_removed = FALSE,
|
||||
r_minor_detection = "classifier", r_minor_sensitive_only = TRUE,
|
||||
s3_endpoint = "x", s3_region = "x", s3_bucket = "x"
|
||||
)
|
||||
|
||||
expect_identical(built, "riskypkg")
|
||||
})
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run tests to verify they fail**
|
||||
|
||||
Run: `R -q -e 'devtools::load_all("."); testthat::test_file("tests/testthat/test-process_cran_updates.R")'`
|
||||
Expected: FAIL — `unused argument (r_minor_detection = ...)`.
|
||||
|
||||
- [ ] **Step 3: Add the parameters**
|
||||
|
||||
In the `process_cran_updates()` signature, after `filter_r_minor_sensitive = FALSE,` add:
|
||||
|
||||
```r
|
||||
r_minor_detection = c("none", "issue", "classifier"),
|
||||
r_minor_sensitive_only = FALSE,
|
||||
```
|
||||
|
||||
Immediately after the three `stop()` validation lines at the top of the body, add:
|
||||
|
||||
```r
|
||||
r_minor_detection <- match.arg(r_minor_detection)
|
||||
# back-compat: the old boolean maps onto the issue-list path
|
||||
if (isTRUE(filter_r_minor_sensitive) && r_minor_detection == "none") {
|
||||
r_minor_detection <- "issue"
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Replace the issue-filter gate**
|
||||
|
||||
The existing block reads `if (filter_r_minor_sensitive) { all_pkgs <- get_r_minor_sensitive_packages(...) }`. Change its condition to:
|
||||
|
||||
```r
|
||||
if (r_minor_detection == "issue") {
|
||||
all_pkgs <- get_r_minor_sensitive_packages(
|
||||
r_minor_packages_forge_type,
|
||||
r_minor_packages_issue_url,
|
||||
interval,
|
||||
updated_packages = updated_pkgs,
|
||||
new_packages = new_pkgs
|
||||
)
|
||||
}
|
||||
```
|
||||
|
||||
Also change the two later `if (filter_r_minor_sensitive)` log branches to `if (r_minor_detection != "none")`.
|
||||
|
||||
- [ ] **Step 5: Replace the build loop**
|
||||
|
||||
Replace the `purrr::walk2(all_pkgs$name, all_pkgs$version, ~ { build_binary_package(... is_r_minor_sensitive = filter_r_minor_sensitive ...) })` block with:
|
||||
|
||||
```r
|
||||
if (nrow(all_pkgs) > 0L) {
|
||||
sensitive <- switch(
|
||||
r_minor_detection,
|
||||
classifier = vapply(
|
||||
seq_len(nrow(all_pkgs)),
|
||||
function(i) {
|
||||
classify_r_minor_sensitive(
|
||||
all_pkgs$name[i],
|
||||
all_pkgs$version[i],
|
||||
local_clone_dir = local_clone_dir
|
||||
)
|
||||
},
|
||||
logical(1L)
|
||||
),
|
||||
issue = rep(TRUE, nrow(all_pkgs)),
|
||||
none = rep(FALSE, nrow(all_pkgs))
|
||||
)
|
||||
|
||||
if (isTRUE(r_minor_sensitive_only)) {
|
||||
all_pkgs <- all_pkgs[sensitive, , drop = FALSE]
|
||||
sensitive <- sensitive[sensitive]
|
||||
}
|
||||
|
||||
purrr::pwalk(
|
||||
list(all_pkgs$name, all_pkgs$version, sensitive),
|
||||
function(.name, .version, .sensitive) {
|
||||
build_binary_package(
|
||||
.name,
|
||||
.version,
|
||||
platform = platform,
|
||||
upload = upload,
|
||||
archive = archive,
|
||||
force = force,
|
||||
store_build_metadata = store_build_metadata,
|
||||
s3_endpoint = s3_endpoint,
|
||||
s3_bucket = s3_bucket,
|
||||
s3_region = s3_region,
|
||||
s3_access_key_id = s3_access_key_id,
|
||||
s3_secret_access_key = s3_secret_access_key,
|
||||
is_r_minor_sensitive = .sensitive,
|
||||
metadata_db_type = metadata_db_type,
|
||||
metadata_db_host = metadata_db_host,
|
||||
metadata_db_name = metadata_db_name,
|
||||
metadata_db_table = metadata_db_table,
|
||||
metadata_db_port = metadata_db_port,
|
||||
metadata_db_user = metadata_db_user,
|
||||
metadata_db_password = metadata_db_password,
|
||||
metadata_db_sslmode = metadata_db_sslmode
|
||||
)
|
||||
}
|
||||
)
|
||||
} else {
|
||||
log_info("No packages to process after filtering Windows-only packages")
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 6: Run tests to verify they pass**
|
||||
|
||||
Run: `R -q -e 'devtools::load_all("."); testthat::test_file("tests/testthat/test-process_cran_updates.R")'`
|
||||
Expected: PASS (5/5 — 3 from Task 3 + 2 new).
|
||||
|
||||
- [ ] **Step 7: Commit**
|
||||
|
||||
```bash
|
||||
git add R/process_cran_updates.R tests/testthat/test-process_cran_updates.R
|
||||
git commit -m "feat(updates): classifier-driven per-package r-minor routing + sensitive-only pass"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 5: docs, roxygen params, version bump, NEWS
|
||||
|
||||
**Files:**
|
||||
- Create: `man-roxygen/param-r_minor_detection.R`, `man-roxygen/param-r_minor_sensitive_only.R`
|
||||
- Modify: `R/process_cran_updates.R` (roxygen `@template` lines), `DESCRIPTION`, `NEWS.md`
|
||||
|
||||
- [ ] **Step 1: Add the man templates**
|
||||
|
||||
```r
|
||||
# man-roxygen/param-r_minor_detection.R
|
||||
#' @param r_minor_detection How to decide which packages are R-minor-sensitive.
|
||||
#' `"none"` (default) builds everything into the generic slot. `"issue"` uses
|
||||
#' the curated tracking issue (the legacy `filter_r_minor_sensitive` path).
|
||||
#' `"classifier"` classifies each candidate via [needs_per_minor_recompile()]
|
||||
#' and routes only `risky` packages to the per-minor slot.
|
||||
```
|
||||
|
||||
```r
|
||||
# man-roxygen/param-r_minor_sensitive_only.R
|
||||
#' @param r_minor_sensitive_only When `TRUE`, only R-minor-sensitive packages are
|
||||
#' built (used for the additional per-minor passes under non-primary R versions).
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Reference the templates**
|
||||
|
||||
In the roxygen block above `process_cran_updates`, add:
|
||||
|
||||
```r
|
||||
#' @template param-r_minor_detection
|
||||
#' @template param-r_minor_sensitive_only
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Bump version and NEWS**
|
||||
|
||||
In `DESCRIPTION` set `Version: 4.2.0`. Add to the top of `NEWS.md`:
|
||||
|
||||
```markdown
|
||||
# bincraft 4.2.0
|
||||
|
||||
* `process_cran_updates()` gains `r_minor_detection` (`"none"`/`"issue"`/`"classifier"`)
|
||||
and `r_minor_sensitive_only`, classifying each candidate via the ABI classifier and
|
||||
routing only `risky` packages to per-minor slots.
|
||||
* `upload_package_index()` / `add_to_package_index()` gain an `r_minor` argument to
|
||||
write/serve a per-minor `PACKAGES*` index under `…/contrib/<x.y>/`.
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Regenerate docs and run full check**
|
||||
|
||||
Run: `R -q -e 'devtools::document()'`
|
||||
Run: `R -q -e 'devtools::test()'`
|
||||
Expected: docs regenerate clean; all tests PASS.
|
||||
|
||||
- [ ] **Step 5: Commit, push, open PR**
|
||||
|
||||
```bash
|
||||
git add DESCRIPTION NEWS.md man-roxygen/ man/ R/process_cran_updates.R
|
||||
git commit -m "docs(release): bincraft 4.2.0 — per-minor index + classifier-driven updates"
|
||||
git push -u origin <branch>
|
||||
fj -H codefloe.com pr create --base main --head <branch> --body "<summary>" "feat: per-minor index + classifier-driven r-minor builds (4.2.0)"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Self-Review
|
||||
|
||||
- **Spec coverage:** §"Required bincraft enhancements" item 1 (process_cran_updates) → Tasks 3-5; item 2 (upload_package_index per-minor) → Tasks 1-2. `build_binary_package` unchanged per the spec — no task, correct.
|
||||
- **Type consistency:** helper named `package_index_remote_dir` (Tasks 1-2), `classify_r_minor_sensitive` (Tasks 3-4) consistently; params `r_minor_detection`/`r_minor_sensitive_only`/`r_minor` consistent across signature, tests, and docs.
|
||||
- **Placeholders:** none — every code/edit step shows the actual code; `<branch>`/`<summary>` in the final push step are deliberate operator inputs.
|
||||
- **Release dependency:** the pipeline plan pins bincraft `4.2.0`, matching the bump here.
|
||||
Loading…
Reference in a new issue