From c442e8d35c6a4ed2f0a58e31c35e0503492c9d95 Mon Sep 17 00:00:00 2001 From: pat-s Date: Tue, 1 Sep 2026 21:47:17 +0000 Subject: [PATCH] fix(build): give the existence cache a per-minor view of the slot #189 stopped per-minor objects from pruning the candidate list, which was right, but s3_cache.rds is derived from the same file_names. The cache handed to build_binary_package() therefore contained no per-minor object at all, its 'not present in the remote bucket' check answered not-present for every one, and a restarted run recompiled everything it had already built: amd64/resolute attempted 8683 packages, compiled 8683, and reported 'already exists in S3' three times. The two consumers need opposite views of one listing, and conflating them was wrong in both directions. The candidate list must ignore per-minor objects or a package present under one minor prunes itself from every other minor's work. The existence cache must not ignore them or the work is redone. The cache now keeps paths relative to the slot and build-all.R selects the part matching its pass: / for a per-minor pass, the flat slot plus Archive/ for the primary. build_binary_package() compares basenames and cannot tell 4.4/curl from curl, so the choice has to be made where the running R minor is known. A cache written before this change holds bare basenames; filtering those by path would select nothing and trigger the very rebuild this prevents, so they are used unfiltered and #190's staleness check replaces them on the next pipeline. --- local/build-all.R | 38 ++++++++++++++++++++++++++++++++++++-- local/packages-to-build.R | 24 +++++++++++++++++++++++- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/local/build-all.R b/local/build-all.R index c4d7f97..eda92e9 100644 --- a/local/build-all.R +++ b/local/build-all.R @@ -189,8 +189,42 @@ sprintf( # Read pre-computed S3 listing from install-deps step # This avoids loading s3fs/reticulate/Python in the build container, # saving significant memory for the dependency-installer subprocesses -s3_cache <- readRDS("/mnt/cache/packages/s3_cache.rds") -sprintf("S3 cache: %s files", length(s3_cache)) +s3_cache_paths <- readRDS("/mnt/cache/packages/s3_cache.rds") + +# The cache is stored as paths relative to the slot, so a pass can select the +# objects that belong to it. `build_binary_package()` compares basenames, which +# cannot distinguish `4.4/curl_1.0.tar.gz` from `curl_1.0.tar.gz`, so the choice +# has to be made here where the running R minor is known. +# +# Getting this wrong is expensive in both directions: hand it everything and a +# per-minor pass believes the flat slot's binaries are its own and builds +# nothing; hand it nothing and it rebuilds what it already has. amd64/resolute +# recompiled 8683 packages that way. +select_cache_for_pass <- function(paths, sensitive_only, r_minor) { + # A cache written before this change holds bare basenames. Filtering those by + # path would select nothing and trigger a full rebuild, so use them as they + # are; #190's staleness check replaces it on the next pipeline anyway. + if (!any(grepl("/", paths, fixed = TRUE))) { + message("S3 cache is in the legacy basename format; using it unfiltered.") + return(paths) + } + in_minor <- grepl(sprintf("^%s/", r_minor), paths) + if (sensitive_only) { + basename(paths[in_minor]) + } else { + # The primary pass writes the flat slot. Archive/ counts as present there: + # those are versions built and later superseded. + basename(paths[!grepl("^[0-9]+\\.[0-9]+/", paths)]) + } +} + +s3_cache <- select_cache_for_pass(s3_cache_paths, sensitive_only, r_minor) +sprintf( + "S3 cache: %s files (%s of %s objects apply to this pass)", + length(s3_cache), + length(s3_cache), + length(s3_cache_paths) +) n <- nrow(chunk) mapply( diff --git a/local/packages-to-build.R b/local/packages-to-build.R index 9cc1ed6..64a2f5f 100644 --- a/local/packages-to-build.R +++ b/local/packages-to-build.R @@ -176,6 +176,28 @@ source_served <- tryCatch( ) binary_cache <- setdiff(file_names, source_served) + +# The existence cache and the candidate list need different views of the same +# listing, and conflating them is what made this wrong in both directions. +# +# The candidate list must ignore per-minor objects, or a package present under +# one minor prunes itself from every other minor's work (#189). The existence +# cache must NOT ignore them, or `build_binary_package()` is told nothing is +# present under any minor and recompiles the lot: amd64/resolute recompiled +# 8683 packages it had already built, reporting "already exists in S3" three +# times. +# +# So the cache keeps the path relative to the slot, and `build-all.R` selects +# the part that matches the pass it is running: the flat slot for the primary, +# `/` for a per-minor pass. +contrib_prefix <- sprintf( + "devxy-rpkgs-binaries/%s/%s/latest/src/contrib/", + arch, + codename +) +relative_paths <- sub(contrib_prefix, "", s3_pkgs, fixed = TRUE) +source_basenames <- source_served +existence_cache <- relative_paths[!basename(relative_paths) %in% source_basenames] cat(sprintf( "S3 cache: %d objects, %d served as CRAN source, %d usable binaries\n", length(file_names), @@ -186,7 +208,7 @@ cat(sprintf( # Save the S3 file listing for the build step to use as s3_package_cache. # This avoids loading s3fs/reticulate in the build container, saving memory for # the dependency-installer subprocesses -saveRDS(binary_cache, "/mnt/cache/packages/s3_cache.rds") +saveRDS(existence_cache, "/mnt/cache/packages/s3_cache.rds") # Built from the filtered listing, not the raw one: `s3_dt` is subtracted from # the build list below, so a source fallback left in here would exclude the very # package that needs building. -- 2.54.0