Skip to content

Added FreeBSD aarch64 (ARM64) support #218

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 1 commit into from
Jun 16, 2021

Conversation

siepkes
Copy link
Contributor

@siepkes siepkes commented Jun 13, 2021

Tested on a Raspberry Pi 4.

Comment on lines 189 to 190
if(OSUtil.isARM()) {
if(OSUtil.is64Bit()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could just be

Suggested change
if(OSUtil.isARM()) {
if(OSUtil.is64Bit()) {
if(OSUtil.isARM() && OSUtil.is64Bit()) {

Copy link
Contributor

@MrDOS MrDOS 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 for your contribution! This looks good. As noted below, can you confirm that the -m64 flag is strictly necessary? Then I'm happy to merge.

My biggest reservation is that native libraries for all of the other supported platforms can be cross-compiled, and there's no off-the-shelf cross-compiler to target FreeBSD arm64 from Linux amd64. However, the actual build process integrates cleanly with the others, and it looks possible to extend the current FreeBSD cross-compilation environment to support arm64, so I'm happy to accept this as un-CI'd for now with the expectation that I'll be able to add it to CI later.

Comment on lines 243 to 244
freebsdarm64v8: export CFLAGS += -I$(JAVA_HOME)/include/freebsd -m64
freebsdarm64v8: export LDFLAGS += -m64
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the -m64 flag is AMD64-specific, and I suspect it's ignored outright when building for AArch64. Have you tried without the flag to confirm it's necessary, or if not, can you please do so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your right! I missed that one. I have removed the flag and verified the resulting binary still works.

@siepkes siepkes force-pushed the freebsd-aarch64 branch 2 times, most recently from 96434ac to c7affa3 Compare June 13, 2021 18:35
@siepkes
Copy link
Contributor Author

siepkes commented Jun 13, 2021

Thanks for the prompt review! I've addressed your feedback. On closer inspection I also noticed I needed to add the variant in the makefile so the resulting binary has the correct name.

You might already know about it but there is also the Maven NAR plugin to aid in dealing with native code.

@siepkes siepkes requested a review from MrDOS June 14, 2021 08:34
@@ -239,6 +239,13 @@ freebsd64: export platform := freebsd/x86_64
freebsd64:
$(MAKE) -f natives.mk

# Requires a FreeBSD host running on aarch64 (arm64) architecture.
freebsdarm64v8: export CFLAGS += -I$(JAVA_HOME)/include/freebsd -m64
Copy link
Contributor

Choose a reason for hiding this comment

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

You've removed the -m64 linker flag; did you try without the compiler one, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I totally missed the compiler flag. I either need more caffeine or more vacation it seems ;-)

@siepkes
Copy link
Contributor Author

siepkes commented Jun 15, 2021

Alright, took some tries but I think we've got there! Thanks for your patience!

Building with the -m64 compiler flag removed yields exactly the same binary (I compared the hashes).

Copy link
Contributor

@MrDOS MrDOS 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 for being willing to iterate on this. (Thank you also for keeping a neat and tidy squashed history.) I'm quite happy with this now, and glad to merge it.

@MrDOS MrDOS merged commit 7aa21d1 into NeuronRobotics:master Jun 16, 2021
@MrDOS MrDOS added this to the v5.3.0 milestone Aug 29, 2023
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.

2 participants