Makefile / library path modifications to make M1 building work (koreader/koreader#8797) #1570

ptrm · 2023-01-02T20:38:06Z

These are mostly changes mentioned in issue koreader/koreader#8797 implemented. So far koreader builds and shows the emulator.

I used both /usr/local/opt and /opt/homebrew/ Homebrew prefixes, as the Apple Silicon Homebrew path has been only recently established to /opt/homebrew (https://apple.stackexchange.com/a/410829), though some users may have older brew installation with /usr/local/opt prefix (as opposed to /usr/local/ without opt for Intel brew).

This change is

Frenzie · 2023-01-02T20:51:09Z

ffi/SDL2_0.lua

+    "/usr/local/opt/lib/libSDL2.dylib",
+    "/opt/homebrew/lib/libSDL2.dylib",


This should go lower since SDL2 is the default. See http://luajit.org/ext_ffi_api.html#ffi_load.

On POSIX systems, if the name contains no dot, the extension .so is appended. Also, the lib prefix is prepended if necessary. So ffi.load("z") looks for "libz.so" in the default shared library search path.

On Windows systems, if the name contains no dot, the extension .dll is appended. So ffi.load("ws2_32") looks for "ws2_32.dll" in the default DLL search path.

Frenzie · 2023-01-02T20:51:54Z

ffi/util.lua

@@ -619,7 +619,7 @@ function util.haveSDL2()
    if haveSDL2 == nil then
        local candidates
        if jit.os == "OSX" then
-            candidates = {"libs/libSDL2.dylib", "SDL2"}
+            candidates = {"/opt/homebrew/lib/libSDL2.dylib", "SDL2"}


It looks like this should be added, not replaced.

Frenzie · 2023-01-02T20:52:49Z

thirdparty/glib/CMakeLists.txt

+    set(CFLAGS "-I/usr/local/opt/gettext/include -I/opt/homebrew/include ${CFLAGS}")
+    set(LDFLAGS "${LDFLAGS} -L/usr/local/opt/gettext/lib -L/opt/homebrew/lib -L${LIBICONV_DIR}/lib -L${ZLIB_DIR}/lib -L${LIBFFI_DIR}/.libs -L${BINARY_DIR}/gmodule/.libs")


I don't like this hardcoding.

You mean /opt/homebrew/lib should get a variable and become ${HOMEBREW_PREFIX}/lib?

Also is it a good idea to check for brew binary in Makefile.defs in case of Darwin host and set the HOMEBREW_PREFIX from $(shell brew --prefix)? And could this be passed to the lua files somehow? This way custom prefixes set during brew installation could be covered, though probably people who bother to set them know how to fix their builds later on ;)

Definitely that, but I'd also prefer to be able to plug in e.g. Nix or gentoo-prefix with little effort if reasonably possible. HOMEBREW_PREFIX might allow for that but then the name (at least further down at this point) should really just be something like MAC_LIBRARY_PREFIX. But you could always write MAC_LIBRARY_PREFIX := HOMEBREW_PREFIX up above in defs or something so it's self-documenting without comments.

IIRC, there's a runtime homebrew command that will actually spit out the right prefix for the current system ;).

Yup, there's brew --prefix. I'll try to unhardcode the paths around evening CET.

What about the paths in lua part, can / should they somehow be based on brew --prefix as well?

But SIP probably means that if you exec from an SIP/hardened app, you're essentially fucked? Yay, Apple.

So, yeah, seems like a giant "fuck you, FOSS", from Apple. c.f., Homebrew/brew#13481 for more context (notably the hilarious bit about the consequences of stuff like env being hardened, it's fantastic).

TL;DR: Hardcoded search paths it is. >_<".

I think you still meant to base these ones on brew --prefix or some such as well? :-)

Ok, just pushed a commit with variables instead of hardcoded brew path ;)

Frenzie

Thanks! Looks very promising.

ffi/util.lua

NiLuJe

(goddamnit GH review UI sucks)

NiLuJe · 2023-01-06T14:31:00Z

Makefile.defs

@@ -189,8 +190,14 @@ endif

 ifneq (,$(filter $(UNAME), Darwin))
 	export DARWINHOST=1
+	ifneq (,$(filter $(UNAME_ARCH), arm64))
+		export DARWINM1HOST=1


nit: arm64, m2 chips already exist ;p

True, not without a reason being called Apple Sillicon ;> Will fix

ffi/util.lua

NiLuJe

Reviewed 3 of 5 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @Frenzie and @ptrm)

Makefile.defs line 11 at r3 (raw file):


UNAME:=$(shell uname -s)
UNAME_ARCH:=$(shell uname -m)

nit: I'd rename that to HOST_ARCH (and I'm mildly intrigued by the fact that we don't already have that stored somewhere?).

EDIT: Oh, yeah, we have it, but we use it for the actual march flag, meh :/. Nevermind that one, then.

Makefile.defs line 194 at r3 (raw file):

	export DARWINHOST=1
	ifneq (,$(filter $(UNAME_ARCH), arm64))
		export DARWINM1HOST=1

nit: DARWIN_AARCH64_HOST (extra columns are free!)

Makefile.defs line 199 at r3 (raw file):

		export DARWIN=1
		ifdef DARWINM1HOST
			export DARWIN_M1=1

nit: DARWIN_AARCH64

Makefile.defs line 511 at r3 (raw file):


ifdef DARWINM1HOST
	HOST_ARCH:=-mcpu=apple-m1

And a comment that M1 is the lowest common denominator for Apple AARCH64 silicon for future archeologists ;).

ffi/SDL2_0.lua line 28 at r3 (raw file):

    "libSDL2-2.0.so",
    "libSDL2-2.0.so.0",
    util.KO_DYLD_PREFIX .. "/lib/libSDL2.dylib",

Mildly mystified why this one doesn't have the libs/libSDL2.dylib variant found in util?

ffi/util.lua line 63 at r3 (raw file):

]]

local getlibprefix = function()

Missing a jit.os == "OSX" branch, so we don't try to run brew everywhere.

ffi/util.lua line 64 at r3 (raw file):

local getlibprefix = function()
    -- Apple M1 homebrew installs libraries outside of default searchpaths,

nit: AArch64 ;)

thirdparty/glib/CMakeLists.txt line 73 at r1 (raw file):

Previously, Frenzie (Frans de Jonge) wrote…

I think you still meant to base these ones on brew --prefix or some such as well? :-)

These are still hard-coded ;).

NiLuJe · 2023-01-08T22:35:28Z

My GH "review" comments may be outdated, I had once again forgotten to hit the big green button.

The Reviewable stuff is current ;).

NiLuJe · 2023-01-09T17:59:20Z

And I just noticed that I mixed aarch64 w/ arm64 in my renaming requests; aarch64 is technically more correct, but arm64 might be easier on the eyes? ;).

…der/koreader#8797)

NiLuJe · 2023-03-23T19:49:56Z

ffi/util.lua

+            end
+    end
+
+    return libprefix;


nit: trailing semicolon

Indeed, better add some more:

Suggested change

return libprefix;

return libprefix;;;;;;;;;;

NiLuJe

LGTM besides that very small nit ;).

ptrm · 2023-03-24T09:04:15Z

Removed the semicolon, I'm blind to them at EOL ;) Also added a comment proposed in Reviewable about apple-m1 being lowest common denominator for Apple silicon chips.

Frenzie · 2023-04-02T20:10:01Z

ffi/util.lua

 require("ffi/posix_h")

 local util = {}

+util.KO_DYLD_PREFIX = getlibprefix()


This shouldn't be called except on OS X, so probably on line 654 below.

Yeah, thanks for the unbreaking commit elsewhere, I didn't consider this might throw an exception ;)

Good thing the CI caught it. ;-)

Regression from koreader#1570.

Regression from #1570.

ptrm changed the title ~~Makefile / library path modifications to make M1 building work (#8797)~~ Makefile / library path modifications to make M1 building work (koreader#8797) Jan 2, 2023

ptrm changed the title ~~Makefile / library path modifications to make M1 building work (koreader#8797)~~ Makefile / library path modifications to make M1 building work (koreader/koreader#8797) Jan 2, 2023

ptrm changed the title ~~Makefile / library path modifications to make M1 building work (koreader/koreader#8797)~~ Makefile / library path modifications to make M1 building work koreader/koreader#8797 Jan 2, 2023

ptrm changed the title ~~Makefile / library path modifications to make M1 building work koreader/koreader#8797~~ Makefile / library path modifications to make M1 building work (koreader/koreader#8797) Jan 2, 2023

ptrm force-pushed the apple-m1-build branch from 92b1f7a to 90f641c Compare January 2, 2023 20:43

Frenzie reviewed Jan 2, 2023

View reviewed changes

ptrm commented Jan 6, 2023

View reviewed changes

ffi/util.lua Show resolved Hide resolved

ptrm requested review from Frenzie and NiLuJe and removed request for Frenzie January 8, 2023 18:17

NiLuJe reviewed Jan 8, 2023

View reviewed changes

NiLuJe requested changes Jan 8, 2023

View reviewed changes

NiLuJe mentioned this pull request Jan 27, 2023

FR: AppImage Aarch64 build koreader/koreader#10055

Open

ptrm force-pushed the apple-m1-build branch from 8b7a8a9 to c8f36e3 Compare March 22, 2023 16:59

ptrm added 3 commits March 22, 2023 18:24

Makefile / library path modifications to make M1 building work (korea…

2aa7ad9

…der/koreader#8797)

fix ffi libsdl2 candidate order and paths

c02e773

Unhardcode Apple M1 brew paths for SDL2

93fefd4

ptrm force-pushed the apple-m1-build branch from c8f36e3 to 93fefd4 Compare March 22, 2023 17:26

CR fixes

590fbc6

ptrm force-pushed the apple-m1-build branch from 8e6502c to 590fbc6 Compare March 22, 2023 18:09

Pass brew prefix through a variable in makefiles

ababb47

ptrm requested a review from NiLuJe March 22, 2023 18:35

Frenzie approved these changes Mar 23, 2023

View reviewed changes

NiLuJe reviewed Mar 23, 2023

View reviewed changes

NiLuJe requested changes Mar 23, 2023

View reviewed changes

Nits and comments

0db16e1

ptrm requested a review from NiLuJe March 24, 2023 09:12

NiLuJe approved these changes Mar 24, 2023

View reviewed changes

Frenzie merged commit 30bf21c into koreader:master Mar 24, 2023
1 of 2 checks passed

This was referenced Apr 1, 2023

Building on macOS 12.x or 13 on Rosetta results in a crashy libcrengine koreader/koreader#9689

Open

Bump base for M1 building koreader/koreader#10291

Merged

Frenzie reviewed Apr 2, 2023

View reviewed changes

Frenzie added a commit to Frenzie/koreader-base that referenced this pull request Apr 2, 2023

Fix brew not found on non-Mac platforms

a89be62

Regression from koreader#1570.

Frenzie mentioned this pull request Apr 2, 2023

Fix brew not found on non-Mac platforms #1593

Merged

Frenzie added a commit that referenced this pull request Apr 2, 2023

Fix brew not found on non-Mac platforms (#1593)

52aeac3

Regression from #1570.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile / library path modifications to make M1 building work (koreader/koreader#8797) #1570

Makefile / library path modifications to make M1 building work (koreader/koreader#8797) #1570

ptrm commented Jan 2, 2023 •

edited

Frenzie Jan 2, 2023

Frenzie Jan 2, 2023

Frenzie Jan 2, 2023

ptrm Jan 2, 2023

Frenzie Jan 2, 2023

NiLuJe Jan 2, 2023

ptrm Jan 3, 2023 •

edited

NiLuJe Jan 3, 2023 •

edited

NiLuJe Jan 3, 2023 •

edited

Frenzie Jan 8, 2023

NiLuJe Jan 8, 2023

ptrm Mar 22, 2023

Frenzie left a comment

NiLuJe left a comment

NiLuJe Jan 6, 2023

ptrm Jan 9, 2023

NiLuJe left a comment

NiLuJe commented Jan 8, 2023

NiLuJe commented Jan 9, 2023

NiLuJe Mar 23, 2023 •

edited

Frenzie Mar 23, 2023

NiLuJe left a comment

ptrm commented Mar 24, 2023

Frenzie Apr 2, 2023

ptrm Apr 6, 2023

Frenzie Apr 7, 2023

		"/usr/local/opt/lib/libSDL2.dylib",
		"/opt/homebrew/lib/libSDL2.dylib",

		set(CFLAGS "-I/usr/local/opt/gettext/include -I/opt/homebrew/include ${CFLAGS}")
		set(LDFLAGS "${LDFLAGS} -L/usr/local/opt/gettext/lib -L/opt/homebrew/lib -L${LIBICONV_DIR}/lib -L${ZLIB_DIR}/lib -L${LIBFFI_DIR}/.libs -L${BINARY_DIR}/gmodule/.libs")

Makefile / library path modifications to make M1 building work (koreader/koreader#8797) #1570

Makefile / library path modifications to make M1 building work (koreader/koreader#8797) #1570

Conversation

ptrm commented Jan 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptrm Jan 3, 2023 • edited

Choose a reason for hiding this comment

NiLuJe Jan 3, 2023 • edited

Choose a reason for hiding this comment

NiLuJe Jan 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frenzie left a comment

Choose a reason for hiding this comment

NiLuJe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe left a comment

Choose a reason for hiding this comment

NiLuJe commented Jan 8, 2023

NiLuJe commented Jan 9, 2023

NiLuJe Mar 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe left a comment

Choose a reason for hiding this comment

ptrm commented Mar 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptrm commented Jan 2, 2023 •

edited

ptrm Jan 3, 2023 •

edited

NiLuJe Jan 3, 2023 •

edited

NiLuJe Jan 3, 2023 •

edited

NiLuJe Mar 23, 2023 •

edited