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.

Sign up for GitHub

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

Allow installing rocks locally - fix openssl include #109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow installing rocks locally - fix openssl include #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,29 @@
DEV_ROCKS=busted luacov luacov-coveralls luacheck ldoc
BUSTED_ARGS ?= -v -o gtest
CASSANDRA ?= 3.10
PROD_ROCKFILE = $(shell ls lua-cassandra-*.rockspec | grep -v dev)
DEV_ROCKFILE = $(shell ls lua-cassandra-*.rockspec | grep dev)
FLAGS ?=

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

install:
@luarocks make
@luarocks make $(PROD_ROCKFILE)
Copy link
Owner

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@drolando drolando Oct 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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)
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.


dev:
@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 ; \
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is nice to keep 👍

else \
echo $$rock already installed, skipping ; \
fi \
done;

busted:
busted: install-dev
@busted $(BUSTED_ARGS)

prove:
Expand All @@ -31,7 +37,7 @@ clean:
@rm -f luacov.*
@util/clean_ccm.sh

coverage: clean
coverage: clean install-dev
@busted $(BUSTED_ARGS) --coverage
@util/prove_ccm.sh $(CASSANDRA)
@TEST_COVERAGE_ENABLED=true TEST_NGINX_TIMEOUT=30 prove
Expand Down
20 changes: 20 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,26 @@ The documentation is generated with
$ make doc
```

#### macOS notes

## Openssl headers

Newer versions of macOS don't include the openssl headers by purpose. You'll need to install
them via `brew install openssl` or manually.

Homebrew openssl headers however don't get symlinked in /usr/local, so you need to tell
luarocks where to find them via `OPENSSL_DIR`.

```
OPENSSL_DIR=/usr/local/opt/openssl/ FLAGS='--local' make busted
```
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


## Install rocks locally

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.
Copy link
Owner

Choose a reason for hiding this comment

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

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


[Back to TOC](#table-of-contents)

[Luarocks]: https://luarocks.org
Expand Down
4 changes: 3 additions & 1 deletion lua-cassandra-1.2.3-0.rockspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ description = {
license = "MIT"
}
dependencies = {
"luabitop"
"luabitop",
"luasec",
"luasocket"
Copy link
Owner

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

DEV_ROCKS sounds good! 👍

}
build = {
type = "builtin",
Expand Down
4 changes: 3 additions & 1 deletion lua-cassandra-dev-0.rockspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ description = {
license = "MIT"
}
dependencies = {
"luabitop"
"luabitop",
"luasec",
"luasocket"
}
build = {
type = "builtin",
Expand Down