Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the endianness detection #12

Merged
merged 4 commits into from
Jan 25, 2024
Merged

Conversation

namrata-ibm
Copy link
Contributor

@namrata-ibm namrata-ibm commented Jan 19, 2024

The current BYTE_ORDER check is not detecting big endian correctly and test failures were observed because of this.
With this fix, all tests pass on s390x(big endian). Also this does not affect amd64 results.

Will fix #11

@namrata-ibm
Copy link
Contributor Author

@big-r81 Could you please have a look? Thank you.

@big-r81
Copy link
Contributor

big-r81 commented Jan 19, 2024

Hey @namrata-ibm,

that looks promising. Now running the fast_pbkdf2 test suite successfully on s390x too.

$ ../rebar3/rebar3 as test ct
===> Verifying dependencies...
===> Analyzing applications...
===> Compiling fast_pbkdf2
===> Running Common Test suites...
%%% pbkdf2_SUITE: ................
All 16 tests passed.

@namrata-ibm
Copy link
Contributor Author

@big-r81 can this be merged?

@big-r81
Copy link
Contributor

big-r81 commented Jan 23, 2024

It could be merged, but I’m not the owner of the repository …

@big-r81
Copy link
Contributor

big-r81 commented Jan 23, 2024

@NelsonVides ^^

@NelsonVides
Copy link
Collaborator

Hi guys! Thanks for pinging, this looks great. Let me have a careful look in a few hours.

One comment for the moment I'd like to add is added test for the new platform, currently github actions are already running for windows and macos as support for them was added recently. Do you think you can help me out with this? :)

@big-r81
Copy link
Contributor

big-r81 commented Jan 23, 2024

One comment for the moment I'd like to add is added test for the new platform, currently github actions are already running for windows and macos as support for them was added recently. Do you think you can help me out with this? :)

Sure, I (we) can try. I don't think GitHub is supporting their actions on a s390x system yet, but I'm not sure. What exactly are you thinking about?

@NelsonVides
Copy link
Collaborator

One comment for the moment I'd like to add is added test for the new platform, currently github actions are already running for windows and macos as support for them was added recently. Do you think you can help me out with this? :)

Sure, I (we) can try. I don't think GitHub is supporting their actions on a s390x system yet, but I'm not sure. What exactly are you thinking about?

What about using something like this: https://github.com/marketplace/actions/run-on-architecture? The idea would be to run tests in this architecture too in the same way that they run for MacOS and Windows, they can be slow, this repo is for the most part pretty stable so code changes are so infrequent CI can take a bit of time :)

@namrata-ibm
Copy link
Contributor Author

Hi @NelsonVides @big-r81 Yes, we can enable s390x builds to run similar steps as windows/macos using run-on-arch. Also a local run completed in 7mins, which is not very high as compared to macos build here which shows 6 mins. Please let me know if I can contribute these changes via another PR.

@big-r81
Copy link
Contributor

big-r81 commented Jan 23, 2024

Hi @namrata-ibm,

Please let me know if I can contribute these changes via another PR.

yes, if you already have this in your pipeline please do 👍

@NelsonVides
Copy link
Collaborator

Hi @NelsonVides @big-r81 Yes, we can enable s390x builds to run similar steps as windows/macos using run-on-arch. Also a local run completed in 7mins, which is not very high as compared to macos build here which shows 6 mins. Please let me know if I can contribute these changes via another PR.

That sounds just fine, exactly what we need! However I'd prefer these changes come together with this PR, as a proof that the suggested change does fix the addressed issue, to keep the repo history complete and consistent :)

@namrata-ibm
Copy link
Contributor Author

@NelsonVides @big-r81 I have added s390x in GitHub Actions, Could you please check and share your thoughts?

Copy link
Collaborator

@NelsonVides NelsonVides left a comment

Choose a reason for hiding this comment

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

That's fantastic, so cool it works, thanks a lot for the effort.

Only one small comment about the layout of the github action, if you like I can quickly push that one too :)

@namrata-ibm
Copy link
Contributor Author

@NelsonVides Apologies, but I am not able to see the changes requested here.. Could you please point what changes need to be done?

Comment on lines 98 to 105
run: |
apt-get update -y && DEBIAN_FRONTEND=noninteractive apt-get install -y rebar3 gcc libssl-dev
echo "---rebar3 as test get-deps---"
rebar3 as test get-deps
echo "---rebar3 as test compile---"
rebar3 as test compile
echo "---rebar3 as test ct---"
rebar3 as test ct
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd actually separate the commands into different steps, this way GHA itself will already build the echo printouts and we get more granularity about what is it that failed and took too long

Suggested change
run: |
apt-get update -y && DEBIAN_FRONTEND=noninteractive apt-get install -y rebar3 gcc libssl-dev
echo "---rebar3 as test get-deps---"
rebar3 as test get-deps
echo "---rebar3 as test compile---"
rebar3 as test compile
echo "---rebar3 as test ct---"
rebar3 as test ct
run: apt-get update -y && DEBIAN_FRONTEND=noninteractive apt-get install -y rebar3 gcc libssl-dev
run: rebar3 as test get-deps
run: rebar3 as test compile
run: rebar3 as test ct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this syntax is not supported with run-on-arch. Let me confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NelsonVides Above doesn't work, it throws "'run' is already defined" during startup. Also I checked some examples, however could not find multiple run statements allowed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I see 🤔 what about the install step, maybe the apt-get command can be moved to a prior install, as in https://github.com/uraimo/run-on-arch-action/blob/master/.github/workflows/advanced-example.yml,
something like the following:

Suggested change
run: |
apt-get update -y && DEBIAN_FRONTEND=noninteractive apt-get install -y rebar3 gcc libssl-dev
echo "---rebar3 as test get-deps---"
rebar3 as test get-deps
echo "---rebar3 as test compile---"
rebar3 as test compile
echo "---rebar3 as test ct---"
rebar3 as test ct
install: |
apt-get update -y
DEBIAN_FRONTEND=noninteractive apt-get install -y rebar3 gcc libssl-dev
run: |
echo "---rebar3 as test get-deps---"
rebar3 as test get-deps
echo "---rebar3 as test compile---"
rebar3 as test compile
echo "---rebar3 as test ct---"
rebar3 as test ct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, added above change

@NelsonVides
Copy link
Collaborator

@NelsonVides Apologies, but I am not able to see the changes requested here.. Could you please point what changes need to be done?

Oh, sorry, misclicked something and that one wasn't published together with the review, published now 😅

Copy link
Collaborator

@NelsonVides NelsonVides left a comment

Choose a reason for hiding this comment

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

Thank you! 😄

@NelsonVides NelsonVides merged commit 8f667bf into esl:main Jan 25, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test suite fails on s390x (big endian systems)
3 participants