Skip to content

use int64 explicitly in Actions support#3432

Merged
vilmibm merged 1 commit intotrunkfrom
actions-int64
Apr 19, 2021
Merged

use int64 explicitly in Actions support#3432
vilmibm merged 1 commit intotrunkfrom
actions-int64

Conversation

@vilmibm
Copy link
Contributor

@vilmibm vilmibm commented Apr 15, 2021

A user reported in #3431 that they were unable to use new Actions features on a
32bit OS. The most immediate fix was to just change from the default int to
explicit int64 for run, workflow, and checkrun IDs.

Question:

  • Is there a more elegant approach here?
  • Can we have a 32bit runner in our build matrix? If so, is it worth the additional build overhead?
  • Are we comfortable continuing to support 32bit OSes?

I'll adapt this PR based on the discussion.

@vilmibm vilmibm requested review from mislav and samcoe April 15, 2021 19:13
@jeremyd2019
Copy link

I'll recount my sad story of how I'm stuck with 32-bit Windows for you. I have this tablet, which I like and works well for me, circa 2015/2016. Due to some confluence of events between Microsoft and Intel, when this generation of devices was launched only 32-bit was ready for "Connected Standby" certification, so manufacturers asked for, and Intel provided, UEFI firmware compiled as 32-bit, which does not include legacy BIOS compatibility (another requirement for "Connected Standby" certification from Microsoft). After some time and effort, Linux eventually added support for a 64-bit kernel to be booted from 32-bit UEFI firmware, but apparently Microsoft never took that same effort for Windows.

@vilmibm
Copy link
Contributor Author

vilmibm commented Apr 15, 2021

@jeremyd2019 thanks for the context <3

we've never really worried about 32bit before -- it's something we did reflexively because Go made it easy.

I personally don't think we need to drop support for 32bit (and don't want to) -- I more asked the question to make sure that we go from "implicitly supporting it because it was easy" to "explicitly supporting it moving forward".

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

The fix looks good to me!

Regarding the long term: I do not believe that we should have a 32-bit CI runner just to keep testing this. The CI runner would tell us what we already know: that int is int32 on 32-bit systems. Instead, we could perhaps institute a linter that verifies that any *ID struct field is represented as int64. That would catch future additions of int for fields that represent auto-incrementing database columns.

@vilmibm
Copy link
Contributor Author

vilmibm commented Apr 19, 2021

I'm into the linter idea! opened an issue for that

@vilmibm vilmibm merged commit 057f5f2 into trunk Apr 19, 2021
@vilmibm vilmibm mentioned this pull request Apr 19, 2021
6 tasks
Copy link

@mohammed-ibra mohammed-ibra left a comment

Choose a reason for hiding this comment

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

Yes

paypl

This comment was marked as spam.

reevesPAC

This comment was marked as spam.

@mislav mislav deleted the actions-int64 branch May 7, 2021 10:39
hayalet27

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants