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

CMakeLists: Introduce AMD64 (to fix x86_64/--native builds) #670

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

flokli
Copy link

@flokli flokli commented Dec 27, 2020

CMakeLists.txt currently assume everything not ARM64 to be ARM. When
compiling on x86_64-linux, this pulls in some binaries that are arm32
only.

Some tools make some sense to have accessible even on non-ARM
workstations. For example, dtmerge can be used to merge .dtb files
with overlays on a x86_64 machine (while mounting an sdcard for
example).

Simply by adding this conditionals, we're able to get a dtmerge binary
that runs on x86_64-linux.

@6by9
Copy link
Contributor

6by9 commented Dec 27, 2020

Have you actually tested this on a Pi?

ARM64 is a define passed in from the buildme script - https://github.com/raspberrypi/userland/blob/master/buildme#L24

I don't see you having added a similar flag for ARM, therefore it is never going to be defined and you'll never build any of the libraries under you if (ARM) conditionals.

MMAL should build for x86_64 - it was one of the test platforms for the framework, and used to be used under Windows. There may be some 32bit assumptions/checks in some of the structs. Those should be resolved when we unrevert the 64bit support.
Yes the Khronos libraries are going to be borked where they include ARM assembly.

The ./buildme --native option does appear to be broken (if it has ever really worked). Fixing that to choose a sensible list of applications to build would be useful.

@flokli
Copy link
Author

flokli commented Dec 27, 2020

I wasn't using the ./buildme script - the build runs in some sandboxed environment without access to sudo (and outputs need to go to a custom location), so it won't work.
I was essentially invoking cmake directly, and passed in ARM(64)=1 to it.

I updated a new version, which now includes the necessary changes in the ./buildme script. Also, changed ARM to be ARM32, to make things more clear this is only ON on the armv6/7 architectures.

MMAL should build for x86_64 - it was one of the test platforms for the framework, and used to be used under Windows. There may be some 32bit assumptions/checks in some of the structs. Those should be resolved when we unrevert the 64bit support.

Yes, this indeed didn't build for me. For now, I made it ARM32 only. Also, had to disable some targets that link against it.

I successfully built this natively for x86_64-linux and aarch64-linux, and cross-compiled from x86_64-linux to armv6l-linux and aarch64-linux. I couldn't test on armv6/7l natively (as I don't have access to the hardware).

@6by9
Copy link
Contributor

6by9 commented Dec 27, 2020

TBH I'd prefer to go the other way - default to ARM and add a NATIVE flag.

This repo is specifically targetted at the Pi, therefore that is the absolute priority, anything else is the odd-ball.
There will also be forked repos that build via cmake and rely on the current behaviour, so minimising the behavioural changes to those is important.

@flokli
Copy link
Author

flokli commented Dec 27, 2020

How would the semantics of NATIVE be? No cross-compilation involved?

If we build this on a raspberry pi or on x86-64-linux, that's both no cross-compilation, it's just that less of the tools can build in the x86-64-linux case.

@6by9
Copy link
Contributor

6by9 commented Dec 27, 2020

i was following the ```--native`` option from buildme (https://github.com/raspberrypi/userland/blob/master/buildme#L20). ARM/ARM64 has already been filtered off, so it's build for the host vs cross-compile on non-ARM platforms.

Perhaps better to add it as an X86 option to avoid confusion. At least it makes it explicit which platform it's being targetted at.

@flokli
Copy link
Author

flokli commented Dec 27, 2020

Yeah, X86 might be more understandable, but then, we'd need to distinguish between I686 and AMD64 (and every other host cpu we might build this for).

It's really a pity CMake has no standardized concept to get this information, like meson or autotools have.

I'll change this to AMD64, because X86 could also mean i686, alongside with some comments.

@6by9
Copy link
Contributor

6by9 commented Dec 27, 2020

If you'd make the --native option of buildme set the new flag it'd be appreciated.

CMakeLists.txt currently assumes everything not ARM64 to be ARM32. When
compiling on x86_64-linux, this tries to build some binaries that are
arm(32) only.

Some tools make some sense to use even on non-ARM workstations. For
example, `dtmerge` can be used to merge .dtb files with overlays on a
x86_64 machine (while mounting an sdcard for example).

This introduces a "AMD64" cmake flag, that's set in the `buildme` script
only when compiling for x86_64.

Signed-off-by: Florian Klink <[email protected]>
@flokli
Copy link
Author

flokli commented Dec 27, 2020

I updated the patch, now introducing the AMD64 flag (and some documentation in /CMakeLists.txt on the reasoning.

Successfully natively compiled on x86_64, aarch64, and crosscompiled from x86_64 to armv6,7,aarch64.

@flokli flokli changed the title CMakeLists: don't assume !ARM64 is ARM CMakeLists: Introduce AMD64 (to fix x86_64/--native builds) Dec 27, 2020
@mweinelt
Copy link

mweinelt commented Apr 5, 2021

We are actively relying on this patch downstream in NixOS and we'de be happy if it could be merged upstream.

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.

3 participants