Makefile interoperability updates #19
Conversation
Removed unnecessary arch detection which broke builds on other platforms e.g AARCH64 Removed hardcoded gcc compiler setting as its not required, clang works just as fine
| CFLAGS=-fPIC -O3 -fomit-frame-pointer -fno-exceptions -Wall -std=c99 -pedantic | ||
| ifeq (${PROC},x86_64) |
There was a problem hiding this comment.
this may be needed if 32bit code is compiled on x86_64 machines, so I'm not confortable in turning it off.
There was a problem hiding this comment.
It breaks compilation on 64BIT arm architectures.
If people really need that they can specify it in the make commandline.
make CFLAGS=-m32
There was a problem hiding this comment.
I know at least some users that use it in automated build systems, so this is not really a good option. (don't like to expose CFLAGS to the normal user)
The best way is to adapt the Makefile to do proper discovery, maybe using uname -i ?
I don't have an x86_64 arm to test right now (maybe later)
There was a problem hiding this comment.
If you want to separate between 32 and 64 we could use getconf LONG_BIT this should return 64 or 32 based on the host OS architecture.
But lets think about what we are doing to the CFLAGS here, basically we are only specifying options for 64bit systems that are already set if you use a 64bit compiler. For 32bit systems we force the arch to i686 but why do we do that ? Whats the reasoning behind this ?
There was a problem hiding this comment.
well, agreed about the i686 arch (basically I never thought about arm or others).
the important part here is the m32 flag that generates 32bit code.
There was a problem hiding this comment.
Sounds about right, as far as i know you cant really crosscompile on arm anyway.
x86_64 and x86 are indeed the most prominent example of natively working crosscompilation.
There was a problem hiding this comment.
Okay was busy last week will give this a shot in a few days/weeks.
There was a problem hiding this comment.
And i never did, sorry for that!
I dont know whats the best way to do this
There was a problem hiding this comment.
Actually, i think this entire thing is not needed and can be dropped.
Reasons:
- GCC automatically defaults to 32bit if the installed compiler is 32bit (like it is by default on a 32bit machine with an x64 cpu)
- It only causes problem for other architectures.
- 32bit linux is going to die soon enough Ubuntu 17.10 already dropped support.
- I could not find a valid use-case for this code (tried a 32bit vm with an x64 cpu)
I suggest accepting this merge request in its current and original state.
If you can provide me with a solid proof that the code is actually needed i will figure out additional options.
There was a problem hiding this comment.
ok, granted :) going to merge as is.
Removed unnecessary arch detection which broke builds on other platforms e.g AARCH64
Removed hardcoded gcc compiler setting as its not required, clang works just as fine
I tried to find a reason for this but found none should be safe on all systems.
(Tested on aarch64, x86_64)