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

Implemented Lua C API and LuaJIT Wrappers for CoolProp #440

Merged
merged 7 commits into from Feb 1, 2015

Conversation

bungle
Copy link
Contributor

@bungle bungle commented Jan 29, 2015

Okay, here is the first version where I implemented both Lua C API, and LuaJIT FFI wrappers. The Makefile is rudimentary, and the README.rst is currently just a placeholder. I have tested these on my Mac OSX 10.9.5, but I have not yet checked other platforms. Please give a feedback, and suggestions on these to get merged with the upstream. I had never earlier done any Lua C API bindings (only LuaJIT FFIs), so it would be good if someone more experienced with Lua C API could look at these.

@jowr
Copy link
Member

jowr commented Jan 29, 2015

I install LuaJIT on my Debian system and test it here. Hang on.

@bungle
Copy link
Contributor Author

bungle commented Jan 29, 2015

Check also that PUC-Lua wrapper works too on your Debian system.

@jowr
Copy link
Member

jowr commented Jan 29, 2015

I guess you have to help a little bit, here is the first error:

cc -std=c99 -pedantic-errors -fpic -I/usr/include/luajit-2.0    -g -O2 -Wall -Wextra -Wswitch-enum -Wwrite-strings -Wshadow -c -o coolprop/capi.o coolprop/capi.c 
coolprop/capi.c: In function ‘luaopen_coolprop_capi’: 
coolprop/capi.c:166:3: error: implicit declaration of function ‘luaL_newlib’ [-Wimplicit-function-declaration] 
    luaL_newlib(L,funcs);
    ^ 
lualib.mk:71: recipe for target 'coolprop/capi.o' failed 
make: *** [coolprop/capi.o] Error 1 

@jowr
Copy link
Member

jowr commented Jan 29, 2015

I tried adding the -Wimplicit-function-declaration to the compiler options, but that did not help.

@bungle
Copy link
Contributor Author

bungle commented Jan 29, 2015

The capi.c needs only be build when using PUC Lua, you can go ahead and just run luajit examples.lua. It will use FFI bindings then. There may be differences in header files, for example there is this kind of check in lua-snappy: https://github.com/forhappy/lua-snappy/blob/master/lua-snappy.cc#L91-L95 (so i think luaL_newlib might be missing from LuaJIT (and you have to use luaL_register instead).

@jowr
Copy link
Member

jowr commented Jan 29, 2015

Sorry, but #436 has to be solved first. I would like to update my installed library...

@craigbarnes
Copy link

Please give a feedback, and suggestions

I'd appreciate it if you updated lualib.mk with the license header I just added. 😉

@craigbarnes
Copy link

You may also find this useful for the luaL_newlib issue.

@bungle
Copy link
Contributor Author

bungle commented Jan 29, 2015

Thanks @craigbarnes, I added the license information (thanks for that great config), and also added detection of luaL_newlib.

@jowr
Copy link
Member

jowr commented Jan 29, 2015

Compilation and luajit example.lua seems to work. How can I test the FFI version?

@jowr
Copy link
Member

jowr commented Jan 29, 2015

... or the C version if the example uses the FFI.

@bungle
Copy link
Contributor Author

bungle commented Jan 29, 2015

It will (the example and the coolprop.lua) automatically select FFI version of the binding (ffi.lua) if you run (local cp = require "coolprop") it with LuaJIT (and have not turned JIT off, it's on by default). If you are not using LuaJIT it will automatically select Lua C API version (the capi.c wrapper). LuaJIT doesn't need any compiled wrapper, only that the library is in lib search path (the normal CoolProp lib, not the wrapper). Lua modules are cached, so this check is only done the first time the module is required.

@bungle
Copy link
Contributor Author

bungle commented Jan 29, 2015

I will add more documentation to README.rst, hopefully I have some time for it tomorrow.

@jowr
Copy link
Member

jowr commented Jan 29, 2015

OK. Just let me know if you need someone to do more testing.

@bungle
Copy link
Contributor Author

bungle commented Jan 29, 2015

I will shape this a little bit more (maybe add it to LuaRocks as well). I will let you know when I feel that this is ready to be merged. At its current shape it doesn't look too bad when compared to others in the wrappers folder. So I think we are quite close to integrate it (I had some problems with get_mixture_binary_pair_data (symbol not found) so I had to comment it on capi.c version). Of course more testing, and with different platforms is always something that is really valuable. Thanks @jowr for testing this on your Debian system.

@ibell
Copy link
Contributor

ibell commented Jan 30, 2015

How about windows? I guess the makefile might not work, but otherwise
should it be able to be compiled on windows?

On Thu, Jan 29, 2015 at 1:38 PM, Aapo Talvensaari notifications@github.com
wrote:

I will shape this a little bit more (maybe add it to LuaRocks as well). I
will let you know when I feel that this is ready to be merged. At its
current shape it doesn't look too bad when compared to others in the
wrappers folder. So I think we are quite close to integrate it (I had some
problems with get_mixture_binary_pair_data (symbol not found) so I had to
comment it on capi.c version). Of course more testing, and with different
platforms is always something that it really valuable. Thanks @jowr
https://github.com/jowr for testing this on your Debian system.


Reply to this email directly or view it on GitHub
#440 (comment).

@bungle
Copy link
Contributor Author

bungle commented Jan 30, 2015

Now that the bug #441 is fixed I think that I have finished implementing this. I have also added documentation (see: https://github.com/bungle/CoolProp/tree/master/wrappers/Lua). So I consider this as good-to-be-merged with upstream.

I'm not sure if this needs to be integrated with Cmake build process, but I think that you can figure it out?

Regards
Aapo

@ibell
Copy link
Contributor

ibell commented Jan 30, 2015

Yes, very nice work! Out of curiosity, what are you planning on using
CoolProp for in Lua? I'm aware of Lua, but I've not heard of anyone
wanting to combine Lua and CoolProp

On Fri, Jan 30, 2015 at 5:54 AM, Aapo Talvensaari notifications@github.com
wrote:

Not that the bug #441 #441
is fixed I think that I have finished implementing this. I have also added
documentation (see:
https://github.com/bungle/CoolProp/tree/master/wrappers/Lua). So I
consider this as good-to-be-merged with upstream.

I'm not sure if this needs to be integrated with Cmake build process, but
I think that you can figure it out?

Regards
Aapo


Reply to this email directly or view it on GitHub
#440 (comment).

@bungle
Copy link
Contributor Author

bungle commented Jan 30, 2015

@ibell, well it all started from this message to lua-l (a mailing list):
http://lua-users.org/lists/lua-l/2015-01/msg00695.html

So, I'm not the end user. Although sometimes I wish that I could do some scientific work as well, but I'm a programmer, not a scientist. CoolLib is a really nice library. I learned this and that about thermodynamics when implemented it, and now I know where to look if I ever get to the point where I would need to solve problem related to thermodynamics.

I found your high-level C API and thought that it would be simple to do. So I started with LuaJIT and implemented FFI interface to it in couple of hours. I informed Renaud of that but he said that he's using Lua 5.1. I had never done any Lua C API bindings so I thought this could be a good excercise because the high-level API of CoolProp is dead-simple. I also want to learn more about C and its toolset (make, cmake, gcc etc.).

Why I did it? Because I could, and I wanted to learn. That's all. Great library indeed.

I think Lua is somewhat a hidden gem. More and more people seem to find it though. Google and Facebook are using https://github.com/torch/torch7 and there is also http://www.scilua.org/. Also the Lua was originally developed for the people working on scientific and mathematical topics, more about it here: http://www.lua.org/history.html. So I think that Lua is a perfect match for people working with Thermodynamics. My personal interest in Lua focuses to OpenResty (http://openresty.org/ - Nginx based web development platform / framework).

@jowr
Copy link
Member

jowr commented Jan 31, 2015

Great, let's hope that someone comes along to test the C implementation.

ibell added a commit that referenced this pull request Feb 1, 2015
Implemented Lua C API and LuaJIT Wrappers for CoolProp
@ibell ibell merged commit 4f8bb2a into CoolProp:master Feb 1, 2015
COOLPROP_LDLIBS ?= $(or $(shell $(PKGCONFIG) --libs-only-l CoolProp), -lCoolProp)
COOLPROP_INCDIR ?= $(shell $(PKGCONFIG) --variable=includedir CoolProp)
COOLPROP_LIBDIR ?= $(shell $(PKGCONFIG) --variable=libdir CoolProp)
COOLPROP_HEADER ?= $(or $(COOLPROP_INCDIR)

Choose a reason for hiding this comment

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

This line doesn't seem right...

Copy link
Contributor

Choose a reason for hiding this comment

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

Pull request? Snark helps no one.
On Feb 1, 2015 10:02 PM, "Craig Barnes" notifications@github.com wrote:

In wrappers/Lua/Makefile
#440 (comment):

@@ -0,0 +1,36 @@
+include lualib.mk
+
+COOLPROP_CFLAGS ?= $(shell $(PKGCONFIG) --cflags CoolProp)
+COOLPROP_LDFLAGS ?= $(shell $(PKGCONFIG) --libs-only-L CoolProp)
+COOLPROP_LDLIBS ?= $(or $(shell $(PKGCONFIG) --libs-only-l CoolProp), -lCoolProp)
+COOLPROP_INCDIR ?= $(shell $(PKGCONFIG) --variable=includedir CoolProp)
+COOLPROP_LIBDIR ?= $(shell $(PKGCONFIG) --variable=libdir CoolProp)
+COOLPROP_HEADER ?= $(or $(COOLPROP_INCDIR)

This line seems wrong in at least 3 ways.


Reply to this email directly or view it on GitHub
https://github.com/CoolProp/CoolProp/pull/440/files#r23905306.

Choose a reason for hiding this comment

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

Yes, point taken. I edited my comment shortly after posting it. The line in question isn't actually used, so the fix would just be to remove it.

@bungle
Copy link
Contributor Author

bungle commented Feb 2, 2015

I added a pull request to remove unneeded lines from makefile:
#445

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.

None yet

4 participants