Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
Willy, Am 24.01.20 um 15:11 schrieb Willy Tarreau: > Well, I guess this is reasonable. I'm still just not 100% sold to the > syntax yet, so depending on users feedback I don't exclude changing it > again, in which case we'll have to backport it as well so that 2.1->2.2 > remains a smooth upgrade. It shouldn't be a big deal as users running > such non-LTS versions cannot have outdated versions anyway (or they're > trying hard to run into trouble), and know they may face a bit more > rough edges than with LTS ones. > I can work with adjusting my configuration, if necessary. Compiling my own HAProxy comes with a little more effort. So that's entirely fine for me. Thanks Tim Düsterhus
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
On Fri, Jan 24, 2020 at 12:12:26PM +0100, Tim Düsterhus wrote: > Willy, > > Am 24.01.20 um 09:23 schrieb Willy Tarreau: > > On Thu, Jan 23, 2020 at 03:56:29PM +0100, Tim Düsterhus wrote: > >> This might have slipped through the cracks. Can you please take a look? > >> If you don't have the time to look at it right now, just let me know. > > > > OK, now merged. > > Thanks. > > Any chance for a backport to the 2.1 non-LTS stable branch? I'm using > Vincent's Debian Packages, the option would ease deployment for me and > it's roughly 4 months to go for 2.2. Well, I guess this is reasonable. I'm still just not 100% sold to the syntax yet, so depending on users feedback I don't exclude changing it again, in which case we'll have to backport it as well so that 2.1->2.2 remains a smooth upgrade. It shouldn't be a big deal as users running such non-LTS versions cannot have outdated versions anyway (or they're trying hard to run into trouble), and know they may face a bit more rough edges than with LTS ones. So that's backported now. Willy
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
Willy, Am 24.01.20 um 09:23 schrieb Willy Tarreau: > On Thu, Jan 23, 2020 at 03:56:29PM +0100, Tim Düsterhus wrote: >> This might have slipped through the cracks. Can you please take a look? >> If you don't have the time to look at it right now, just let me know. > > OK, now merged. Thanks. Any chance for a backport to the 2.1 non-LTS stable branch? I'm using Vincent's Debian Packages, the option would ease deployment for me and it's roughly 4 months to go for 2.2. Best regards Tim Düsterhus
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
On Thu, Jan 23, 2020 at 03:56:29PM +0100, Tim Düsterhus wrote: > This might have slipped through the cracks. Can you please take a look? > If you don't have the time to look at it right now, just let me know. OK, now merged. thanks, Willy
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
Hi Tim, On Thu, Jan 23, 2020 at 03:56:29PM +0100, Tim Düsterhus wrote: > > Are you happy with my patch series as is or would you like to see > > changes? Should I update the config syntax? > > This might have slipped through the cracks. Can you please take a look? > If you don't have the time to look at it right now, just let me know. Sorry, Thierry responded to me he was OK with either solution yesterday but immediately after I got diverted by lots of other stuff and forgot again :-( I'm adding it in my todo list, maybe for tomorrow. Thanks, Willy
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
Thierry, Willy, Am 17.01.20 um 13:58 schrieb Tim Düsterhus: Idea 1: lua-prepend-path path /etc/haproxy/lua-modules/?.lua lua-prepend-path cpath /etc/haproxy/lua-cmodules/?.lua Idea 2: lua-prepend-path /etc/haproxy/lua-modules/?.lua lua-prepend-path cpath /etc/haproxy/lua-cmodules/?.lua Idea 3: lua-prepend-path /etc/haproxy/lua-modules/?.lua lua-prepend-cpath /etc/haproxy/lua-cmodules/?.lua >>> >>> I guess the third one is better, at least for a reason which is that >>> it causes less confusion when asking a bug reporter "what's in your >>> lua-prepend-path ?". We've seen sufficient confusion from the maxconn >>> word being used for different tihngs depending on where it's set, so >>> we'd rather keep this clear. >> >> Let me give my reasoning for my choice: >> >> - I wanted to avoid two completely distinct configuration options for >> what is essentially the same thing. It would require adding both to the >> documentation. >> - I made the type optional, because I expect the majority of modules >> loaded via this option to be pure Lua modules. The path is meant to be >> the home of HAProxy specific modules. I consider it unlikely for an user >> to develop a C based Lua module that is HAProxy specific when they can >> simply use a regular C based HAProxy module without corkscrewing it >> through Lua. Stuff such as cjson would be put into the system wide Lua >> path and thus be available to every Lua script including HAProxy, >> because it sits in the default path that is compiled into the Lua VM. >> - I put the type last, because I consider optional parameters that are >> in the middle to be unintuitive and it complicates parsing, because a >> single index might either be the type or the path if the type is not given. >> >> However I don't particularly care for any of this. If you feel like we >> should to lua-prepend-path and lua-prepend-cpath instead then I'm happy >> to adjust the patch (or you do it). > > Are you happy with my patch series as is or would you like to see > changes? Should I update the config syntax? This might have slipped through the cracks. Can you please take a look? If you don't have the time to look at it right now, just let me know. Best regards Tim Düsterhus
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
Thierry, Am 14.01.20 um 19:37 schrieb Tim Düsterhus: > Willy, > > Am 14.01.20 um 14:53 schrieb Willy Tarreau: >> On Tue, Jan 14, 2020 at 02:04:04PM +0100, Thierry Fournier wrote: >>> Idea 1: >>> >>>lua-prepend-path path /etc/haproxy/lua-modules/?.lua >>>lua-prepend-path cpath /etc/haproxy/lua-cmodules/?.lua >>> >>> Idea 2: >>> >>>lua-prepend-path /etc/haproxy/lua-modules/?.lua >>>lua-prepend-path cpath /etc/haproxy/lua-cmodules/?.lua >>> >>> Idea 3: >>> >>>lua-prepend-path /etc/haproxy/lua-modules/?.lua >>>lua-prepend-cpath /etc/haproxy/lua-cmodules/?.lua >> >> I guess the third one is better, at least for a reason which is that >> it causes less confusion when asking a bug reporter "what's in your >> lua-prepend-path ?". We've seen sufficient confusion from the maxconn >> word being used for different tihngs depending on where it's set, so >> we'd rather keep this clear. > > Let me give my reasoning for my choice: > > - I wanted to avoid two completely distinct configuration options for > what is essentially the same thing. It would require adding both to the > documentation. > - I made the type optional, because I expect the majority of modules > loaded via this option to be pure Lua modules. The path is meant to be > the home of HAProxy specific modules. I consider it unlikely for an user > to develop a C based Lua module that is HAProxy specific when they can > simply use a regular C based HAProxy module without corkscrewing it > through Lua. Stuff such as cjson would be put into the system wide Lua > path and thus be available to every Lua script including HAProxy, > because it sits in the default path that is compiled into the Lua VM. > - I put the type last, because I consider optional parameters that are > in the middle to be unintuitive and it complicates parsing, because a > single index might either be the type or the path if the type is not given. > > However I don't particularly care for any of this. If you feel like we > should to lua-prepend-path and lua-prepend-cpath instead then I'm happy > to adjust the patch (or you do it). > > Best regards > Tim Düsterhus > Are you happy with my patch series as is or would you like to see changes? Should I update the config syntax? Best regards Tim Düsterhus
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
Willy, Am 14.01.20 um 14:53 schrieb Willy Tarreau: > On Tue, Jan 14, 2020 at 02:04:04PM +0100, Thierry Fournier wrote: >> Idea 1: >> >>lua-prepend-path path /etc/haproxy/lua-modules/?.lua >>lua-prepend-path cpath /etc/haproxy/lua-cmodules/?.lua >> >> Idea 2: >> >>lua-prepend-path /etc/haproxy/lua-modules/?.lua >>lua-prepend-path cpath /etc/haproxy/lua-cmodules/?.lua >> >> Idea 3: >> >>lua-prepend-path /etc/haproxy/lua-modules/?.lua >>lua-prepend-cpath /etc/haproxy/lua-cmodules/?.lua > > I guess the third one is better, at least for a reason which is that > it causes less confusion when asking a bug reporter "what's in your > lua-prepend-path ?". We've seen sufficient confusion from the maxconn > word being used for different tihngs depending on where it's set, so > we'd rather keep this clear. Let me give my reasoning for my choice: - I wanted to avoid two completely distinct configuration options for what is essentially the same thing. It would require adding both to the documentation. - I made the type optional, because I expect the majority of modules loaded via this option to be pure Lua modules. The path is meant to be the home of HAProxy specific modules. I consider it unlikely for an user to develop a C based Lua module that is HAProxy specific when they can simply use a regular C based HAProxy module without corkscrewing it through Lua. Stuff such as cjson would be put into the system wide Lua path and thus be available to every Lua script including HAProxy, because it sits in the default path that is compiled into the Lua VM. - I put the type last, because I consider optional parameters that are in the middle to be unintuitive and it complicates parsing, because a single index might either be the type or the path if the type is not given. However I don't particularly care for any of this. If you feel like we should to lua-prepend-path and lua-prepend-cpath instead then I'm happy to adjust the patch (or you do it). Best regards Tim Düsterhus
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
Thierry, Am 14.01.20 um 14:04 schrieb Thierry Fournier: > Just a little remark, I hope without consequences. Lua C libraries > are .so and not .lua: > >lua-prepend-path /etc/haproxy/lua-cmodules/?.lua cpath > > becomes: > >lua-prepend-path /etc/haproxy/lua-cmodules/?.so cpath > Of course. That just was a copy and paste mistake within my cover letter of the new patch series. You can specify arbitrary paths with arbitrary file extensions. The implementation prepends the path as specified within the configuration option. Best regards Tim Düsterhus
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
On Tue, Jan 14, 2020 at 02:04:04PM +0100, Thierry Fournier wrote: > Idea 1: > >lua-prepend-path path /etc/haproxy/lua-modules/?.lua >lua-prepend-path cpath /etc/haproxy/lua-cmodules/?.lua > > Idea 2: > >lua-prepend-path /etc/haproxy/lua-modules/?.lua >lua-prepend-path cpath /etc/haproxy/lua-cmodules/?.lua > > Idea 3: > >lua-prepend-path /etc/haproxy/lua-modules/?.lua >lua-prepend-cpath /etc/haproxy/lua-cmodules/?.lua I guess the third one is better, at least for a reason which is that it causes less confusion when asking a bug reporter "what's in your lua-prepend-path ?". We've seen sufficient confusion from the maxconn word being used for different tihngs depending on where it's set, so we'd rather keep this clear. Thanks, Willy
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
> On 14 Jan 2020, at 13:53, Willy Tarreau wrote: > > On Tue, Jan 14, 2020 at 01:46:55PM +0100, Thierry Fournier wrote: >> Hi, It have two default path in the library path: >> >> - path for lua libraries >> - cpath for loading lua C libraries. >> >> In some cases, haproxy requires C libraries like cjson which manipulates >> json. >> >> I'm not sure, but it seems that the patch "lua-prepend-path" doesn't include >> the cpath. > > Yes it does now with an optional "cpath" keyword on the line. Maybe it's > not the most convenient way and you'd prefer separate keywords ? Sorry, I don’t see the keyword "cpath". Maybe a lua-prepend-cpath would be clearer than a final keyword ? I don’t known. Maybe the cpath word would be specified at the start in place of the end ? The most important it was the cpath was taken in account ! Just a little remark, I hope without consequences. Lua C libraries are .so and not .lua: lua-prepend-path /etc/haproxy/lua-cmodules/?.lua cpath becomes: lua-prepend-path /etc/haproxy/lua-cmodules/?.so cpath Idea 1: lua-prepend-path path /etc/haproxy/lua-modules/?.lua lua-prepend-path cpath /etc/haproxy/lua-cmodules/?.lua Idea 2: lua-prepend-path /etc/haproxy/lua-modules/?.lua lua-prepend-path cpath /etc/haproxy/lua-cmodules/?.lua Idea 3: lua-prepend-path /etc/haproxy/lua-modules/?.lua lua-prepend-cpath /etc/haproxy/lua-cmodules/?.lua Thierry
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
On Tue, Jan 14, 2020 at 01:46:55PM +0100, Thierry Fournier wrote: > Hi, It have two default path in the library path: > > - path for lua libraries > - cpath for loading lua C libraries. > > In some cases, haproxy requires C libraries like cjson which manipulates json. > > I'm not sure, but it seems that the patch "lua-prepend-path" doesn't include > the cpath. Yes it does now with an optional "cpath" keyword on the line. Maybe it's not the most convenient way and you'd prefer separate keywords ? Willy
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
> On 14 Jan 2020, at 07:08, Willy Tarreau wrote: > > On Sun, Jan 12, 2020 at 01:55:38PM +0100, Tim Duesterhus wrote: >> Willy, >> Thierry, >> Vincent, >> >> I've just prepared a new version of my proposal with the following >> differences: >> >> 1. I adjusted the documentation as suggested by Willy. >> 2. I added support for `package.cpath` as suggested by Thierry. >> 3. I added a build time option to be used by distros as suggested by Vincent. > (...) > > Thanks Tim! Thierry, just let me know if you think it's OK for merging > and I'll handle it, or if you need more time to review it or are still > having any doubts. > Hi, It have two default path in the library path: - path for lua libraries - cpath for loading lua C libraries. In some cases, haproxy requires C libraries like cjson which manipulates json. I’m not sure, but it seems that the patch “lua-prepend-path” doesn’t include the cpath. Thierry
Re: [PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
On Sun, Jan 12, 2020 at 01:55:38PM +0100, Tim Duesterhus wrote: > Willy, > Thierry, > Vincent, > > I've just prepared a new version of my proposal with the following > differences: > > 1. I adjusted the documentation as suggested by Willy. > 2. I added support for `package.cpath` as suggested by Thierry. > 3. I added a build time option to be used by distros as suggested by Vincent. (...) Thanks Tim! Thierry, just let me know if you think it's OK for merging and I'll handle it, or if you need more time to review it or are still having any doubts. Thanks, Willy
[PATCH v2 0/3] MINOR: lua: Add lua-prepend-path configuration option
Willy, Thierry, Vincent, I've just prepared a new version of my proposal with the following differences: 1. I adjusted the documentation as suggested by Willy. 2. I added support for `package.cpath` as suggested by Thierry. 3. I added a build time option to be used by distros as suggested by Vincent. The build time option will appear in the OPTIONS value of `haproxy -vv` and default to empty: OPTIONS = USE_LUA=1 HLUA_PREPEND_PATH=/foobar/baz/? With that build option and this configuration: lua-prepend-path /etc/haproxy/lua-modules/?/init.lua lua-prepend-path /etc/haproxy/lua-modules/?.lua lua-prepend-path /etc/haproxy/lua-cmodules/?.lua cpath The loaded paths look like this: [timwolla@/s/haproxy (lua-prepend-path) [1]]./haproxy -d -f ./haproxy.cfg 13:52:53 [ALERT] 011/135320 (27431) : parsing [./haproxy.cfg:6] : Lua runtime error: /tmp/haproxy-auth-request/auth-request.lua:23: module 'http' not found: no field package.preload['http'] no file '/etc/haproxy/lua-modules/http.lua' no file '/etc/haproxy/lua-modules/http/init.lua' no file '/foobar/baz/http' no file '/usr/local/share/lua/5.3/http.lua' no file '/usr/local/share/lua/5.3/http/init.lua' no file '/usr/local/lib/lua/5.3/http.lua' no file '/usr/local/lib/lua/5.3/http/init.lua' no file '/usr/share/lua/5.3/http.lua' no file '/usr/share/lua/5.3/http/init.lua' no file './http.lua' no file './http/init.lua' no file '/etc/haproxy/lua-cmodules/http.lua' no file '/usr/local/lib/lua/5.3/http.so' no file '/usr/lib/x86_64-linux-gnu/lua/5.3/http.so' no file '/usr/lib/lua/5.3/http.so' no file '/usr/local/lib/lua/5.3/loadall.so' no file './http.so' Please have a look if you are happy with my adjustments. Best regards Tim Duesterhus (3): MINOR: lua: Add hlua_prepend_path function MINOR: lua: Add lua-prepend-path configuration option MINOR: lua: Add HLUA_PREPEND_C?PATH build option Makefile | 10 doc/configuration.txt | 26 + src/hlua.c| 54 +++ 3 files changed, 90 insertions(+) -- 2.24.1
