Allow installing rocks locally - fix openssl include #109

drolando · 2017-10-05T23:49:05Z

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

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

drolando · 2017-10-06T00:16:37Z

Oh also, luarocks make requires the rockspec file as argument. I don't know if this changed in a recent version of luarocks or why it used to work for you

thibaultcha

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 --local flag 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).

thibaultcha · 2017-10-06T00:42:54Z

README.md

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


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

thibaultcha · 2017-10-06T00:43:51Z

README.md

+
+```
+OPENSSL_DIR=/usr/local/opt/openssl/ FLAGS='--local' make busted
+```


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

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.

thibaultcha · 2017-10-06T00:45:37Z

Makefile


 .PHONY: install dev busted prove test clean coverage lint doc

 install:
-	@luarocks make
+	@luarocks make $(PROD_ROCKFILE)


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)

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

thibaultcha · 2017-10-06T00:46:37Z

Makefile


-dev: install
+install-dev:
+	@luarocks $(FLAGS) make $(DEV_ROCKFILE) OPENSSL_DIR=$(OPENSSL_DIR)


I believe you can already specify this with OPENSSL_DIR=/usr/local/opt/openssl/ make dev

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

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.

thibaultcha · 2017-10-06T00:47:26Z

lua-cassandra-1.2.3-0.rockspec

-  "luabitop"
+  "luabitop",
+  "luasec",
+  "luasocket"


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)

Should the be listed in DEV_ROCKS then? They are needed for tests and they don't get installed by anything right now

DEV_ROCKS sounds good! 👍

drolando · 2017-10-06T01:48:40Z

I believe if /usr/local isn't writable by default on your machine, that it may be your responsibility to either:

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 /usr/local is not writable on our work devboxes since they're shared between multiple user, so we'd have even worse conflicts.

install the LuaRocks system tree in a writable directory

that's basically what --local does

install this library and its dependencies in the user tree (via the --local flag like you did)

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 --local in the Makefile itself is a big issue (the changes are minimal and backward compatible).

thibaultcha · 2017-10-06T18:20:34Z

Makefile

 	@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 ; \


I think this is nice to keep 👍

thibaultcha reviewed Oct 6, 2017

View reviewed changes

thibaultcha force-pushed the master branch from 3bb1e4b to 59b278a Compare November 15, 2018 01:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow installing rocks locally - fix openssl include #109

Allow installing rocks locally - fix openssl include #109

drolando commented Oct 5, 2017

drolando commented Oct 6, 2017

thibaultcha left a comment

thibaultcha Oct 6, 2017

thibaultcha Oct 6, 2017

drolando Oct 6, 2017

thibaultcha Oct 6, 2017

drolando Oct 6, 2017 •

edited

Loading

thibaultcha Oct 6, 2017

drolando Oct 6, 2017

thibaultcha Oct 6, 2017

thibaultcha Oct 6, 2017

drolando Oct 6, 2017

thibaultcha Oct 6, 2017

drolando commented Oct 6, 2017

thibaultcha Oct 6, 2017

Allow installing rocks locally - fix openssl include #109

Are you sure you want to change the base?

Allow installing rocks locally - fix openssl include #109

Conversation

drolando commented Oct 5, 2017

drolando commented Oct 6, 2017

thibaultcha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drolando Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drolando commented Oct 6, 2017

Choose a reason for hiding this comment

drolando Oct 6, 2017 •

edited

Loading