search

p0pr0ck5 / lua-resty-waf

High-performance WAF built on the OpenResty stack
GNU General Public License v3.0
1.28k stars 305 forks source link

Hard-coded /usr/local prefix #290

Closed kwaping closed 7 years ago

kwaping commented 7 years ago

My company wants to use our own prefix for the OpenResty install, but currently are bound by the hard-coded /usr/local prefix in this module. Is there any reason that can't be made into a variable/parameter? I grepped through the code and it didn't look hard, just wondering if there's a good reason why it's not already a variable.

I'm willing to do the work and submit a PR.

p0pr0ck5 commented 7 years ago

hey @kwaping,

Do you mean https://github.com/p0pr0ck5/lua-resty-waf/blob/master/Makefile#L1? You can override that when calling make. Otherwise, can you clarify what specific definitions you're referencing?

kwaping commented 7 years ago

Thanks for the quick reply! Here are the results of my grep:

lua-resty-waf oready$ grep -rin '/usr/local' *
Makefile:1:OPENRESTY_PREFIX ?= /usr/local/openresty
libinjection/lua/Makefile:23:   LUA_FLAGS=-I/usr/local/include/luajit-2.0 -L/usr/local/lib -lluajit-5.1
libinjection/src/Makefile:12:PREFIX=/usr/local
lua-resty-htmlentities/Makefile:20:PREFIX := /usr/local

libinjection/lua seems the most hard-coded but libinjection/src also is a concern. It looks like the lua-resty-htmlentities/Makefile instance can be overridden like in your own Makefile.

p0pr0ck5 commented 7 years ago

Hey,

Both of these are third party libraries, so for adjustments to the Makefiles, best to contact the authors for long-term fixes.

For libjection, it's not assignable, but since we're using a fork (https://github.com/p0pr0ck5/libinjection), I would accept a PR on that repo to make PREFIX assign conditionally. We (or you :) ) can then PR it upstream as well.

For lua-resty-htmlentities, probably best to make a PR upstream. If they don't accept a PR in a timely manner, I would be okay with forking that submodule as well to meet this use case. Long term for all submodules we should probably move away from forks, toward a base on upstream directly.

And again for our own Makefile, you can define OPENRESTY_PREFIX on your own as part of the build process (e.g., OPENRESTY_PREFIX=/foo make && make install)

Does this cover your use cases? Let me know if there's anything I missed :)

kwaping commented 7 years ago

Thanks Robert, I will take your excellent advice. I think that covers all the bases. 👍 I'll send you a PR if/when it's ready, but will try for upstream first. We have a time constraint so we might have to go with Plan B instead (forked).

kwaping commented 7 years ago

After looking at things more closely, I don't think any edits are needed. I think we just need to override OPENRESTY_PREFIX. Thank you again for your help!

p0pr0ck5 commented 7 years ago

Great, that's encouraging to hear! Feel free to pop in and let me know if you notice anything else. The more feedback we get, the better!