Skip to content

refactor(ixgo): use aggregate package registration#158

Open
MeteorsLiu wants to merge 1 commit into
xgo-dev:mainfrom
MeteorsLiu:refactor/ixgo-package-registration
Open

refactor(ixgo): use aggregate package registration#158
MeteorsLiu wants to merge 1 commit into
xgo-dev:mainfrom
MeteorsLiu:refactor/ixgo-package-registration

Conversation

@MeteorsLiu

Copy link
Copy Markdown
Collaborator

No description provided.

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cmd/llar/internal/install.go 72.72% 4 Missing and 2 partials ⚠️

📢 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.

FennoAI Review — PR #158: refactor(ixgo): use aggregate package registration

Collapsing ~150 hand-maintained blank imports in internal/ixgo/pkgs.go into the single aggregate _ "github.com/goplus/ixgo/pkg" is a real maintainability win, and I verified the mechanics hold up locally:

  • go build ./... → exit 0
  • go mod tidy → no changes (go.mod/go.sum are tidy-clean)
  • go mod verify → all modules verified
  • Keeping net/rpc / net/rpc/jsonrpc separate is correct — the aggregate does not register them (confirmed against goplus/ixgo@v1.1.1/pkg/go124_pkgs.go).
  • The testing/* exclusion is preserved — the aggregate does not include testing/* for go1.24.
  • The 8 //go:generate qexp directives still map 1:1 to the local generated imports, so they are not stale.

The one substantive concern is a behavioral change hidden inside a "refactor", called out inline below. A few non-line-specific notes:

Implicit testing/* exclusion is now coincidental, not enforced. Previously the excluded packages were visible in the file. Under the aggregate, testing/* stays out only because the current upstream aggregate happens to omit it; a future ixgo bump could silently register it. Consider a small test that pins the expected registered set (asserts net/rpc* present, testing absent) so these invariants survive future dependency bumps.

Binary size / linker weight. The aggregate force-links every registered subpackage (160 for go1.24), including the newly-added debug/* readers (debug/dwarf, debug/elf, debug/macho, …) and their transitive deps. The runtime/startup delta is negligible for a CLI, but binary size will grow. A quick before/after ls -l on the built binary would confirm the increase is acceptable. (os/exec and syscall were already registered before this PR — pre-existing, not introduced here.)

Overall: the refactor is fine to land; please just confirm the debug/* re-inclusion is intended (and ideally reframe as more than a pure refactor, since it changes what interpreted code can reach).

Comment thread internal/ixgo/pkgs.go
Comment thread internal/ixgo/pkgs.go
@MeteorsLiu
MeteorsLiu force-pushed the refactor/ixgo-package-registration branch from 81878b2 to 571ad59 Compare July 24, 2026 07:09
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