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

Fix build & install on osx #31

Merged
merged 6 commits into from
Jun 26, 2022
Merged

Fix build & install on osx #31

merged 6 commits into from
Jun 26, 2022

Conversation

hjdivad
Copy link
Contributor

@hjdivad hjdivad commented Jun 11, 2022

Fix building on osx and add install instructions for non-linux platforms.

uname -a
# Darwin hostname 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:22 PDT 2 022; root:xnu-8020.121.3~4/RELEASE_X86_64 x86_64

On master

make
# ...

# gcc -o lua  lua.o liblua.a -lm -Wl,-E -ldl -lreadline -lhistory -lncurses
# ld: unknown option: -E
# clang: error: linker command failed with exit code 1 (use -v to see invocation)

This error installing lua is fixed by specifying a LUA_TARGET of macosx instead of linux. In general, several LUA_TARGETs can be specified. I've added them to README.md. The source of truth is make from lua itself, e.g.

cd .deps/5.1.5/src/lua && make
# Please do
#    make PLATFORM
# where PLATFORM is one of these:
#    aix ansi bsd freebsd generic linux macosx mingw posix solaris
# See INSTALL for complete instructions.

After building lua successfully there are still two build errors for libmpack:

lmpack.c:721:3: error: implicitly declaring library function 'snprintf' with ty
pe 'int (char *, unsigned long, const char *, ...)' [-Werror,-Wimplicit-functio
n-declaration]

and a few occurrences of:

./mpack-src/src/conv.h:7:9: error: 'long long' is an extension when C99 mode is
 not enabled [-Werror,-Wlong-long]
typedef long long mpack_sintmax_t;

The first is fixed by #30 (this PR includes @nashidau's commit) and the second is fixed by adding -std=c99 to CFLAGS.

With these changes I'm able to build and install, which satisfies the dep for neovim/lua-client.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated
# Lua-related configuration
MPACK_LUA_VERSION ?= 5.1.5
MPACK_LUA_VERSION_NOPATCH = $(basename $(MPACK_LUA_VERSION))
LUA_URL ?= https://lua.org/ftp/lua-$(MPACK_LUA_VERSION).tar.gz
LUAROCKS_URL ?= https://github.com/keplerproject/luarocks/archive/v2.2.0.tar.gz
ifeq ($(PLATFORM), Darwin)
LUA_TARGET ?= macosx
else
LUA_TARGET ?= linux
Copy link
Collaborator

Choose a reason for hiding this comment

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

looking again, it seems like LUA_TARGET was intended to be set by the caller or else default to "linux".

So adding the uname call seems questionable, because it would presumably fail on Windows. Why not pass LUA_TARGET from the caller (lua-client in your case specifically)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justinmk good point about windows, I should drop the use of uname.

I'm not following your point about the caller being lua-client -- LUA_TARGET here as I understand it is just about building the local lua in .deps for a local build. I'm not seeing how it affects downstream libraries.


An alternative to trying to have make work out of the box on osx is to update README with build instructions for osx users that amount to LUA_TARGET=macosx make. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

An alternative to trying to have make work out of the box on osx is to update README with build instructions for osx users that amount to LUA_TARGET=macosx make. WDYT?

Yes. And if there's a LUA_TARGET for Windows that also helps :)

not following your point about the caller being lua-client -- LUA_TARGET here as I understand it is just about building the local lua in .deps for a local build.

oops, right. I didn't inspect where it was used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justinmk I've updated the PR so that it fixes the build error and only provides instructions on building lua for non-linux platforms, including windows.

LMK if you'd like to see any other tweaks before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justinmk I've similarly updated neovim/lua-client#55

@hjdivad hjdivad force-pushed the hjdivad/osx branch 2 times, most recently from b5cd85b to 5789ac7 Compare June 15, 2022 14:42
justinmk pushed a commit to neovim/lua-client that referenced this pull request Jun 15, 2022
Fix building on osx and add install instructions for non-linux platforms.

Trying to build & install on linux leads to three errors:
1. Lua doesn't build
2. libmpack doesn't build
3. neovim/lua-client doesn't build

With this PR and libmpack/libmpack-lua#31 I'm able to build the lua client on osx.

```bash

uname -a
# Darwin hostname 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:22 PDT 2 022; root:xnu-8020.121.3~4/RELEASE_X86_64 x86_64
```

On `master`
```bash
make
# ...

# gcc -o lua  lua.o liblua.a -lm -Wl,-E -ldl -lreadline -lhistory -lncurses
# ld: unknown option: -E
# clang: error: linker command failed with exit code 1 (use -v to see invocation)
```

This error installing lua is fixed by specifying a `LUA_TARGET` of `macosx` instead of `linux`. In general, several `LUA_TARGET`s can be specified. I've added them to `README.md`. The source of truth is `make` from lua itself, e.g.

```bash
cd .deps/5.1.5/src/lua && make
# Please do
#    make PLATFORM
# where PLATFORM is one of these:
#    aix ansi bsd freebsd generic linux macosx mingw posix solaris
# See INSTALL for complete instructions.
```

The next issue is that `libmpack` doesn't build. See [libpack-lua#131](libmpack/libmpack-lua#31). Apply the fix locally and install via:

```bash
cd ~/src/libmpack/libmpack-lua/
~/src/neovim/lua-client/.deps/usr/bin/luarocks make
```

With `libmpack` and lua installed `make` now errors with:

```
cc -g -fPIC -Wall -Wextra -Werror -Wconversion -Wextra -Wstrict-prototypes -ped
antic -o nvim/native.o -c nvim/native.c -I/Users/hjdivad/src/neovim/lua-client/
.deps/usr/include
cc -shared -fPIC nvim/native.o -o nvim/native.so
Undefined symbols for architecture x86_64:
  "_luaL_checkinteger", referenced from:
      _pid_wait in native.o
  "_luaL_register", referenced from:
      _luaopen_nvim_native in native.o
  "_lua_createtable", referenced from:
      _luaopen_nvim_native in native.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
```

This is fixed by adding `-undefined dynamic_lookup` to `LDFLAGS` and not relying on it being the default behaviour.
Copy link
Collaborator

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

README.md Outdated Show resolved Hide resolved
hjdivad and others added 3 commits June 15, 2022 14:50
Specify build instructions for non-linux platforms.
Seems the OS X requires _C99_SOURCE or similar to define snprintf.  Add that define.
This fixes some build issues on osx of the form:
```
./mpack-src/src/conv.h:7:9: error: 'long long' is an extension when C99 mode is
 not enabled [-Werror,-Wlong-long]
typedef long long mpack_sintmax_t;
```
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@justinmk justinmk merged commit 10ed199 into libmpack:master Jun 26, 2022
@hjdivad hjdivad deleted the hjdivad/osx branch June 27, 2022 15:57
This was referenced Jun 27, 2022
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