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

link iconv library on macOS #5

Closed
wants to merge 2 commits into from

Conversation

metaspatial
Copy link

current rockspec creates an .so not linked with libiconv which whilst it builds, cannot be used; have added library definition to the rockspec for both cygwin and macosx (possibly other platforms e.g. freebsd may also need; at least some flavours of linux don't need it)

@richardipsum
Copy link

I am also experiencing issues on Darwin, I've managed to get iconv to compiler but the tests fail:

 sh-3.2# make test
 lua test_iconv.lua
 lua: error loading module 'iconv' from file './iconv.so':
         dlopen(./iconv.so, 6): Symbol not found: _iconv
   Referenced from: ./iconv.so
   Expected in: flat namespace
  in ./iconv.so
 stack traceback:
         [C]: in ?
         [C]: in function 'require'
         test_iconv.lua:3: in main chunk
         [C]: in ?
 make: *** [test] Error 1

@alerque
Copy link
Member

alerque commented Oct 17, 2023

I presume this is related to #6, and may even fix it. Is anybody with a macOS machine around willing to test this and se where we are at?

@Tieske
Copy link
Member

Tieske commented Oct 17, 2023

a quick test

on master:

10:lua-iconv thijs$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
10:lua-iconv thijs$ luarocks make

lua-iconv 7.0.0-4 depends on lua >= 5.1 (5.1-1 provided by VM)
env MACOSX_DEPLOYMENT_TARGET=11.0 gcc -O2 -fPIC -I/usr/local/Cellar/[email protected]/1.21.4.1/openresty/luajit/include/luajit-2.1 -c luaiconv.c -o luaiconv.o -I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include
luaiconv.c:47:10: warning: 'luaL_newlib' macro redefined [-Wmacro-redefined]
 #define luaL_newlib(L, f)  { lua_newtable(L); luaL_register(L, NULL, f); }
         ^
/usr/local/Cellar/[email protected]/1.21.4.1/openresty/luajit/include/luajit-2.1/lauxlib.h:125:9: note: previous definition is here
#define luaL_newlib(L, l)       (luaL_newlibtable(L, l), luaL_setfuncs(L, l, 0))
        ^
1 warning generated.
env MACOSX_DEPLOYMENT_TARGET=11.0 gcc  -bundle -undefined dynamic_lookup -all_load -o iconv.so luaiconv.o -L/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr
lua-iconv 7.0.0-4 is now installed in /usr/local/Cellar/[email protected]/1.21.4.1/luarocks (license: MIT/X11)

10:lua-iconv thijs$ lua test_iconv.lua
lua: error loading module 'iconv' from file './iconv.so':
	dlopen(./iconv.so, 0x0006): symbol not found in flat namespace '_iconv'
stack traceback:
	[C]: at 0x01091eb4ff
	[C]: in function 'require'
	test_iconv.lua:3: in main chunk
	[C]: at 0x010919960c

This PR:

10:lua-iconv thijs$ gh pr checkout 5
Switched to branch 'metaspatial/master'
10:lua-iconv thijs$ luarocks make

lua-iconv 7-1 depends on lua >= 5.1 (5.1-1 provided by VM)
env MACOSX_DEPLOYMENT_TARGET=11.0 gcc -O2 -fPIC -I/usr/local/Cellar/[email protected]/1.21.4.1/openresty/luajit/include/luajit-2.1 -c luaiconv.c -o luaiconv.o -I/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include
luaiconv.c:47:10: warning: 'luaL_newlib' macro redefined [-Wmacro-redefined]
 #define luaL_newlib(L, f)  { lua_newtable(L); luaL_register(L, NULL, f); }
         ^
/usr/local/Cellar/[email protected]/1.21.4.1/openresty/luajit/include/luajit-2.1/lauxlib.h:125:9: note: previous definition is here
#define luaL_newlib(L, l)       (luaL_newlibtable(L, l), luaL_setfuncs(L, l, 0))
        ^
1 warning generated.
env MACOSX_DEPLOYMENT_TARGET=11.0 gcc  -bundle -undefined dynamic_lookup -all_load -o iconv.so luaiconv.o -L/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr -liconv
lua-iconv 7-1 is now installed in /usr/local/Cellar/[email protected]/1.21.4.1/luarocks (license: MIT/X11)

Checking stability of dependencies in the absence of
lua-iconv 7.0.0-4...

Removing lua-iconv 7.0.0-4...
Removal successful.
10:lua-iconv thijs$ lua test_iconv.lua

-- Testing conversion from iso-8859-1 to utf-8
Ao longe, ao luar
No rio uma vela
Serena a passar,
Que é que me revela

Não sei, mas mue ser
Tornou-se-me estranho,
E eu sonho sem ver
Os sonhos que tenho.

Que angústia me enlaça?
Que amor não se explica?
É a vela que passa
Na noite que fica

    -- Fernando Pessoa


-- Testing conversion from utf8 to utf-8
Ao longe, ao luar
No rio uma vela
Serena a passar,
Que é que me revela

Não sei, mas mue ser
Tornou-se-me estranho,
E eu sonho sem ver
Os sonhos que tenho.

Que angústia me enlaça?
Que amor não se explica?
É a vela que passa
Na noite que fica

    -- Fernando Pessoa


-- Testing conversion from utf16 to utf-8
lua: test_iconv.lua:82: Failed to create a converter object.
stack traceback:
	[C]: in function 'assert'
	test_iconv.lua:82: in function 'check_one'
	test_iconv.lua:99: in main chunk
	[C]: at 0x010b51c60c
10:lua-iconv thijs$

@alerque
Copy link
Member

alerque commented Oct 17, 2023

Thanks. That's kind of interesting but I think the main difference between master and this PR is just which rockspec is considered current. We don't even have a dev rockspec in here yet to realistically test from Git HEAD!

I guess the short story is macOS is still broken and it's waiting for somebody to fix it.

@Tieske
Copy link
Member

Tieske commented Oct 17, 2023

created #12 which adds a dev rockspec that includes this fix for mac.

The error above was not a problem with the lib, but an expected error since the requested conversion apparently is not supported. The PR #12 also adds an errorcode as return value to the new/open function.

That would allow us to publish a new rock, as 7.1, with that fix, and a new version number to overcome the LuaRocks issue with the numbers.

@Tieske
Copy link
Member

Tieske commented Oct 17, 2023

also means we shouldn't merge this PR, since it modifies a rockspec, which shouldn't happen, we should only create new ones (unless its a dev rockspec)

@Tieske
Copy link
Member

Tieske commented Oct 24, 2023

can anyone review the #12 PR, so we can close this?

@Tieske
Copy link
Member

Tieske commented Oct 30, 2023

#12 was merged, so we can close this now.

@Tieske Tieske closed this Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants