feat(build): support crosscompile - #166
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
MeteorsLiu
changed the title
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Solid, well-tested addition of C cross-compilation (LLVM toolchain, c.Target/build.Patch middleware, sysroot + CMake/autotools/pkg-config handling). Nice touches: Toolchain defensively copies its slices, CMake toolchain uses if(NOT DEFINED ...) so explicit Formula settings win, and the cross-target llar test rejection is validated early. Documentation comments all check out against behavior.
A few findings below; the inline comments cover concrete diff lines. Highest-value item is the panic()-on-I/O inside Use().
Latent data race once parallel builds land
internal/build/c/target.go:121-132 lazily initializes c.tempDir/c.toolchainFile with an unsynchronized read-check-then-write. It is safe today because Builder.Build iterates buildList sequentially (internal/build/build.go:428, guarded by the TODO(MeteorsLiu): Parallel build). But this file explicitly anticipates parallel builds — when that lands, two goroutines sharing one Target hitting the cmake branch would race on these fields and leak a temp dir (the second tempDir overwrites the first, so Close() only removes one). Generating the toolchain file eagerly in NewTarget (which already returns error) would remove both this race and the panics flagged inline.
Notes (low severity)
internal/build/c/target.go:110,114:filepath.Base(cmd.Name)is recomputed at line 114 instead of reusingbasefrom line 110. Minor readability nit.internal/build/c/target.go:318(cmakeEscape): backslash→/and"→\"correctly prevent breaking out of theset("...")quoted string, but${...}is left intact and CMake expands variable references inside quoted values at configure time. The sysroot string originates from third-party sysroot-formula metadata (metadata.Sysroot()), so it crosses a trust boundary. Not exploitable given pinned formulas, but consider rejecting/escaping$(and newlines) in these values, or documenting them as trusted.
| if c.toolchainFile == "" { | ||
| tempDir, err := os.MkdirTemp("", "llar-c-target-*") | ||
| if err != nil { | ||
| panic(fmt.Errorf("prepare CMake toolchain for %s: %w", c.target, err)) |
There was a problem hiding this comment.
Use() runs inside the exec broker middleware and cannot return an error, so I/O failures here are handled with panic() (also at line 129 and at line 182 for the configure read). This only degrades to a build error because the triggering command happens to run inside an OnBuild/OnTest hook wrapped by runFormulaHook's recover (internal/build/build.go). Any brokered command outside that wrapper — or a missing/renamed configure script (configurePath derived from cmd.Dir+cmd.Name) — would crash the whole llar process.
Suggest generating the CMake toolchain file eagerly in NewTarget (which already returns error) instead of lazily in Use(); that removes these panics and also fixes the unsynchronized lazy-init once parallel builds land. Alternatively, cache the first error on the Target and return an empty Patch.
| } | ||
|
|
||
| func commandPatch(command []string) build.Patch { | ||
| return build.Patch{Name: command[0], PrependArg: command[1:]} |
There was a problem hiding this comment.
command[0] assumes a non-empty slice. This is safe because validateToolchain rejects empty CC/CXX/Linker at construction, but a one-line // precondition: non-empty (validateToolchain) comment or a defensive length guard would make the invariant explicit for future callers.
1 participant
Design: https://github.com/xgo-dev/llar/wiki/Cross-compile-Design