[nejdetkadir]

Companion to geastack/core#75.

Problem

Core 81cbe9f gives every non-bundle app a weak gea_embedded_assets_prepare, while core declares the reference with a plain __attribute__((weak)); on COFF that pair is a duplicate. geastack/core#75 declares it with GEA_PROGRAM_HOOK, so this target (/DGEA_LINKED_HOOK_DEFAULTS=1) references it as an ordinary symbol. build-windows.mjs adds gea_embedded_assets_generated.cpp only when it exists, and a build without it would then have no definition.

Change

• win32_hook_defaults.cpp: gea_win32_default_embedded_assets_prepare() returns true and is linked through /alternatename, like the other hook defaults. Also drops an unused <cstddef> include.
• hook-defaults.test.mjs and its fixtures: the framework fixture calls the hook. program.cpp defines it weakly, as the generator does; app_override.cpp and a new bundle_prepare.cpp define it strongly. The override and bundle runs expect a strong definition to replace the program's, and prepare-duplicate expects two strong ones to fail.

Tests

Not run on Windows. On macOS I compiled this repo's win32_hook_defaults.cpp and the fixtures with clang-cl, using the test's flags minus /MD, and linked each case of hook-defaults.test.mjs with lld-link 23.1.3. There is no CRT here, so printf, puts and the /GS cookie are stubs, and the executables were not run; each hook's provider comes from the linker map. All six cases match the test's expectations. With main's win32_hook_defaults.cpp, the defaults case fails with undefined symbol: gea_embedded_assets_prepare.

Please run hook-defaults.test.mjs on Windows before merging.

Merge order

Land this together with geastack/core#75. On its own, with core's plain weak reference, it does not conflict: a strong definition still wins, and with no generated assets unit the reference stays null.

Summary by CodeRabbit

• Bug Fixes

  • Improved Windows embedded-asset preparation behavior by providing a reliable default when applications do not supply their own implementation.
  • Application-provided preparation behavior now takes precedence where configured, including file-backed asset scenarios.

• Tests

  • Expanded Windows coverage for default, overridden, weak, and duplicate asset-preparation hooks.
  • Added validation that preparation and lookup results remain consistent across supported configurations.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: d31d5881-869f-47d4-a6fe-8f02ae1bd196

📥 Commits

Reviewing files that changed from the base of the PR and between eaf7989 and 856ba96.

📒 Files selected for processing: 7

• packages/geastack-windows/targets/win32/main/win32_hook_defaults.cpp
• packages/geastack-windows/targets/win32/test/fixtures/hook-defaults/app_duplicate.cpp
• packages/geastack-windows/targets/win32/test/fixtures/hook-defaults/app_override.cpp
• packages/geastack-windows/targets/win32/test/fixtures/hook-defaults/bundle_prepare.cpp
• packages/geastack-windows/targets/win32/test/fixtures/hook-defaults/framework.cpp
• packages/geastack-windows/targets/win32/test/fixtures/hook-defaults/program.cpp
• packages/geastack-windows/targets/win32/test/hook-defaults.test.mjs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The Win32 target now provides a default embedded-assets preparation hook. Hook-default fixtures and tests cover its invocation, linked definitions, and duplicate-definition failures.

Changes: Win32 embedded-assets preparation hook

Layer / File(s)	Summary
Register the default hook packages/geastack-windows/targets/win32/main/win32_hook_defaults.cpp	Adds a default gea_embedded_assets_prepare implementation that returns true and registers it as a linker fallback.
Exercise hook linking and invocation packages/geastack-windows/targets/win32/test/fixtures/hook-defaults/*, packages/geastack-windows/targets/win32/test/hook-defaults.test.mjs	The fixtures define preparation hooks for app, bundle, and program cases. The framework calls the hook before asset lookup. Tests check preparation and lookup outputs, and verify that duplicate strong definitions fail to link.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: dashersw

Merge Risk: ⚪ Minimal · up to 856ba

The Win32 hook default and override behavior are covered, with no concrete merge-blocking risk remaining.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 7 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: adding a linker default for the embedded-assets prepare hook.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[dashersw]

Reviewed alongside geastack/core#75. The change is correct: main/*.cpp is globbed into every Windows build (build-windows.mjs:506), so the new /alternatename default is always linked. The no-bundle weak definition the generator now emits, and the strong one it emits for file-backed bundles, both replace it as the comment describes.

1. It has not run anywhere. No workflow in this repo runs on Windows, so hook-defaults.test.mjs skips in CI as well as on macOS. Someone needs to run it on a Windows box with clang-cl and lld-link before merging. Until then this change and core#75's Windows half are both source-only.

2. Merge order. I agree these merge together. If this lands first, gea_app_entry.cpp still declares the hook __attribute__((weak)), so a clang-cl weak external with its own default meets the new /alternatename. The file header says that pairing is a duplicate on COFF. That's worth confirming on Windows, or avoiding by landing core#75 first or in the same release.

3. The test doesn't fully cover the new hook. prepare only gets the default case and the program's weak definition. There's no app-override fixture for it (the override run still prints prepare 0 from program.cpp) and no duplicate-symbol check, unlike gea_embedded_asset_lookup. The real generator also emits a strong gea_embedded_assets_prepare for file-backed bundles, and a strong-definition fixture would cover that path.

Nit: #include <cstddef> in win32_hook_defaults.cpp is unused (it predates this PR).

[nejdetkadir]

Pushed 3f33699, updated for core 81cbe9f, which now gives every non-bundle app a weak gea_embedded_assets_prepare.

1. Still not run on Windows; I'm arranging a run and will post the output here. Meanwhile I compiled this repo's win32_hook_defaults.cpp and the real fixtures with clang-cl, using the test's flags minus /MD, and linked every case of hook-defaults.test.mjs with lld-link 23.1.3 on macOS. There is no CRT here, so printf, puts and the /GS cookie are stubs, and I can't run the executables. Instead I read each hook's provider from the linker map. All six cases match the test's expectations. With main's win32_hook_defaults.cpp, the defaults case fails with undefined symbol: gea_embedded_assets_prepare.
2. With core's plain weak reference, this landing first does not conflict: a strong definition still wins, and with no generated assets unit the reference stays null. The non-bundle case is already a duplicate on main after 81cbe9f, with or without this PR (details on Reference the prepare hook through GEA_PROGRAM_HOOK and clean up stale asset manifests core#75). Both PRs together fix it.
3. program.cpp defines the hook weakly again, as the generator now does. app_override.cpp defines it strongly, and a new bundle_prepare.cpp stands in for the strong definition a file-backed app gets. The override and bundle runs expect those to replace the program's, duplicate now fails on both hooks, and prepare-duplicate checks two strong prepare definitions on their own.

Nit: removed the unused include.

[nejdetkadir]

Ran hook-defaults.test.mjs on a GitHub Actions windows-latest runner (Windows 10.0.26100, clang-cl and lld-link from LLVM 20.1.8), from a fork-only branch with this PR plus a workflow file: https://github.com/nejdetkadir/windows/actions/runs/39495396424 (job hook-defaults)

• This PR: # pass 1, # fail 0, # skipped 0.
• With geastack/windows main's win32_hook_defaults.cpp swapped in, the test fails at the defaults case: the link fails, so its lines are undefined instead of ['prepare 1', 'lookup 0 0'].

[dashersw]

Thanks for the Windows run. Merging together with geastack/core#75.