rddepman: record full asset metadata and add the guest manifest - #511
rddepman: record full asset metadata and add the guest manifest#511jandubois wants to merge 2 commits into
Conversation
c113ec2 to
3996c0f
Compare
1cdf838 to
217e11b
Compare
The manifest paired each filename with a checksum, leaving every reader to rebuild the download URL from per-package rules. The embed-distro downloader reads the manifest and nothing else, so it cannot. Each dependency now lists its assets, and every asset carries its own url and checksum. rddepman resolves them at bump time; the install path reads them back. rddepman also takes a manifest name, so a guest manifest can follow, and `--regenerate` rewrites a manifest at its recorded versions. Signed-off-by: Jan Dubois <jan.dubois@suse.com>
The guest binaries baked into the rdd image — the distro and nerdctl — need a manifest of their own, separate from the host resources, and a second rddepman config to track them. Both sit a release behind latest on purpose, so the first CI run opens a bump PR for each. Signed-off-by: Jan Dubois <jan.dubois@suse.com>
217e11b to
d28483a
Compare
|
Consider doing this in the RD1 repo (minus the guest stuff), so we can do merges in the future. I'll do a review either way. |
|
Switched to draft mode. Commit 1 should be ported to RD1 and then merged back here. Commit 2 depends on commit 1 and will have to wait. |
mook-as
left a comment
mook-as
left a comment
There was a problem hiding this comment.
Partial review; I'll probably look at the RD1 version after instead.
| * Distribution format, when one package ships several for the same | ||
| * platform/arch (the guest distro's `raw` ext4 image vs `tar` rootfs). | ||
| */ | ||
| format?: string; |
There was a problem hiding this comment.
I think this is better called variant, because there may be non-format differences for other assets. For example, if some time down the road we supported Leap vs SLES.
| export interface DependencyEntry<K extends keyof DependencyVersions = keyof DependencyVersions> { | ||
| version: DependencyVersions[K]; | ||
| checksums: Record<string, Sha256Checksum>; | ||
| export interface DependencyAsset { |
There was a problem hiding this comment.
I guess this structure is designing mirroring Lima's image download stuff? Because using the platform/arch as the key to a map instead of iterating through everything seems easier, but it's not as expandable (format/variant). So I guess this is fine.
| assets?: unknown; | ||
| } | ||
|
|
||
| const ASSET_PLATFORMS: readonly AssetPlatform[] = ['linux', 'darwin', 'windows', 'wsl']; |
There was a problem hiding this comment.
It would be good to ensure that we covered all possible platforms here; it looks like that can be done via:
const ASSET_PLATFORMS = ['linux', 'darwin', 'windows', 'wsl'] satisfies readonly AssetPlatform[];
const _assertPlatformCoverage: AssetPlatform extends (typeof ASSET_PLATFORMS)[number] ? true : never = true;The const ASSET_PLATFORMS line ensures that all the elements are a valid AssetPlatform (via satisfies), but keeps its type as the literal strings.
The const _assert… line then checks that all possible values of AssertPlatform are a value in ASSERT_PLATFORMS, and on failure, it declares the type of the constant to be never; we then assign true to that, so if it fails we boil down to const foo: never = true; which is invalid.
The same can be done for ASSET_ARCHES.
Actually, since you have if (!ASSET_PLATFORMS.includes(raw.platform as AssetPlatform)) below, just the first line (converting the definition to satisifies) is enough; if ASSET_PLATFORMS is missing values in AssetPlatform, that becomes [A].includes([A, B]) and it fails type checking.
| if (raw.arch !== undefined && !ASSET_ARCHES.includes(raw.arch as GoArch)) { | ||
| throw new Error(`Asset for ${ name } in ${ path } has invalid arch ${ JSON.stringify(raw.arch) }`); | ||
| } | ||
| if (typeof raw.url !== 'string') { |
There was a problem hiding this comment.
| if (typeof raw.url !== 'string') { | |
| if (typeof raw.url !== 'string' || !URL.canParse(raw.url)) { |
That seems to work even for Windows file: URLs on a mac.
| }])); | ||
|
|
||
| await fs.promises.writeFile(path, MANIFEST_HEADER + YAML.stringify(serializable), { encoding: 'utf-8' }); | ||
| manifestCache.delete(path); |
There was a problem hiding this comment.
Consider ordering the delete above the write, so a partial write would not result in a stale cache (yes, I know it would throw and everything)
| * drops any other comments on the next rddepman bump. | ||
| */ | ||
| export async function writeDependencyManifest(path: string, manifest: DependencyManifest): Promise<void> { | ||
| const serializable = Object.fromEntries(Object.entries(manifest).map(([name, entry]) => [name, { |
There was a problem hiding this comment.
Isn't DependencyManifest already meant to be the on-disk format? Why is serializable needed? To drop the undefined value? Both JSON.stringify and YAML.stringify seem to do that already.
Tested with this code:
import YAML from "https://esm.sh/yaml";
const input = {
a: undefined,
b: 3,
nested: {
value: true,
missing: undefined,
}
}
document.body.append(YAML.stringify(input));
document.body.append(JSON.stringify(input));Output:
b: 3
nested:
value: true
{"b":3,"nested":{"value":true}}
Of course, if we did extra things like sorting, that might make it worthwhile.
| const versions: Partial<DependencyVersions> = {}; | ||
|
|
||
| for (const name of Object.keys(manifest) as (keyof DependencyVersions)[]) { | ||
| (versions as any)[name] = manifest[name].version; |
There was a problem hiding this comment.
| versions[name] = manifest[name].version; |
Since you didn't actually change this function, this is optional; I just happened to be reading the file directly.
| throw new Error( | ||
| `No checksum recorded for ${ name } artifact "${ artifactName }" in ${ DEP_VERSIONS_PATH }. ` + | ||
| `Available: ${ available }`, | ||
| `Expected exactly one ${ name } asset for ${ describeSelector(selector) }, found ${ matches.length }.`, |
There was a problem hiding this comment.
It would be nice to list the matching assets (by URL or something), so we can see what the issue is.
| * calls this at bump time and stores the result in `dependencies.yaml`. | ||
| * Classes that download nothing (e.g. `check-spelling`) return an empty map. | ||
| * calls this at bump time and records the result in the manifest. Classes | ||
| * that download nothing (e.g. `check-spelling`) return an empty list. |
There was a problem hiding this comment.
| * A GlobalDependency is a dependency where the version is managed in the file | ||
| * {@link DEP_VERSIONS_PATH}. | ||
| * A GlobalDependency is a dependency whose version and assets are managed in a | ||
| * `dependencies.yaml` manifest. {@link manifestPath} selects which one; |
There was a problem hiding this comment.
| * `dependencies.yaml` manifest. {@link manifestPath} selects which one; | |
| * `dependencies.yaml` manifest. {@link GlobalDependency.manifestPath} selects which one; |
Otherwise it couldn't find what you're talking about. Unfortunately this means fixing the line wrapping.
2 participants
First step of the embed-distro work: give
rddepmana self-describing manifest and add a second one for the guest binaries.Commit 1 — schema + host migration. Dependency entries now record a full url and checksum per asset, so the embed-distro downloader can fetch and verify without per-package knowledge. The host install path reads the manifest instead of re-deriving URLs.
Commit 2 — guest manifest. Adds
rdd/dependencies.yamlfor the distro and nerdctl, pinned a release behind latest on purpose so the first CI run opens a bump PR for each.