Skip to content

ci: Run tests also on i686 #283

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

Merged
merged 2 commits into from
Jul 8, 2025
Merged

ci: Run tests also on i686 #283

merged 2 commits into from
Jul 8, 2025

Conversation

Jakuje
Copy link
Collaborator

@Jakuje Jakuje commented Jun 9, 2025

Fixes: #285

@Jakuje Jakuje force-pushed the i686 branch 6 times, most recently from 80b6a97 to 28ba599 Compare June 9, 2025 18:13
@Jakuje Jakuje marked this pull request as ready for review June 9, 2025 18:16
@Jakuje Jakuje changed the title ci: Run tests also on i686 ci: Run tests also on i686 and fix them Jun 9, 2025
@Jakuje Jakuje requested a review from hug-dev June 10, 2025 16:41
@Jakuje Jakuje force-pushed the i686 branch 2 times, most recently from 3712940 to 0c1a6e2 Compare June 30, 2025 13:52
@Jakuje Jakuje changed the title ci: Run tests also on i686 and fix them ci: Run tests also on i686 Jun 30, 2025
@Jakuje Jakuje force-pushed the i686 branch 4 times, most recently from 1a864d3 to f7646c9 Compare June 30, 2025 14:30
@wiktor-k
Copy link
Collaborator

wiktor-k commented Jul 7, 2025

#282 has already been fixed... Should this be closed or is there anything valuable that we can scavenge? 😁

@Jakuje
Copy link
Collaborator Author

Jakuje commented Jul 7, 2025

#282 has already been fixed... Should this be closed or is there anything valuable that we can scavenge? 😁

This is about running the tests on i386, not just building as with the ulong size on 32b arch, some things might explode even during runtime. I tried to rework it, but did not manage to finalize the linking on multiarch yet.

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
@Jakuje Jakuje force-pushed the i686 branch 3 times, most recently from 0d8d37c to a8ccd1f Compare July 8, 2025 14:46
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
@Jakuje
Copy link
Collaborator Author

Jakuje commented Jul 8, 2025

And it was a missing dollar sign all the time 🤦

image

So this should be good to review.

If not done early, I will start adding arm builds :)

@Jakuje Jakuje requested a review from wiktor-k July 8, 2025 14:53
@wiktor-k
Copy link
Collaborator

wiktor-k commented Jul 8, 2025

Okay, I think this looks good and should be merged 🚀

One question though, couldn't we adjust the test to use usize::MAX and have one thing that works regardless of the target pointer width?

I'll approve the thing anyway 😅

@Jakuje
Copy link
Collaborator Author

Jakuje commented Jul 8, 2025

One question though, couldn't we adjust the test to use usize::MAX and have one thing that works regardless of the target pointer width?

Good idea! Its always better to fix the test than skip it. I will adjust it. I was probably too focused on fixing the CI, rather than fixing the tests.

@Jakuje
Copy link
Collaborator Author

Jakuje commented Jul 8, 2025

One question though, couldn't we adjust the test to use usize::MAX and have one thing that works regardless of the target pointer width?

Good idea! Its always better to fix the test than skip it. I will adjust it. I was probably too focused on fixing the CI, rather than fixing the tests.

Actually the value 4294967295 is u32::MAX-1.

But actually this already fails non-gracefully in the test itself if changed to this value, as it looks like the max vector capacity is different on the 32b arch rust:

---- gcm_param_graceful_failure stdout ----

thread 'gcm_param_graceful_failure' panicked at cryptoki/tests/basic.rs:1865:18:
capacity overflow

So yes, I can probably try to find the vector capacity, but this would be already different test. This test tries this input specially made for the specs (commented in the code)

// Further, in v3.0, the IV is permitted to be up to 2^32-1 bytes,
// which would cause ulIvBits to overflow on platforms where
// sizeof(CK_ULONG) = 4.

So reverting back my first comment. Its probably best to skip it on 32b arch in the end.

Good to get merged.

@wiktor-k
Copy link
Collaborator

wiktor-k commented Jul 8, 2025

Oh yes, definitely. Sorry to send you on such a rabbit chase I thought it will be "just a simple fix" 😉

Merging, thanks for your work! 🙇

@wiktor-k wiktor-k merged commit b944a7d into parallaxsecond:main Jul 8, 2025
41 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.

Run CI on more different architectures in some more systematic manner
3 participants