Skip to content

fix(make): prefer existing local artifacts - #197

Merged
luoliwoshang merged 3 commits into
xgo-dev:mainfrom
MeteorsLiu:codex/install-skip-existing
Sep 20, 2026
Merged

luoliwoshang merged 3 commits into
xgo-dev:mainfrom
MeteorsLiu:codex/install-skip-existing

Conversation

@MeteorsLiu

@MeteorsLiu MeteorsLiu commented Sep 20, 2026 •

Copy link
Copy Markdown
Collaborator

Prefer artifacts already installed in the local LLAR workspace and persist public remote hits for later commands.

The implementation includes:

  • Changes readThroughCache.Get to return local cache hits before querying the public Kodo cache.
  • Writes successful remote hits through the local cache so the restored artifact receives a .cache.json entry.
  • Surfaces local write-through failures and keeps source-build Put operations scoped to the local cache.
  • Covers local hits and errors, remote hits and errors, complete misses, write-through failures, .cache.json persistence, and repeated reads that skip remote access.
  • Validates the affected paths with go test -ldflags="-checklinkname=0" ./cmd/llar/internal ./internal/build/cache ./internal/crosscompile/...; the full local suite previously reported the unrelated macOS zlib v1.2.11 build failure in TestE2E_RealLibpngBuild.

This prevents existing artifacts from being overwritten and avoids downloading the same public artifact again on later llar make or llar test runs.

@codecov

codecov Bot commented Sep 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: prefer local cache before remote in readThroughCache

The core change is small and correct. Swapping readThroughCache.Get to read the local workspace cache first and fall back to remote short-circuits a per-module HTTP round-trip on incremental builds and lets locally built artifacts take precedence for a pinned version+matrix. This is consistent with Put writing only to the local cache. The refactored table-driven test in make_test.go is clear and locks in the ordering plus the per-branch call counts (gets/remoteGets). Nice.

One documentation fix worth making, plus two design notes for the record.

Should fix

  • Stale comment contradicts the new read order — cmd/llar/internal/make.go:208-209. The comment at the readThroughCache construction site still describes the old remote-first behavior:
    // Reuse published artifacts from the public Kodo store when available, and
    // fall back to the local workspace cache and source builds otherwise.
    After this PR, Get reads local first (lines 54-60), matching the updated struct doc at lines 46-48 and the new tests. This inline comment now states the ordering backwards and will mislead the next reader. Suggested rewrite:
    // Reuse artifacts from the local workspace cache when available, and fall
    // back to published artifacts in the public Kodo store (and source builds)
    // otherwise.

Design notes (non-blocking — confirm this matches intent)

  • Local-first shadows republished/security-patched remote artifacts. Cache keys are keyed only on path/version/matrix. Once a local .cache.json entry exists for a key, the remote artifact for that same key is never consulted again. If a corrected artifact is ever republished under an identical version+matrix, users with a populated workspace cache won't pick it up. If artifacts under a fixed version are considered immutable, this is fine — just confirming it's intended.

  • Local hit trusts installDir as-is without re-materializing or verifying it. On a local hit, localCache.Get (internal/build/cache.go:90-100) returns only Metadata and the builder trusts the on-disk install dir. Unlike the remote path, there's no checksum re-verification, and the dir isn't re-unpacked. If the .cache.json entry survives but its installDir is deleted/partially removed, the local-first path reports a hit with a stale/empty output instead of falling back to remote. Worth confirming that the metadata entry and its installDir are always written and invalidated together (or validating installDir existence on a local hit).

Additional findings

  • cmd/llar/internal/make.go:209: [P2] Stale comment describes old remote-first cache order: This comment still describes the pre-PR remote-first behavior, but readThroughCache.Get now reads local first and falls back to remote (matching the updated struct doc at lines 46-48 and the new tests). The ordering is stated backwards. Suggested rewrite:

@luoliwoshang

Copy link
Copy Markdown
Collaborator

》 LLARD Docker Image / build (pull_request)
LLARD Docker Image / build (pull_request)Failing after 10m

@luoliwoshang luoliwoshang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@luoliwoshang
luoliwoshang merged commit 37f7f63 into xgo-dev:main Sep 20, 2026
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants