feat(make): reuse public kodo artifacts before source builds - #196
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: reuse LLAR Cloud builds before source builds
The change is small and the fallback design (swallow install errors, proceed to source build) is sound. The core "populate cache, then builder skips onBuild" mechanism works correctly in the default invocation: install and the default builder both resolve to os.UserCacheDir()/.llar/workspaces with an identical {Module, Matrix} cache key, and the builder's fast path (internal/build/build.go:276, cacheHit && !testThisMod) returns the cached artifact.
However, a few issues are worth addressing before merge — the most important being a no-timeout network call now on the build hot path, and an -o/--output case where the install work is silently wasted.
Additional findings
cmd/llar/internal/make.go:44: [P3] Command help text is now stale ("Build a module from source"): makeCmd's Short ("Build a module from source") and Long ("...builds the selected matrix from source using LLAR formulas.") no longer fully describe the behavior: make now prefers a prebuilt LLAR Cloud artifact and only falls back to source. Worth updating this user-facing help text, and thebuildModuledoc comment (make.go:126-131), which omits the new install-before-build step.
| installMods := make([]module.Version, 0, len(mods)+1) | ||
| for _, mod := range mods { | ||
| installMods = append(installMods, module.Version{Path: mod.Path, Version: mod.Version}) | ||
| } |
There was a problem hiding this comment.
[P2] With -o/--output, install work is wasted and no reuse happens
The optimization relies on install and the builder sharing a workspace dir. install always writes to os.UserCacheDir()/.llar/workspaces (install.go:133-141), ignoring any override. But when makeOutput != "", make.go:167-173 sets buildOpts.WorkspaceDir = tmpDir (a fresh os.MkdirTemp), so NewBuilder builds a cache rooted at tmpDir (build.go:87-99), gets a cache miss, and rebuilds from source anyway.
So with -o, the full install download + unpack + disk write is paid but never reused — the advertised "skip onBuild" benefit does not occur, and the inline comment's "reuse ... otherwise fall back to source" is inaccurate for this path. Consider skipping the install call when makeOutput != "", or wiring install to buildOpts.WorkspaceDir.
| for _, mod := range mods { | ||
| installMods = append(installMods, module.Version{Path: mod.Path, Version: mod.Version}) | ||
| } | ||
| if crossCompile { |
There was a problem hiding this comment.
[P3] Error message overstates cause; not all install failures mean "unavailable"
The message "install %s unavailable, building from source" is printed for any install error, but install fails for many local, non-"unavailable" reasons too: invalid service URL, os.UserCacheDir() failure, cache read/write errors (install.go:165/177), and archive unpack failures. A local failure (e.g. disk full during cache.Put) would be misattributed to remote unavailability. Consider distinguishing "no artifact available" from a genuine fetch/validation error, and softening the wording.
There was a problem hiding this comment.
Review (cont.): two findings dropped from the prior review due to a title-length limit
Adding the two most important findings that were dropped from my previous review (their titles exceeded the inline-comment length limit): the no-timeout network call and the silent remote-binary substitution.
| installMods := make([]module.Version, 0, len(mods)+1) | ||
| for _, mod := range mods { | ||
| installMods = append(installMods, module.Version{Path: mod.Path, Version: mod.Version}) | ||
| } |
There was a problem hiding this comment.
[P1] Install call has no timeout; build can hang
install(ctx, ...) is now on the hot path of every buildModule. ctx comes from context.Background() (make.go:63, no deadline) and install uses http.DefaultClient (install.go:128/208/282), which has Timeout: 0.
When llar.xgo.dev is unreachable or hung, this turns a previously offline-capable source build into one that blocks until the OS TCP timeout (tens of seconds) or, for a server that accepts but never responds, indefinitely. The error is swallowed and only logged to buildOutput (which is io.Discard unless -v), so the user just sees the build "hang" with no cause. Consider wrapping install with a bounded context.WithTimeout (or a dedicated http.Client{Timeout: ...}) so the source-build fallback triggers quickly. This is the main issue worth fixing before merge.
|
|
||
| // Reuse existing LLAR Cloud builds when available, and fall back to source | ||
| // builds for the remaining build objects, including the cross sysroot. | ||
| installMods := make([]module.Version, 0, len(mods)+1) | ||
| for _, mod := range mods { | ||
| installMods = append(installMods, module.Version{Path: mod.Path, Version: mod.Version}) | ||
| } | ||
| if crossCompile { | ||
| if sysroot, ok := c.Sysroot(targetOS, targetArch); ok && sysroot.Path != modPath { |
There was a problem hiding this comment.
[P2] make silently reuses unverified remote binary
In the default (non--o) case, a successful install makes the builder skip OnBuild and reuse the cloud artifact, so llar make — documented as "Build a module from source" — transparently returns a remote binary whenever LLAR Cloud has a match. Two concerns:
- Integrity: the download path (install.go:259-332) does no checksum/digest/signature verification before unpacking and installing; the artifact URL/content is server-controlled (HTTPS to a fixed host mitigates passive MITM but not server compromise). A pinned content digest verified before unpack would close this.
- Visibility: the substitution is invisible without
-v(buildOutput isio.Discard), so users get a prebuilt binary with no signal that no source build occurred.
At minimum this warrants a deliberate decision and a doc note; ideally artifact integrity verification.
MeteorsLiu
changed the title
This reverts commit 3beb05a.
|
目前为什么会需要云端缓存 |
Some packages cannot be built, there's only prebuilt packages for them( private packages ) |
|
Okay I Got it |
2 participants
llar makeandllar testnow reuse artifacts published on the public Kodo domain before building from source:cache.NewKodosupports credential-less reads:GetfetchespublicDomain/<module>/<version>/<matrix>.tar.gzdirectly, and treats any fetch failure as a cache miss.llar install(LLAR Cloud, builds on demand) is unchanged.Verification:
go test -ldflags="-checklinkname=0" ./internal/build/cache ./cmd/llar/internalTestMakeReal_ReusesPublishedArtifactandTestTestReal_ReusesPublishedArtifactrun the real commands againsthttps://llarpackages.xgo.devwith allgithub.comclones disabled, and assert the installedinclude/zlib.hcomes from the published artifact.