cross-platform-actions / action

Cross-platform GitHub action
MIT License
128 stars 19 forks source link

Add architecture alias for x86-64: x64 #58

Closed PiotrSikora closed 1 year ago

PiotrSikora commented 1 year ago

When using act or self-hosted runners, the host could be aarch64, at which point the cost of the additional virtualization to the default x86_64 architecture is not worth it for someone trying to test if a project builds on a given OS, without caring about the specific architecture.

Also, I've tried a few options to set it explicitly to $RUNNER_ARCH, but for some reason none of them work, even though I can echo the variable just fine:

architecture: $RUNNER_ARCH
architecture: ${{ env.RUNNER_ARCH }}
architecture: ${{ format('{0}', env.RUNNER_ARCH) }}

Any ideas?

jacob-carlborg commented 1 year ago

When using act or self-hosted runners, the host could be aarch64, at which point the cost of the additional virtualization to the default x86_64 architecture is not worth it for someone trying to test if a project builds on a given OS, without caring about the specific architecture.

Do you want the default architecture to follow the host/runner architecture?

Also, I've tried a few options to set it explicitly to $RUNNER_ARCH, but for some reason none of them work, even though I can echo the variable just fine:

Which value is actually passed to the action? You can see that in the log, here's an example:

Screenshot 2023-08-02 at 09 07 09
PiotrSikora commented 1 year ago

Do you want the default architecture to follow the host/runner architecture?

Yes, I think that's a better default than any hardcoded value (with a caveat that x64_64 is the only available architecture on runners hosted by GitHub, but presumably it was selected for the same reason as I'm suggesting here, i.e. don't virtualize another CPU for no reason).

But of course it's only a suggestion.

Which value is actually passed to the action? You can see that in the log, here's an example:

It was empty in all 3 cases, but that could be an issue with act.

However, right after I gave up and filled this PR, I found a way to use it:

architecture: ${{ runner.arch }}

Note that this evaluates to X64 on x86_64, which isn't supported value, so it doesn't work right now.

From the docs for variables in GitHub Actions:

RUNNER_ARCH | The architecture of the runner executing the job. Possible values are X86X64ARM, or ARM64.

jacob-carlborg commented 1 year ago

Yes, I think that's a better default than any hardcoded value

Hmm, I'm not sure if I think that's a good API. It would be a breaking change as well, although unlikely someone would be affected. If someone would run this action on a AArch64 self hosted runner (I don't even know if the action works on that).

From the docs for variables in GitHub Actions:

RUNNER_ARCH | The architecture of the runner executing the job. Possible values are X86, X64, ARM, or ARM64.

X64, that's an odd choice of name. I guess I could add an alias for that name. In the meantime you can use a conditional to map the value, here's an example:

https://github.com/cross-platform-actions/openbsd-builder/blob/d00dce49c326a40f295df380f70e9b7913b954d9/.github/workflows/build.yml#L40-L44

PiotrSikora commented 1 year ago

Hmm, I'm not sure if I think that's a good API. It would be a breaking change as well, although unlikely someone would be affected. If someone would run this action on a AArch64 self hosted runner

Fair enough!

(I don't even know if the action works on that).

I thought it did, but now I'm consistently getting this weird error on Linux/aarch64 host using act, so perhaps it doesn't:

/tmp/5fe578c4-9d15-494c-a2f3-2d9f154a1775/qemu-img convert -f qcow2 -O raw /tmp/bb159e62-b118-4411-bf00-a57a0c1be7a5 /tmp/5fe578c4-9d15-494c-a2f3-2d9f154a1775/disk.raw
/tmp/5fe578c4-9d15-494c-a2f3-2d9f154a1775/qemu-img: 1: Syntax error: "(" unexpected

I think this issue can be closed, unless you want to keep it open as a reminder to add X64.

jacob-carlborg commented 1 year ago

I thought it did, but now I'm consistently getting this weird error on Linux/aarch64 host using act, so perhaps it doesn't:

Yeah, that error looks weird.

PiotrSikora commented 1 year ago

Thanks!