Makefile / library path modifications to make M1 building work (koreader/koreader#8797) by ptrm · Pull Request #1570 · koreader/koreader-base

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

+    "/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

        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

+    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.

NiLuJe

(goddamnit GH review UI sucks)

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

 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

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

+            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


 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

Comment thread ffi/util.lua

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

Closed

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

This was referenced Apr 1, 2023

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

Closed

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.

		"/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")

Uh oh!

Conversation

ptrm commented Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptrm Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NiLuJe Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NiLuJe Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Frenzie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NiLuJe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NiLuJe left a comment

Choose a reason for hiding this comment

Uh oh!

NiLuJe commented Jan 8, 2023

Uh oh!

NiLuJe commented Jan 9, 2023

Uh oh!

NiLuJe Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NiLuJe left a comment

Choose a reason for hiding this comment

Uh oh!

ptrm commented Mar 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ptrm commented Jan 2, 2023 •

edited

Loading

ptrm Jan 3, 2023 •

edited

Loading

NiLuJe Jan 3, 2023 •

edited

Loading

NiLuJe Jan 3, 2023 •

edited

Loading

NiLuJe Mar 23, 2023 •

edited

Loading