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.

Sign up for GitHub

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

Jump to bottom

CMakeLists: add ppc case to known archs #1816

Merged
merged 1 commit into from
Aug 27, 2024
Merged

CMakeLists: add ppc case to known archs #1816

merged 1 commit into from
Aug 27, 2024
+18 −0

Conversation

barracuda156
Copy link
Contributor

This is a trivial PR: it simply adds ppc to a list of known archs, retaining a warning re no CI for this platform.

I have confirmed the build on Darwin ppc for liboqs 0.10.1 with no other alterations to the code.

SWilson4
SWilson4 approved these changes Jun 11, 2024
View reviewed changes
Copy link
Member

@SWilson4 SWilson4 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @barracuda156. This mirrors the current configuration for ppc64, so it seems reasonable to me.

@barracuda156
Copy link
Contributor Author

@SWilson4 I just thought we may need to verify one thing. What is the value of CMAKE_SYSTEM_PROCESSOR when the build is done on 10.5 for ppc64?
If it uses uname -p under the hood, this returns powerpc, but the build arch should be ppc64.

I cannot test that immediately, since I have no build environment on 10.5, but let me check with CMake documentation.

A fallback would be to use CMAKE_OSX_ARCHITECTURES when on Apple, that returns a value matching the arch of the build. (Since gcc does not support fat builds, it will be a single arch always, on PowerPC.)

@barracuda156
Copy link
Contributor Author

@SWilson4 @baentsch Sorry for a delay, will finalize this over a coming weekend.

baentsch
baentsch previously approved these changes Jun 28, 2024
View reviewed changes
@baentsch
Copy link
Member

baentsch commented Jul 1, 2024

@SWilson4 @baentsch Sorry for a delay, will finalize this over a coming weekend.

@barracuda156 Please let us know when you're ready. Should this be moved to Draft to avoid an unintended merge?

@dstebila dstebila marked this pull request as draft August 14, 2024 20:34
@baentsch
Copy link
Member

@barracuda156 This PR is now a bit old and I removed my approval for the time being: Can you please comment (by) when you may have time to move this to completion as discussed above?

@barracuda156
Copy link
Contributor Author

@barracuda156 This PR is now a bit old and I removed my approval for the time being: Can you please comment (by) when you may have time to move this to completion as discussed above?

@baentsch Yeah, sorry, will finalize this by this weekend.

@barracuda156
Copy link
Contributor Author

Ok, so yeah, CMake will have the same value for ppc and ppc64 builds on Darwin, since it uses uname -m: https://cmake.org/cmake/help/latest/variable/CMAKE_HOST_SYSTEM_PROCESSOR.html

I will rewrite a patch in a way it gonna work correctly now.

@barracuda156
CMakeLists: add ppc case
659fa27 
Signed-off-by: Sergey Fedorov <[email protected]>
@barracuda156 barracuda156 marked this pull request as ready for review August 27, 2024 04:10
@barracuda156
Copy link
Contributor Author

@baentsch @dstebila Could you please review?

baentsch
baentsch reviewed Aug 27, 2024
View reviewed changes
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Please see the question, @barracuda156 . I also triggered CI...

baentsch
baentsch approved these changes Aug 27, 2024
View reviewed changes
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Thanks again for the contribution @barracuda156 !

@baentsch baentsch merged commit 0a8ec57 into open-quantum-safe:main Aug 27, 2024
73 checks passed
@barracuda156 barracuda156 deleted the ppc branch August 27, 2024 14:13
@barracuda156
Copy link
Contributor Author

Thanks for merging!

@BrewTestBot BrewTestBot mentioned this pull request Sep 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baentsch baentsch baentsch approved these changes

@SWilson4 SWilson4 SWilson4 approved these changes

@dstebila dstebila dstebila approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@barracuda156 @baentsch @dstebila @SWilson4