Skip to content

refactor(spx): remove ixgo-aware codegen path#1500

Draft
joeykchen wants to merge 9 commits into
goplus:devfrom
joeykchen:pr/spx-drop-ixgo-codegen
Draft

refactor(spx): remove ixgo-aware codegen path#1500
joeykchen wants to merge 9 commits into
goplus:devfrom
joeykchen:pr/spx-drop-ixgo-codegen

Conversation

@joeykchen
Copy link
Copy Markdown
Contributor

Summary

This change removes the ixgo-aware codegen path from spx and keeps embedded package handling on the ispx side only.

What Changed

  • remove cmd/spx/internal/gengo
  • remove the --ixgogen flag and related branching in spx
  • switch spx code generation to embedded xgo/tool
  • add a fallback path:
    • prefer go list -m github.com/goplus/xgo
    • if that fails, fall back to system xgo env XGOROOT XGOVERSION
  • update install script and release verification to stop referencing the removed ixgogen flow
  • update --goenv help text to reflect the current behavior

Why

embeddedpkgs and ixgo-specific handling should not leak into spx.
spx should only care about generating Go code through XGo, while ispx remains the only place that is aware of embedded package behavior.

This also removes the extra codegen implementation inside spx and keeps the generation path closer to XGo's public API.

Validation

  • go test ./cmd/spx/... ./pkg/ispx/...

Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot left a comment

Choose a reason for hiding this comment

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

One regression worth fixing before this lands: the new embedded-xgo path no longer works for freshly scaffolded projects unless xgo is separately installed.

Comment thread cmd/spx/internal/command/build.go Outdated
Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot left a comment

Choose a reason for hiding this comment

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

I found two regressions in the updated generation/build flow that look worth fixing before merge.

Comment thread Makefile
Comment thread cmd/ispx/build.sh Outdated
Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot left a comment

Choose a reason for hiding this comment

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

One test-coverage regression in the diff.

Comment thread cmd/spx/install_script_test.go
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the code generation logic by removing the --ixgogen flag and transitioning from the xgobuild library to the xgo tool package. It simplifies the build process by consolidating Makefile targets, removing the internal/embeddedpkgs package, and moving embedded package registrations directly into pkg/ispx. Additionally, it introduces a more robust mechanism for resolving the xgo module environment. I have no feedback to provide.

Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot left a comment

Choose a reason for hiding this comment

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

One regression worth fixing before merging: the main generation target no longer regenerates the relocated pkg/ispx exports.

Comment thread Makefile
Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot left a comment

Choose a reason for hiding this comment

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

Found 2 issues worth addressing.

Comment thread cmd/spx/internal/command/build.go Outdated
Comment thread Makefile
@joeykchen joeykchen force-pushed the pr/spx-drop-ixgo-codegen branch from 9d0b078 to 7bd1938 Compare April 17, 2026 06:57
@joeykchen joeykchen force-pushed the pr/spx-drop-ixgo-codegen branch from 7bd1938 to 9a696f6 Compare April 17, 2026 06:57
@joeykchen joeykchen marked this pull request as draft April 17, 2026 09:48
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