Cross-platform compatibility

[matteosz]

This PR resolves #494 and resolves #492

I tested the code on a 32-bit Ubuntu VM and tests run smoothly now.
It also adds a new step on the CI to run the tests on Alpine Linux x86.

[github-actions]

🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer.

[ineiti]

What did you decide regarding putting this only in a v4, like discussed in the PRs you cite? Did you suppose that, because it panics anyway on a 32-bit architecture, it doesn't matter? Please comment this at least in this PR.

I also strongly suggest to start a CHANGELOG.md. I try to put mine in the following format:

https://keepachangelog.com/en/1.1.0/

For all the very nice PRs you're currently doing on kyber, there should be at least one line in the CHANGELOG.md

PS: For small projects, I put the CHANGELOG in the README.md. But for projects like kyber, it deserves its own file.

[AnomalRoil]

Overall, it doesn't make sense that "just using explicitly int64 instead of int" should solve the 32-bit issue unless we're actually doing something that is assuming a int64 somewhere on a int.
It'd be more interesting to bissect on these changes to find the exact culprit and fix it there rather than just use int64 everywhere.

@ineiti these changes are part of the dev effort for the v4 as I understand it.

[ineiti]

Overall, it doesn't make sense that "just using explicitly int64 instead of int" should solve the 32-bit issue unless we're actually doing something that is assuming a int64 somewhere on a int. It'd be more interesting to bissect on these changes to find the exact culprit and fix it there rather than just use int64 everywhere.

From what I understand, protobuf is complaining that detected a 32bit machine, please use either int64 or int32. So, yes, changing everything to int32 or int64 will actually solve this problem :)

The other problem it solves, for which there is not est, is hashing an int: once as int32 on 32-bit machines, and once as int64 on 64-bit machines, which would give two different results in the hash.

@ineiti these changes are part of the dev effort for the v4 as I understand it.

OK, great

[sonarcloud]

Quality Gate failed

Failed conditions
13.9% Duplication on New Code (required ≤ 10%)

See analysis details on SonarCloud

[AnomalRoil]

Let's wait for the drand/kyber merge before merging this, since it's touching the DKG code and will conflict.

[AnomalRoil]

Maybe add a step that does a uname -a and a go env just to be able to double check that it works?

[ineiti]

You can see that in the runner anyway, at the top...

[AnomalRoil]

I don't think that would be the case for the Alpine x86 job, since it seems to run on the same worker, no?

[ineiti]

Ah, I didn't see that it only runs 3 out of the 4 entries in the matrix - according to the Standard Github Runners, the ubuntu-latest-x86 doesn't exist. So perhaps it's just ignored?

If you look at the checks from this PR, only 3 are run. And I cannot see anything about two tests being run on the ubuntu-latest.

[matteosz]

I don't think the new workflow would be executed until the changes haven't been merged

[ineiti]

Now I'm confused:

• according to the header, the synchronize type will launch the script every time you do a git push
• I can't find your changes to the file .github/workflows/test_on_pr.yml anymore - did you remove the changes?

I do think it's a good idea to add the 32-bit test to the workflow. And if I'm not mistaken, the workflow has actually been run. It's just that the -x86 target doesn't exist. But I didn't test it.

[matteosz]

So, yes the synchronize make the CI to run for every git push, however the CI that is run is the one which is already on the master branch (or the drandmerge since it's the base branch now). Hence, I changed the workflow to execute separate jobs (thus avoiding the -x86 target issue) and the changes weren't detected either, so I moved the CI changes to a separate PR to merge before this one so we have the correct CI tests running.

[ineiti]

OK, as I have too many things going on and couldn't decide where to start, I did this:

https://github.com/ineiti/test_ubuntu_32bits/blob/main/.github/workflows/test.yaml

some notes:

• it's nicer to use a matrix, it keeps the things simpler and easier to extend
• you don't use the shell: alpine.sh {0}, so I think you're still running it on 64-bits and not 32-bits
• uname doesn't show it's 32-bits, so I used getconf LONG_BIT - thanks to stackoverflow
• ChatGPT was useless in this endeavour...

I hope that helps...

[matteosz]

Will update the CI with that

[ineiti]

Great, a good CI-pipeline is worth putting some effort in!

[sonarcloud]

Quality Gate failed

Failed conditions
37.0% Duplication on New Code (required ≤ 10%)

See analysis details on SonarCloud

[matteosz]

Closing this as it has been done with a simplified approach in #529