Conversation
|
Fire 🔥 |
krdlab
left a comment
There was a problem hiding this comment.
Thank you for working on this.
I had taken the absence of upstream ARM64 archives to mean Windows ARM64 was out of reach, so using the existing x86/x64 builds under emulation is not an approach I had considered. All three windows-11-arm jobs are green, including 3.4.7 on the 32-bit toolchain.
Four changes are needed before merge: the README update below, and three points in inline comments on src/asset.ts and src/asset.test.ts.
That assumption of mine is written into the README, so please update it in this PR. Line 20 still says Windows ARM64 is unsupported, and the "Supported combinations" table has no windows-11-arm row. README.md is not in this diff, so I am raising it here rather than inline.
Suggested row:
| `windows-11-arm` (Windows 11 ARM64) | stable / `latest` (x86/x64 binaries under emulation) |Suggested note:
- Windows ARM64 has no upstream Haxe / Neko ARM64 archives. This action installs the Windows
x64 builds and relies on the emulation built into Windows 11 on ARM. For Haxe 3.x it selects
the 32-bit x86 builds of both Haxe and Neko, because Haxe 3's Windows haxelib is 32-bit.Once the source changes are in: dist/ is committed, so please run npm run dist and include the regenerated bundle.
| export function resolveTarget(input: ResolveInput): Resolution { | ||
| const { tool, platform, arch } = input; | ||
| let { tool, platform, arch } = input; | ||
|
|
||
| if (platform === 'win32' && arch === 'arm64') { | ||
| return { | ||
| kind: 'unsupported', | ||
| reason: 'Windows ARM64 is not supported (no upstream Haxe/Neko archives).', | ||
| }; | ||
| core.info('Windows ARM64 has no upstream Haxe/Neko archives, falling back to x86_64 emulation.'); | ||
| arch = 'x64'; |
There was a problem hiding this comment.
Telling users what is happening here is worth doing. Before merge, though, please move the notice out of resolveTarget() and adjust the wording.
resolveTarget() was a pure mapping function, and core.info() gives it a side effect. Since cachePlatform, downloadUrl and fileNameWithoutExt each call it, the notice is emitted five times when neither tool is cached and once when both are, both visible in the 3.4.7 job on this PR.
The wording also misses the Haxe 3.x case: there the action selects haxe-3.4.7-win.zip and neko-2.3.0-win.zip, which are 32-bit x86 builds.
A shared predicate keeps the selection rule in one place, and setup() owns the notice. In asset.ts, this replaces the destructuring and the Windows ARM64 block:
// NOTE: no upstream Haxe/Neko ARM64 archives for Windows; the x86/x64 builds run under the
// emulation built into Windows 11 on ARM.
export function usesWindowsEmulation(platform: NodeJS.Platform, arch: string): boolean {
return platform === 'win32' && arch === 'arm64';
}
export function resolveTarget(input: ResolveInput): Resolution {
const { tool, platform } = input;
const arch = usesWindowsEmulation(platform, input.arch) ? 'x64' : input.arch;setup() then gains the notice. os and core are already imported there; usesWindowsEmulation needs adding to the ./asset import:
if (usesWindowsEmulation(os.platform(), os.arch())) {
core.info('Windows ARM64: using the Windows x86/x64 builds under emulation.');
}| ['win32', 'x64', '2.4.0', false, 'neko-2.4.0-win64.zip', 'v2-4-0'], | ||
| ['win32', 'x64', '2.3.0', true, 'neko-2.3.0-win.zip', 'v2-3-0'], | ||
| ['win32', 'arm64', '2.4.0', false, 'neko-2.4.0-win64.zip', 'v2-4-0'], | ||
| ['win32', 'arm64', '2.3.0', true, 'neko-2.3.0-win.zip', 'v2-3-0'], |
There was a problem hiding this comment.
These pin the archive names well, but two gaps are left.
They pass force32 directly, so NekoAsset.resolveFromHaxeVersion, which decides to force 32-bit on Windows, is never exercised on ARM64. The existing test for it, Haxe 3.4.7 on Windows -> force32=true, covers x64 only. Before merge, please add a case to the NekoAsset.resolveFromHaxeVersion describe:
it('Haxe 3.4.7 on Windows ARM64 selects 32-bit Neko', () => {
setOs('win32', 'arm64');
const neko = NekoAsset.resolveFromHaxeVersion('3.4.7', false);
expect(neko.version).toBe('2.3.0');
expect(neko.cachePlatform).toBe('win');
});The resolveTarget cachePlatform (haxelib cache key compatibility) table also has no Windows ARM64 rows. It pins the Haxe cachePlatform values that feed the haxelib cache key, and Windows ARM64 now shares win64 and win with Windows x64. That seems right to me, since the archives are identical, but the values are worth pinning. Before merge, please add these after the ['neko', '2.4.0', 'linux', 'arm64', false, 'linux-arm64'], row, line 228 on this head. The Neko rows do not feed the cache key; they are useful as resolver coverage:
['haxe', '4.3.7', 'win32', 'arm64', false, 'win64'],
['haxe', '3.4.7', 'win32', 'arm64', false, 'win'],
['haxe', 'latest', 'win32', 'arm64', true, 'win64'],
['neko', '2.4.0', 'win32', 'arm64', false, 'win64'],
['neko', 'latest', 'win32', 'arm64', true, 'win64'],
No description provided.