Allow installing rocks locally - fix openssl include #109
Conversation
/usr/local is not writable on my machine by purpose, so `make dev` fails with permission error. I've changed the Makefile to allow installing rocks in the user's home directory. This is optional, so by default `make dev` will have the same behaviour as now. I'm not super happy with this solution either, imo the right thing would be to create a local tree inside this directory and install the dependencies there instead of polluting the system. That's possible and it works, however it required me to set the lua load paths in several places and wasn't "backward compatible", so I chose to go with this solution for now which should be easier to code-review. Another issue that I encountered is that openssl header files on macOS are not inside `/usr/local`, which mean I couldn't install luasec. To fix this I've added an optional OPENSSL_DIR env variable that can be used to configure luarocks. (This is the suggested procedure on luasec's website). I've also added luasec and luasocket as lua-cassandra dependencies since without them the tests break. There doesn't seem to be a way to mark luasec as optional (since it's only require if you enable ssl) unfortunately.
|
Oh also, |
There was a problem hiding this comment.
I believe if /usr/local isn't writable by default on your machine, that it may be your responsibility to either:
- install the LuaRocks system tree in a writable directory
- install this library and its dependencies in the user tree (via the
--localflag like you did)
To run the tests locally, simply install LuaSocket and LuaSec manually (see the .travis.yml setup for a complete example of a test environment).
|
|
||
| To install the development dependencies locally instead of in `/usr/local`, set | ||
| `FLAGS='--local'`. This will use the tree in the user's home directory. | ||
| You might also need to run `eval $(luarocks path --bin)` to add them to your path. |
There was a problem hiding this comment.
I don't think the README is an appropriate place to school users about LuaRocks. That should be something they deal with on their own...
|
|
||
| ``` | ||
| OPENSSL_DIR=/usr/local/opt/openssl/ FLAGS='--local' make busted | ||
| ``` |
There was a problem hiding this comment.
Ditto, I don't think this is a good place to teach users how to properly install OpenSSL on macOS. Makes for opinionated setups that this library should not concern itself with...
There was a problem hiding this comment.
I can remove the reference to macOS, which actually is not strictly relevant since it looks like this problem also happen on some linux distribution.
I just wanted to let developers know that the Makefile already supports this and they only need to set an env variable to fix it. Same for your next comment on --local.
|
|
||
| .PHONY: install dev busted prove test clean coverage lint doc | ||
|
|
||
| install: | ||
| @luarocks make | ||
| @luarocks make $(PROD_ROCKFILE) |
There was a problem hiding this comment.
Works for me without specifying the rockspec:
$ pwd
lua-cassandra
$ luarocks --version
/usr/local/bin/luarocks 2.4.2
LuaRocks main command-line interface
$ luarocks make
lua-cassandra 1.2.3-0 is now installed in /usr/local/opt/kong (license: MIT)
There was a problem hiding this comment.
Looks like this changed in luarocks 2.4.x. On my system I had luarocks 2.3 and it fails.
luarocks install luarocks fixed it.
Will revert
|
|
||
| dev: install | ||
| install-dev: | ||
| @luarocks $(FLAGS) make $(DEV_ROCKFILE) OPENSSL_DIR=$(OPENSSL_DIR) |
There was a problem hiding this comment.
I believe you can already specify this with OPENSSL_DIR=/usr/local/opt/openssl/ make dev
There was a problem hiding this comment.
nope :( luarocks ignores it
~> OPENSSL_DIR=/usr/local/opt/openssl luarocks install --local luasec
Warning: falling back to curl - install luasec to get native HTTPS support
Installing https://luarocks.org/luasec-0.7alpha-1.rockspec...
Using https://luarocks.org/luasec-0.7alpha-1.rockspec... switching to 'build' mode
Error: Could not find header file for OPENSSL
No file openssl/ssl.h in /usr/local/include
No file openssl/ssl.h in /usr/include
You may have to install OPENSSL in your system and/or pass OPENSSL_DIR or OPENSSL_INCDIR to the luarocks command.
Example: luarocks install luasec OPENSSL_DIR=/usr/local
There was a problem hiding this comment.
I'd rather we don't add this new rule to the makefile at all. I'm not sure I understand why we need it.
The Makefile is there only to run make dev and install the dev dependencies. There isn't a need to install the library in the LuaRocks tree - one should use luarocks install lua-cassandra or luarocks make or make install for that, but that's it. The tests will already run against the src tree.
| "luabitop" | ||
| "luabitop", | ||
| "luasec", | ||
| "luasocket" |
There was a problem hiding this comment.
Those dependencies are deliberately not included in this rockspec because they are not needed unless the user runs this library in a non-cosocket context (non-OpenResty or an OpenResty context that doesn't support cosocokets)
There was a problem hiding this comment.
Should the be listed in DEV_ROCKS then? They are needed for tests and they don't get installed by anything right now
Well, /usr/local on my laptop is not writable exactly to forbid tools (like pip or luarocks) to install stuff there without me noticing. Global installs for dependencies are bad because if you work on more than one project you'll keep having conflicting dependencies and one project will keep upgrading / downgrading the libraries. Another example is that
that's basically what --local does
yeah, I can do that but it'd be much nicer to be able to just use the Makefile since we already have one. I don't think adding the possibility to use |
| @for rock in $(DEV_ROCKS) ; do \ | ||
| if ! luarocks list | grep $$rock > /dev/null ; then \ | ||
| echo $$rock not found, installing via luarocks... ; \ | ||
| luarocks install $$rock ; \ | ||
| luarocks install $(FLAGS) $$rock ; \ |
There was a problem hiding this comment.
I think this is nice to keep 👍
/usr/local is not writable on my machine by purpose, so
make devfailswith permission error. I've changed the Makefile to allow installing
rocks in the user's home directory. This is optional, so by default
make devwill have the same behaviour as now.I'm not super happy with this solution either, imo the right thing would
be to create a local tree inside this directory and install the
dependencies there instead of polluting the system. That's possible and
it works, however it required me to set the lua load paths in several
places and wasn't "backward compatible", so I chose to go with this
solution for now which should be easier to code-review.
Another issue that I encountered is that openssl header files on macOS
are not inside
/usr/local, which mean I couldn't install luasec. Tofix this I've added an optional OPENSSL_DIR env variable that can be
used to configure luarocks. (This is the suggested procedure on luasec's
website).
I've also added luasec and luasocket as lua-cassandra dependencies since
without them the tests break. There doesn't seem to be a way to mark
luasec as optional (since it's only require if you enable ssl)
unfortunately.