Skip to content

feat(build): support crosscompile - #166

Open
MeteorsLiu wants to merge 39 commits into
xgo-dev:mainfrom
MeteorsLiu:feat/crosscompile-wiki-mvp
Open

feat(build): support crosscompile #166
MeteorsLiu wants to merge 39 commits into
xgo-dev:mainfrom
MeteorsLiu:feat/crosscompile-wiki-mvp

Conversation

@MeteorsLiu

@MeteorsLiu MeteorsLiu commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

@MeteorsLiu MeteorsLiu changed the title Feat/crosscompile wiki mvp feat(build): support crosscompile Aug 10, 2026
@MeteorsLiu
MeteorsLiu marked this pull request as ready for review August 10, 2026 10:55
fennoai[bot]

This comment was marked as outdated.

@MeteorsLiu

Copy link
Copy Markdown
Collaborator Author

/review

@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: cross-compile target support

Solid, well-tested feature (strong error-path/darwin/linux/gnu-musl coverage, defensive slice copies in the toolchain accessors). One issue is a potential process deadlock and should be addressed before merge; the rest are correctness/robustness and maintainability notes. Non-blocking review.

Highlights

  • Critical: the new target middleware panics for recoverable errors, and the execbroker holds a non-deferred RLock across the middleware call — a panic leaks the read lock and deadlocks the process. See inline comments on internal/execbroker/execbroker.go, internal/crosscompile/c/target.go, and internal/crosscompile/crosscompile.go.
  • Important: a partial --os/--arch matrix is misclassified as cross-compile (cmd/llar/internal/make.go).

Minor / maintainability (not inlined)

  • internal/crosscompile/crosscompile.go (customTarget doc comment, ~lines 33-35): "customTarget uses the bootstrap toolchain to build libc itself" is inaccurate — Load returns bootstrapTarget when root.Path == cSysroot.Path, so customTarget actually builds a consumer against a Formula-supplied libc, not libc itself. Suggest rewording.
  • internal/crosscompile/c/toolchain.go (lines 27-33): exported accessors CC()/CXX()/Linker()/Archiver()/Ranlib()/NM()/Strip() lack doc comments while every other exported symbol in the package has one; revive/golint will flag this.
  • internal/crosscompile/c/target.go (autotoolsPatch): the --host detection re-reads and scans the configure script on every autotools command. The result is deterministic per target and could be cached alongside tempDir/toolchainFile. Low impact.
  • Latent concurrency: both Use methods lazily initialize shared fields (toolchainFile/tempDir, configured) without synchronization. Safe today because builds run sequentially, but build.go carries a // TODO(MeteorsLiu): Parallel build note — worth a comment documenting the assumption or a sync.Once guard.

Findings without inline locations

  • internal/execbroker/execbroker.go:149: scopeMu.RUnlock() here is not deferred, and scope.Middleware(req) (line 146) is called inside the locked region. The new C-target middleware can panic (see crosscompile.go and c/target.go Use methods), and a panic unwinds past this line, leaking the read lock permanently and deadlocking the next scopeMu.Lock(). Recommend defer scopeMu.RUnlock() (or a recover) so the lock is always released, in addition to fixing the middleware to not panic.

Comment thread internal/crosscompile/crosscompile.go
Comment thread internal/crosscompile/c/target.go
Comment thread cmd/llar/internal/make.go
Comment thread internal/build/target.go

// Target applies language-specific target defaults to build commands.
type Target interface {
Use(Command) Patch

@MeteorsLiu MeteorsLiu Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

TODO: change this signature to Use(Command) (Patch, error) to avoid panic, it requres #167

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.

1 participant