Eliminate cache tarballs by synthesizing packuments at build time - #97
Eliminate cache tarballs by synthesizing packuments at build time#97ogajduse wants to merge 2 commits into
Conversation
Instead of generating and storing a pre-built npm cache tarball as an extra Source, synthesize minimal registry packuments from the source tarballs during %prep. This gives npm install --offline everything it needs to resolve the dependency tree without any external artifact. - bundle.mustache: replace cache tarball extraction with inline synthesize script; remove superfluous %clean section - lib/spec_file_generator.js: drop cache tarball Source line - bin/npm2rpm.js: remove createNpmCacheTar() and dead imports - bin/generate_npm_tarball.sh: delete (dead code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Is this new? Back when I built this solution I couldn't find anything that would do this, but to be fair, that was before
And it would be a while before we can use this on RHEL.
I think this is pretty good in the short term. In foreman-packaging we already have logic to regenerate the entire spec file so updating it after the fact is easier. |
ekohl
left a comment
There was a problem hiding this comment.
This solves a massive issue I always had with bundled dependencies. Really neat.
| BuildRequires: nodejs-packaging | ||
| %if 0%{?rhel} == 10 | ||
| # https://issues.redhat.com/browse/RHEL-137712 is fixed in RHEL 10.3 | ||
| BuildRequires: /usr/bin/node |
There was a problem hiding this comment.
Because the %prep phase now uses node directly, should this be an unconditional dependency?
ekohl
left a comment
There was a problem hiding this comment.
I've spent quite a bit of time playing with this and reading the npm sources. This is effectively inlining what the cacache package does. That in turn uses ssri.
The hardcoding of content-v2 and index-v5 means it's tightly coupled to those packages. In the short term, this will probably be OK but longer term it can break.
The synthesize script (~80 lines) is fully generic - no package-specific logic, just takes a cache dir and tarball list. It's currently inlined into bundle.mustache, which adds ~80 lines to every generated spec but requires no extra BuildRequires and has no build-ordering constraints.
There's also another solution to this: create a Source: entry for the file. If you store it in git itself, it should be deduplicated internally. You can also put it on the internet somewhere so regular spectool can fetch it, and we could even annex it. Given its size, I wonder how useful that is.
The ideal was if npm itself actually implemented this. So a variation on option 1: patch npm (ideally in packaging) to synthesize a registry cache. Perhaps the Fedora / RHEL NPM maintainers are open to that. I'd be happy to help start that conversation.
| for tgz in %{sources}; do | ||
| echo $tgz | grep -q registry.npmjs.org || npm cache add --cache %{npm_cache_dir} $tgz | ||
| npm cache add --cache %{npm_cache_dir} $tgz | ||
| done |
There was a problem hiding this comment.
Now that it doesn't have to be filtered, I think you can call it in a single command
| for tgz in %{sources}; do | |
| echo $tgz | grep -q registry.npmjs.org || npm cache add --cache %{npm_cache_dir} $tgz | |
| npm cache add --cache %{npm_cache_dir} $tgz | |
| done | |
| npm cache add --cache %{npm_cache_dir} %{sources} |
| for (const [name, packument] of Object.entries(packuments)) { | ||
| const packBuf = Buffer.from(JSON.stringify(packument)); | ||
| const contentHash = crypto.createHash('sha512').update(packBuf).digest('hex'); | ||
| const contentIntegrity = 'sha512-' + crypto.createHash('sha512').update(packBuf).digest('base64'); | ||
|
|
||
| const contentDir = path.join(cacacheDir, 'content-v2', 'sha512', contentHash.slice(0, 2), contentHash.slice(2, 4)); | ||
| fs.mkdirSync(contentDir, {recursive: true}); | ||
| fs.writeFileSync(path.join(contentDir, contentHash.slice(4)), packBuf); | ||
|
|
||
| const encodedName = name.replace('/', '%2f'); | ||
| const cacheKey = 'make-fetch-happen:request-cache:https://registry.npmjs.org/' + encodedName; | ||
| const keyHash = crypto.createHash('sha256').update(cacheKey).digest('hex'); | ||
| const indexDir = path.join(cacacheDir, 'index-v5', keyHash.slice(0, 2), keyHash.slice(2, 4)); | ||
| fs.mkdirSync(indexDir, {recursive: true}); | ||
|
|
||
| const indexEntry = JSON.stringify({ | ||
| key: cacheKey, | ||
| integrity: contentIntegrity, | ||
| time: 0, | ||
| size: packBuf.length, | ||
| metadata: { | ||
| time: 0, | ||
| url: 'https://registry.npmjs.org/' + encodedName, | ||
| reqHeaders: {accept: 'application/json'}, | ||
| resHeaders: {'cache-control': 'public, max-age=300', 'content-type': 'application/json'}, | ||
| options: {} | ||
| } | ||
| }); | ||
| const entryHash = crypto.createHash('sha1').update(indexEntry).digest('hex'); | ||
| fs.writeFileSync(path.join(indexDir, keyHash.slice(4)), entryHash + '\t' + indexEntry + '\n'); | ||
| } |
There was a problem hiding this comment.
If you could use libraries, I think this would be equivalent to:
for (const [name, packument] of Object.entries(packuments)) {
const packBuf = Buffer.from(JSON.stringify(packument));
const url = 'https://registry.npmjs.org/' + name.replace('/', '%2f');
const cacheKey = 'make-fetch-happen:request-cache:' + url;
const metadata = {
time: 0,
url: url,
reqHeaders: {accept: 'application/json'},
resHeaders: {'cache-control': 'public, max-age=300', 'content-type': 'application/json'},
options: {}
}
cacache.put(cacheDir, cacheKey, packBuf, { metadata: metadata });
}| const encodedName = name.replace('/', '%2f'); | ||
| const cacheKey = 'make-fetch-happen:request-cache:https://registry.npmjs.org/' + encodedName; |
There was a problem hiding this comment.
I think this is cleaner:
| const encodedName = name.replace('/', '%2f'); | |
| const cacheKey = 'make-fetch-happen:request-cache:https://registry.npmjs.org/' + encodedName; | |
| const url = 'https://registry.npmjs.org/' + name.replace('/', '%2f'); | |
| const cacheKey = 'make-fetch-happen:request-cache:' + url; |
| size: packBuf.length, | ||
| metadata: { | ||
| time: 0, | ||
| url: 'https://registry.npmjs.org/' + encodedName, |
There was a problem hiding this comment.
Assuming you defined url above:
| url: 'https://registry.npmjs.org/' + encodedName, | |
| url: url, |
|
I've created theforeman/foreman-packaging#13702 as a standalone PR to make it easy to bulk rewrite all packages. |
execSync defaults to maxBuffer: 1MiB. @patternfly/react-tokens@5.4.1 lists 22763 entries (~1.3MB), so `tar tf` overflows the buffer and throws ENOBUFS. The catch swallowed it, no packument was written for that package, and the build failed later with a misleading ENOTCACHED naming a package other than the one being built. Raise maxBuffer on both tar invocations, and turn the silent `continue` into a throw: every Source must yield a packument entry, so a read failure is always a real problem and should name the tarball that caused it. Reproduced on nodejs-patternfly-react-core (5.4.14) and nodejs-patternfly-react-charts (7.4.9), the two bundle packages in foreman-packaging that depend on @patternfly/react-tokens. Verified offline (podman --network=none, AlmaLinux 10, npm 10.9.8) across all 43 bundle packages: 43/43 pass with this change, 41/43 without. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
%prepnpm install --offlineresolves the full dependency tree without any external artifactgenerate_npm_tarball.sh(dead code),createNpmCacheTar(), and superfluous%cleansectionBackground
foreman-packaging stores bundle-strategy npm cache tarballs directly in git as regular blobs (not git-annexed), because those tarballs have no upstream URL to fetch them from. This adds up to 113 MB across 43 packages currently, and grows every time a bundled package is regenerated. Full writeup: theforeman/foreman-packaging#13080 (comment)... (see also the discussion on ogajduse#1).
How it works
No tarball is generated at all - not as a static asset, not at build time. Instead:
npm cache addputs each source tarball's content into_cacache/content-v2/(npm does this natively)%prepextractspackage.jsonfrom each tarball, computes integrity hashes (sha512/sha1), and writes ~2KB JSON packument entries directly into npm's_cacache/index-v5/and_cacache/content-v2/stores - this is the registry metadata npm needs to map package names/versions to the already-cached tarball contentnpm install --offlinefinds both tarball content and registry metadata in the cache and resolves/installs successfullyOld flow:
npm installonline -> capture_cacache-> tar it -> store in git -> extract at build timeNew flow:
npm cache addeach Source + synthesize packument JSON -> cache is ready in-placeBugs found and fixed during prototyping:
%2fencoding (e.g.@babel%2fruntime-corejs2), not a literal/package.jsonis located viatar tflisting instead of assumingpackage/package.jsonwarning@3.0.0+warning@4.0.3) need all versions accumulated into one packument, not overwrittenBuild validation
Tested with
nodejs-patternfly-react2.40.0 (92 dependencies, including scoped packages and multi-version deps):This affects only the bundle strategy (
single.mustacheis untouched). foreman-packaging currently has 42 bundle-strategy nodejs packages; so far onlynodejs-patternfly-reacthas been tested end-to-end with this change.Impact on foreman-packaging
Eliminates cache tarballs entirely once packages are regenerated with this template - no external hosting infrastructure (Pulp, S3, HTTP server, etc.) needed to solve the storage problem.
Requirements
%{sources}RPM macro (rpm >= 4.19): available on Fedora rawhide and RHEL 9+. Not available on RHEL 8.BuildRequires)Open question: macro extraction
The synthesize script (~80 lines) is fully generic - no package-specific logic, just takes a cache dir and tarball list. It's currently inlined into
bundle.mustache, which adds ~80 lines to every generated spec but requires no extraBuildRequiresand has no build-ordering constraints.We discussed extracting it into an RPM macro on ogajduse#1. Considered homes:
nodejs-packaging(Fedora-owned) - would need Fedora nodejs SIG buy-inforeman-buildsubpackage offoreman.spec- rejected, since it would force all ~42 nodejs bundle packages to wait forforemanto build first (currently they build in parallel)npm2rpm-macrospackage/subpackage - npm2rpm already owns this logic, would be a tiny package with no runtime dependenciesProposing to ship inline for this PR and revisit macro extraction as a follow-up once this is validated across more of the 42 bundle packages in foreman-packaging.
Test plan
🤖 Generated with Claude Code