this patch adds an automatic filesystem refresh in Plack, patch uses the following module... https://metacpan.org/pod/Plack::Middleware::Refresh to test: 1/ start ktd 2/ add warn to about.pl file, loadpage, observe no output in log :( 3/ apply patch to /etc/koha/plack.psgi 4/ restart plack 5/ add warn to about.pl file, loadpage, observe output in log :)
Created attachment 141577 [details] [review] Bug 31729: Enable automatic filesystem refresh in Plack patch uses the following module... https://metacpan.org/pod/Plack::Middleware::Refresh to test: 1/ start ktd 2/ add warn to about.pl file, load page, observe no output in log :( 3/ apply patch to /etc/koha/plack.psgi 4/ restart plack 5/ add warn to about.pl file, load page, observe output in log :)
Created attachment 141578 [details] [review] Bug 31729: Enable automatic filesystem refresh in Plack (2) updating mojo psgi file too, (not sure how to test patch)
hmm, i've spotted a problem here switching back to ASSIGNED
(In reply to Mason James from comment #1) > to test: > > 1/ start ktd > 2/ add warn to about.pl file, load page, observe no output in log :( > 3/ apply patch to /etc/koha/plack.psgi > 4/ restart plack > 5/ add warn to about.pl file, load page, observe output in log :) I don't think Plack::Middleware::Refresh would apply in this situation, since Module::Refresh scans the %INC for modified files. With this test plan, I think that it would only seem to work because the Starman worker replying to the request hasn't yet cached the compiled version of about.pl in its memory (in the Plack::App::CGIBin namespace). All the more reason for us to move our code out of .pl scripts and into .pm modules hehe.
(In reply to David Cook from comment #4) > (In reply to Mason James from comment #1) > > to test: > > > > 1/ start ktd > > 2/ add warn to about.pl file, load page, observe no output in log :( > > 3/ apply patch to /etc/koha/plack.psgi > > 4/ restart plack > > 5/ add warn to about.pl file, load page, observe output in log :) > > I don't think Plack::Middleware::Refresh would apply in this situation, > since Module::Refresh scans the %INC for modified files. this seems to be a better solution for ktd https://gitlab.com/mjames/koha-reload-starman/-/blob/master/koha-reload-starman
(In reply to Mason James from comment #5) > this seems to be a better solution for ktd > > https://gitlab.com/mjames/koha-reload-starman/-/blob/master/koha-reload- > starman Yeah I think inotify is probably the easiest way to do this. Personally, I probably wouldn't want to use this feature. I'm happy to make a batch of changes and then run either a "koha-plack --restart" or "koha-plack --reload". But it would be nice to have that hot deploy option - at least for ktd.
Created attachment 141596 [details] [review] Bug 31729: Enable automatic filesystem refresh in Plack patch uses the following module... https://metacpan.org/pod/Plack::Middleware::Refresh to test: 1/ start ktd 2/ add warn to about.pl file, load page, observe no output in log :( 3/ apply patch to /etc/koha/plack.psgi 4/ restart plack 5/ add warn to about.pl file, load page, observe output in log :) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 141597 [details] [review] Bug 31729: Enable automatic filesystem refresh in Plack (2) updating mojo psgi file too, (not sure how to test patch) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 141598 [details] [review] Bug 31729: Add missing brace (and tidy) This adds a missing closing bracket and runs perltidy over the .psgi scripts.
Oop, I tested first thing this morning without refreshing to see the comments.
Created attachment 141714 [details] [review] Bug 31729: Enable automatic filesystem refresh in Plack to test: 0/ apt install inotify-tools 1/ apply patch 2/ copy files, and set perms manually sudo cp debian/script/koha-plack /usr/sbin sudo cp debian/script/koha-watcher to /usr/sbin sudo chmod 755 debian/script/koha-plack /usr/sbin/koha-plack sudo chmod 755 debian/script/koha-watcher to /usr/sbin/koha-watcher 3/ koha-plack --start --watch kohadev 4/ pat a kitten [U+1F431] 5/ modify file in watched dir echo `date` > /usr/share/koha/xxx1 echo `date` > /usr/share/koha/xxx2 6/ see HUP/inotify messages in log file $ tail -f /var/log/koha/kohadev/plack-error.log /usr/share/koha/ MODIFY xxx1 Sending children hup signal /usr/share/koha/ MODIFY xxx2 Sending children hup signal 7/ observe koha-watch process ps -ef | grep koha-watcher | grep -v grep | wc -l 1 8/ koha-plack --stop kohadev 9/ observe no koha-watch process ps -ef | grep koha-watcher | grep -v grep | wc -l 0
Created attachment 141715 [details] [review] Bug 31729: Enable automatic filesystem refresh in Plack (2) fix typo
Created attachment 141716 [details] [review] Bug 31729 - Enable automatic filesystem refresh in Plack (3) fix 'elif' typo
> 5/ modify file in watched dir > echo `date` > /usr/share/koha/xxx1 > echo `date` > /usr/share/koha/xxx2 if testing in KTD, try... # echo `date` > /kohadevbox/koha/xxx2
Shouldn't we watch for perl files only? What about git checkout branch? Is it going to send 1 signal per file modified?
Created attachment 142043 [details] [review] Bug 31729: watch only perl files
Created attachment 142044 [details] [review] Bug 31729: Enable automatic filesystem refresh in Plack (3) fix 'elif' typo
Created attachment 142050 [details] [review] Bug 31729: add 2 sec loop to koha-watcher, to avoid reload flooding
(In reply to Jonathan Druart from comment #15) > Shouldn't we watch for perl files only? > > What about git checkout branch? Is it going to send 1 signal per file > modified? hiya, i've added 2 patches to handle these 2 conditions
I tried out the latest test plan but cannot continue past Step 3. It seems the test plan is only appropriate for koha installations made with the debian package, is that correct?
Created attachment 148600 [details] [review] Bug 31729: Enable automatic filesystem refresh in Plack to test (on KTD) - install inotify-tools $ sudo apt install inotify-tools - apply patch $ git bz apply 31729 - manually install files and set perms $ cd /kohadevbox/koha $ sudo cp debian/scripts/koha-plack /usr/sbin $ sudo cp debian/scripts/koha-watcher /usr/sbin $ sudo chmod 755 /usr/sbin/koha-plack $ sudo chmod 755 /usr/sbin/koha-watcher - restart, with --watch arg $ sudo koha-plack --restart --watch kohadev - observe koha-watch process $ ps -ef | grep koha-watcher | grep -v grep | wc -l 3 - modify some perl files in watched dir $ echo `date` > /kohadevbox/koha/xxx.pl $ echo `date` > /kohadevbox/koha/xxx.pm - see HUP/inotify messages in log file $ tail -f /var/log/koha/kohadev/plack-error.log CLOSE_WRITE,CLOSE happened to xxx.pl in /kohadevbox/koha/ CLOSE_WRITE,CLOSE happened to xxx.pm in /kohadevbox/koha/ Sending children hup signal - stop koha-plack $ sudo koha-plack --stop kohadev - observe no koha-watch process $ ps -ef | grep koha-watcher | grep -v grep | wc -l 0
(In reply to Olivier Hubert from comment #20) > I tried out the latest test plan but cannot continue past Step 3. It seems > the test plan is only appropriate for koha installations made with the > debian package, is that correct? hi Olivier if you follow the updated patch and test plan - you should have success many thanks :)
Created attachment 148602 [details] [review] Bug 31729: fix quoting and set sh
Created attachment 148603 [details] [review] Bug 31729: fix quoting and set sh
Created attachment 148606 [details] [review] Bug 31729: move $tmpfile to /tmp dir, and add {}
Why are you logging in plack-error.log?
Created attachment 148612 [details] [review] Bug 31729: renamed log file to plack-watcher.log to test... - modify some perl files in watched dir $ echo `date` > /kohadevbox/koha/xxx.pl $ echo `date` > /kohadevbox/koha/xxx.pm - see HUP/inotify messages in log file $ tail -f /var/log/koha/kohadev/plack-watcher.log <<<<<<<< CLOSE_WRITE,CLOSE happened to xxx.pl in /kohadevbox/koha/ CLOSE_WRITE,CLOSE happened to xxx.pm in /kohadevbox/koha/ Sending children hup signal
Created attachment 148613 [details] [review] Bug 31729: renamed log file to plack-watcher.log to test... - modify some perl files in watched dir $ echo `date` > /kohadevbox/koha/xxx.pl $ echo `date` > /kohadevbox/koha/xxx.pm - see HUP/inotify messages in log file $ tail -f /var/log/koha/kohadev/plack-watcher.log \ /var/log/koha/kohadev/plack-error.log CLOSE_WRITE,CLOSE happened to xxx.pl in /kohadevbox/koha/ CLOSE_WRITE,CLOSE happened to xxx.pm in /kohadevbox/koha/ Sending children hup signal
(In reply to Jonathan Druart from comment #26) > Why are you logging in plack-error.log? oops! i've changed log file to plack-watcher.log
Created attachment 148720 [details] [review] Bug 31729: Enable automatic filesystem refresh in Plack to test (on KTD) - install inotify-tools $ sudo apt install inotify-tools - apply patch $ git bz apply 31729 - manually install files and set perms $ cd /kohadevbox/koha $ sudo cp debian/scripts/koha-plack /usr/sbin $ sudo cp debian/scripts/koha-watcher /usr/sbin $ sudo chmod 755 /usr/sbin/koha-plack $ sudo chmod 755 /usr/sbin/koha-watcher - restart, with --watch arg $ sudo koha-plack --restart --watch kohadev - observe koha-watch process $ ps -ef | grep koha-watcher | grep -v grep | wc -l 3 - modify some perl files in watched dir $ echo `date` > /kohadevbox/koha/xxx.pl $ echo `date` > /kohadevbox/koha/xxx.pm - see HUP/inotify messages in log file $ tail -f /var/log/koha/kohadev/plack-watcher.log CLOSE_WRITE,CLOSE event on /kohadevbox/koha/xxx.pl CLOSE_WRITE,CLOSE event on /kohadevbox/koha/xxx.pm koha-plack reloaded - add syntax error to file in watched dir $ echo `date` > /kohadevbox/koha/C4/Context.pm - see HUP/inotify messages in log file $ tail -f /var/log/koha/kohadev/plack-watcher.log CLOSE_WRITE,CLOSE event on /kohadevbox/koha/C4/Context.pm koha-plack failed! sleeping 2 secs koha-plack failed! sleeping 3 secs koha-plack failed! sleeping 4 secs ... koha-plack failed! sleeping 10 secs - fix syntax error in file in watched dir $ git checkout /kohadevbox/koha/C4/Context.pm - see HUP/inotify messages in log file $ tail -f /var/log/koha/kohadev/plack-watcher.log CLOSE_WRITE,CLOSE event on /kohadevbox/koha/C4/Context.pm koha-plack reloaded ok - stop koha-plack $ sudo koha-plack --stop kohadev - observe no koha-watch process $ ps -ef | grep koha-watcher | grep -v grep | wc -l 0
This sounds like it would be great time saver!
Created attachment 154542 [details] [review] Bug 31729: Enable automatic filesystem refresh in Plack to test (on KTD) - install inotify-tools $ sudo apt install inotify-tools - apply patch $ git bz apply 31729 - manually install files and set perms $ cd /kohadevbox/koha $ sudo cp debian/scripts/koha-plack /usr/sbin $ sudo cp debian/scripts/koha-watcher /usr/sbin $ sudo chmod 755 /usr/sbin/koha-plack $ sudo chmod 755 /usr/sbin/koha-watcher - restart, with --watch arg $ sudo koha-plack --restart --watch kohadev - observe koha-watch process $ ps -ef | grep koha-watcher | grep -v grep | wc -l 3 - modify some perl files in watched dir $ echo `date` > /kohadevbox/koha/xxx.pl $ echo `date` > /kohadevbox/koha/xxx.pm - see HUP/inotify messages in log file $ tail -f /var/log/koha/kohadev/plack-watcher.log CLOSE_WRITE,CLOSE event on /kohadevbox/koha/xxx.pl CLOSE_WRITE,CLOSE event on /kohadevbox/koha/xxx.pm koha-plack reloaded - add syntax error to file in watched dir $ echo `date` > /kohadevbox/koha/C4/Context.pm - see HUP/inotify messages in log file $ tail -f /var/log/koha/kohadev/plack-watcher.log CLOSE_WRITE,CLOSE event on /kohadevbox/koha/C4/Context.pm koha-plack failed! sleeping 2 secs koha-plack failed! sleeping 3 secs koha-plack failed! sleeping 4 secs ... koha-plack failed! sleeping 10 secs - fix syntax error in file in watched dir $ git checkout /kohadevbox/koha/C4/Context.pm - see HUP/inotify messages in log file $ tail -f /var/log/koha/kohadev/plack-watcher.log CLOSE_WRITE,CLOSE event on /kohadevbox/koha/C4/Context.pm koha-plack reloaded ok - stop koha-plack $ sudo koha-plack --stop kohadev - observe no koha-watch process $ ps -ef | grep koha-watcher | grep -v grep | wc -l 0 Signed-off-by: Pascal Uphaus <pascal.uphaus@gwdg.de> Signed-off-by: Thomas Klausner <domm@plix.at> Signed-off-by: Lari Strand <lari.strand@koha-suomi.fi>
FYI, I have another approach nearly finished, where instead of creating a new daemon and using inotify we start the Plack app via plackup (instead of just usnig starman) and use the -R flag to reload when something changed. This is the common way to do is in Plack apps, and does not need another service. The patch will be ready tomorrow at the latest (side note: Not sure if you want the patch in this ticket or in a new one?)
(In reply to Thomas Klausner from comment #33) > FYI, I have another approach nearly finished, where instead of creating a > new daemon and using inotify we start the Plack app via plackup (instead of > just usnig starman) and use the -R flag to reload when something changed. > > This is the common way to do is in Plack apps, and does not need another > service. I use plackup (without -R) for a different Plack app of mine, and I find it doesn't work very well. Maybe it's how I'm using it or some middleware I'm using, but it tends to be slow and seems to hang on some requests until I send another request. But it's worth a try. > The patch will be ready tomorrow at the latest > > (side note: Not sure if you want the patch in this ticket or in a new one?) I think you could attach it here and just mark it as [Alternate]
Just reiterating that it would be great to have this be "optional", since I wouldn't want to use it. Definitely not in prod, and not even in koha-testing-docker. I prefer to manually restart my Plack processes.
(In reply to Thomas Klausner from comment #33) > FYI, I have another approach nearly finished, where instead of creating a > new daemon and using inotify we start the Plack app via plackup (instead of > just usnig starman) and use the -R flag to reload when something changed. > > This is the common way to do is in Plack apps, and does not need another > service. > > The patch will be ready tomorrow at the latest > > (side note: Not sure if you want the patch in this ticket or in a new one?) hi Thomas, i think that plack needs to be restarted if template files (.inc and .tt) are modified ...if so, i dont think `plackup -R` with handle modifying template files?
(In reply to Mason James from comment #36) > i think that plack needs to be restarted if template files (.inc and .tt) > are modified Generally it shouldn't need to be restarted if templates are updated. (Although off the top of my head I don't know why since the templates should be cached... [U+1F62C])
Created attachment 154601 [details] [review] Bug 31729: enable plackup hot restart in ktd [Alternate] This patch implements a more idiomatoic way for hot-reloading. Instead of adding a new daemon/watcher+inotify or use a Middleware, we can use `plackup -R /path/to/dir`, which is the standard Perl way to do hot reloading for plack apps. Instead of calling `/usr/bin/perl starman` directly, we use `plackup -s Starman`. And can then use `-R /kohadevbox/koha/Koha` to watch that directory for changes and hot-reload the plack app. I've added a new option `koha-plack --hot-reload`. Only if this option is set AND we're running inside koha-testing-docker (by checking KOHA_INSTANCE = "kohadev") hot reloading is enabled. Test plan: Before applying the patch: * Start koha-testing-docker and enter the container * Stop plack: koha-plack --stop kohadev * Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log You should see lines like Worker processes cleaned up Server closing! * Start it again: koha-plack --start kohadev Output should be "Starting Plack daemon for kohadev:" And you end back at the interactive shell * Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log it should look like: Starman::Server (type Net::Server::PreFork) starting! Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock" Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/ * Stop it again: koha-plack --stop kohadev Now apply the patch. As in koha-testing-docker the koha-plack script (after applying the patch) is NOT installed to /sbin you have to call the dev version. * Start the server with hot-reloading: debian/scripts/koha-plack --start --hot-reload kohadev * Output should be: Starting Plack daemon for kohadev:Watching /kohadevbox/koha/Koha /etc/koha/sites/kohadev/plack.psgi for file updates. * You are now NOT back in the shell, as the plack app is NOT running daemonized * So you need to open another ktd --shell * Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log Starman::Server (type Net::Server::PreFork) starting! pid(27638) Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock" Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/ * Update a code file touch Koha/Biblios.pm * in the other shell (where plack is running) you should see: -- /kohadevbox/koha/Koha/Biblios.pm updated. Killing the existing server Successfully killed! Restarting the new server process. * Yay! * If you want to still stop/restart plack manually, you can hit CTRL-C in the shell were you started plack to stop the process.
Created attachment 154605 [details] [review] Bug 31729: enable plackup hot restart in ktd [Alternate] This patch implements a more idiomatoic way for hot-reloading. Instead of adding a new daemon/watcher+inotify or use a Middleware, we can use `plackup -R /path/to/dir`, which is the standard Perl way to do hot reloading for plack apps. Instead of calling `/usr/bin/perl starman` directly, we use `plackup -s Starman`. And can then use `-R /kohadevbox/koha/Koha` to watch that directory for changes and hot-reload the plack app. I've added a new option `koha-plack --hot-reload`. Only if this option is set AND we're running inside koha-testing-docker (by checking KOHA_INSTANCE = "kohadev") hot reloading is enabled. Test plan: Before applying the patch: * Start koha-testing-docker and enter the container * Stop plack: koha-plack --stop kohadev * Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log You should see lines like Worker processes cleaned up Server closing! * Start it again: koha-plack --start kohadev Output should be "Starting Plack daemon for kohadev:" And you end back at the interactive shell * Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log it should look like: Starman::Server (type Net::Server::PreFork) starting! Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock" Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/ * Stop it again: koha-plack --stop kohadev Now apply the patch. As in koha-testing-docker the koha-plack script (after applying the patch) is NOT installed to /sbin you have to call the dev version. * Start the server with hot-reloading: debian/scripts/koha-plack --start --hot-reload kohadev * Output should be: Starting Plack daemon for kohadev:Watching /kohadevbox/koha/Koha /etc/koha/sites/kohadev/plack.psgi for file updates. * You are now NOT back in the shell, as the plack app is NOT running daemonized * So you need to open another ktd --shell * Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log Starman::Server (type Net::Server::PreFork) starting! pid(27638) Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock" Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/ * Update a code file touch Koha/Biblios.pm * in the other shell (where plack is running) you should see: -- /kohadevbox/koha/Koha/Biblios.pm updated. Killing the existing server Successfully killed! Restarting the new server process. * Yay! * If you want to still stop/restart plack manually, you can hit CTRL-C in the shell were you started plack to stop the process. Sponsored-by: HKS3 - https://koha-support.eu
Ohhh, Thomas that sounds very interesting! Plack::Loader::Restarter is still using inotify under the hood, but it's much easier to implement and maintain this way. I'm going to come back to this...
Comment on attachment 154605 [details] [review] Bug 31729: enable plackup hot restart in ktd [Alternate] Review of attachment 154605 [details] [review]: ----------------------------------------------------------------- Overall, this is looking pretty good. I think I'd just make 1 tweak. I applied the tweak locally and then I applied some patches using "git bz". I noticed all the regular files (and .git files) were updated and then just restarted the server process 1 time, which was good. ::: debian/scripts/koha-plack @@ +112,4 @@ > fi > > + if [ "$hot_reload" = "yes" ] && [ "$KOHA_INSTANCE" = "kohadev" ] && [ -d "/kohadevbox/koha/Koha" ]; then > + plack_hot_reload="-R /kohadevbox/koha/Koha"; I think this should be /kohadevbox/koha since /kohadevbox/koha/Koha is only a subset of the whole codebase
Created attachment 154676 [details] [review] Bug 31729: [Alternate] enable plackup hot restart in ktd This patch implements a more idiomatoic way for hot-reloading. Instead of adding a new daemon/watcher+inotify or use a Middleware, we can use `plackup -R /path/to/dir`, which is the standard Perl way to do hot reloading for plack apps. Instead of calling `/usr/bin/perl starman` directly, we use `plackup -s Starman`. And can then use `-R /kohadevbox/koha/Koha` to watch that directory for changes and hot-reload the plack app. I've added a new option `koha-plack --hot-reload`. Only if this option is set AND we're running inside koha-testing-docker (by checking KOHA_INSTANCE = "kohadev") hot reloading is enabled. Test plan: Before applying the patch: * Start koha-testing-docker and enter the container * Stop plack: koha-plack --stop kohadev * Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log You should see lines like Worker processes cleaned up Server closing! * Start it again: koha-plack --start kohadev Output should be "Starting Plack daemon for kohadev:" And you end back at the interactive shell * Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log it should look like: Starman::Server (type Net::Server::PreFork) starting! Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock" Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/ * Stop it again: koha-plack --stop kohadev Now apply the patch. As in koha-testing-docker the koha-plack script (after applying the patch) is NOT installed to /sbin you have to call the dev version. * Start the server with hot-reloading: debian/scripts/koha-plack --start --hot-reload kohadev * Output should be: Starting Plack daemon for kohadev:Watching /kohadevbox/koha/Koha /etc/koha/sites/kohadev/plack.psgi for file updates. * You are now NOT back in the shell, as the plack app is NOT running daemonized * So you need to open another ktd --shell * Check the log: tail -n 4 /var/log/koha/kohadev/plack-error.log Starman::Server (type Net::Server::PreFork) starting! pid(27638) Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock" Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/ * Update a code file touch Koha/Biblios.pm * in the other shell (where plack is running) you should see: -- /kohadevbox/koha/Koha/Biblios.pm updated. Killing the existing server Successfully killed! Restarting the new server process. * Yay! * If you want to still stop/restart plack manually, you can hit CTRL-C in the shell were you started plack to stop the process. Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 154677 [details] [review] Bug 31729: [Alternate][follow-up] watch whole codebase Signed-off-by: David Cook <dcook@prosentient.com.au>
Signing off on the "Alternate" patch.
I think we should: 1. test DEV_INSTALL instead of KOHA_INSTANCE, and watch KOHA_HOME instead of /kohadevbox/koha 2. Rename hot_reload with watch (what we use to rebuild JS: `yarn js:watch`) 3. There is a tab character l.457 >-----hot-reload) Then, a question: 4. We are moving from starman to plackup. What do we lose/gain? I know I was using plackup before the move to kohadevbox. And, linking to a 2016 related discussion: 5. Found that from a random guy: https://lists.koha-community.org/pipermail/koha-devel/2016-September/043026.html
(In reply to Jonathan Druart from comment #45) > I think we should: > 1. test DEV_INSTALL instead of KOHA_INSTANCE, and watch KOHA_HOME instead of > /kohadevbox/koha > 2. Rename hot_reload with watch (what we use to rebuild JS: `yarn js:watch`) > 3. There is a tab character l.457 >-----hot-reload) Sounds good to me. > Then, a question: > 4. We are moving from starman to plackup. What do we lose/gain? > I know I was using plackup before the move to kohadevbox. Good point. We should double-check that --max-requests and --workers flow through as expected. I was playing around with plackup and Starman on a different project the other day, and there can be subtle differences with options that don't show obvious warnings/errors. Worth taking another look at that. > And, linking to a 2016 related discussion: > 5. Found that from a random guy: > https://lists.koha-community.org/pipermail/koha-devel/2016-September/043026. > html Also interesting. Personally, I'm not even interested in an automatic filesystem refresh for Plack, so I'm not sure why I'm even still involved in this one. I think I originally just commented so that we didn't go down certain problematic paths [U+1F605]
Maybe it would be easiest to NOT change anything in the koha-plack shell script (which is mostly used for prod, where we don't want the hot reload anyway). But to provided either documentations for koha-testing-docker and/or another wrapper script so people who want hot reloading during dev can start ktd, stop the daemonized koha-plack that's running there per default, and then just start a non-daemonizes hot-reloadable plack for their dev session. This way nothing will change for "normal" installations etc, but devs can still increase their hack/reload/try-time Greetings, domm
Here is a rather simple bash script that will set all the envs etc and start Koha via plackup with hot-reload. I did not provide this as a patch (yet), because I don't think this script should go into the Koha repo, as it's tailored specifically for koha-testing-docker. But I'd still appreciate some feedback (and some hints on where to add this to ktd; or the OK to add it to Koha itself) cat koha-plack-hotreload set -e instancename='kohadev' instance_user="${instancename}-koha" placksocket="/var/run/koha/${instancename}/plack.sock" psgifile="/etc/koha/plack.psgi" export DEV_INSTALL=1 export KOHA_HOME=/kohadevbox/koha export KOHA_CONF="/etc/koha/sites/${instancename}/koha-conf.xml" export PERL5LIB=/kohadevbox/koha:/kohadevbox/koha/lib:/kohadevbox/koha/installer # run plackup with hot reload /usr/bin/plackup -M FindBin -R /kohadevbox/koha --workers 2 --user=${instance_user} --group ${instance_user} -E deployment --socket ${placksocket} -s Starman ${psgifile} Greetings, domm
plackup -R doesn't with --daemonize, could that be a problem? https://github.com/miyagawa/Starman/issues/37
> plackup -R doesn't with --daemonize, could that be a problem? It's certainly going to be a problem for us, as that is how we run Plack.
I think we should NOT apply any of these patches: Hot reload is only needed during dev, and it's much easier to (manually) start koha-plack in a non-demaonized way with hot-reload in Koha-Testing-Docker, than to modify this (IMO slightly crazy) script to start Koha in production. This way we won't cause any problems with prod (where hot-reload should never be active anyway). It's a tiny bit annoying that one first has to start ktd, enter the container, stop the daemonzied version, and then start the hot-reload plack, but as one only has to do this once a day (or once per hack-session..) it's acceptable IMO. See Comment 48 So I suggest we reject these patches and instead add either docs or a small wrapper script to koha-testing-docker.
(In reply to Thomas Klausner from comment #51) > I think we should NOT apply any of these patches: > > Hot reload is only needed during dev, and it's much easier to (manually) > start koha-plack in a non-demaonized way with hot-reload in > Koha-Testing-Docker, than to modify this (IMO slightly crazy) script to > start Koha in production. I am not sure if it's only needed during dev time - we have the issue of updating and installing plugins not taking effect without a Plack restart, which would be great to resolve.
(In reply to Katrin Fischer from comment #52) > (In reply to Thomas Klausner from comment #51) > > I think we should NOT apply any of these patches: > > > > Hot reload is only needed during dev, and it's much easier to (manually) > > start koha-plack in a non-demaonized way with hot-reload in > > Koha-Testing-Docker, than to modify this (IMO slightly crazy) script to > > start Koha in production. > > I am not sure if it's only needed during dev time - we have the issue of > updating and installing plugins not taking effect without a Plack restart, > which would be great to resolve. I might have been on the wrong bug for that one... see bug 31074
Here's what I'm using to start a hot-reload server in koha-testing-docker: ktd --shell kohadev-koha@kohadevbox:koha$ koha-plack --stop kohadev Stopping Plack daemon for kohadev:. kohadev-koha@kohadevbox:koha$ DEV_INSTALL=1 KOHA_HOME=/kohadevbox/koha KOHA_CONF=/etc/koha/sites/kohadev/koha-conf.xml PERL5LIB=/kohadevbox/koha:/kohadevbox/koha/lib:/kohadevbox/koha/installer /usr/bin/plackup -M FindBin -R /kohadevbox/koha --workers 2 --user=kohadev-koha --group=kohadev-koha -E deployment --socket /var/run/koha/kohadev/plack.sock -s Starman /etc/koha/plack.psgi Watching /kohadevbox/koha /etc/koha/plack.psgi for file updates. 2023/11/09-09:15:15 Starman::Server (type Net::Server::PreFork) starting! pid(2344) Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock"
(In reply to Olivier Hubert from comment #50) > > plackup -R doesn't with --daemonize, could that be a problem? > > It's certainly going to be a problem for us, as that is how we run Plack. That would have been with --hot-reload so not relevant for prod.
(In reply to Thomas Klausner from comment #51) > I think we should NOT apply any of these patches: > > Hot reload is only needed during dev, and it's much easier to (manually) > start koha-plack in a non-demaonized way with hot-reload in > Koha-Testing-Docker, than to modify this (IMO slightly crazy) script to > start Koha in production. > > [...] > So I suggest we reject these patches and instead add either docs or a small > wrapper script to koha-testing-docker. Thanks for your manual solution. It would still be great to have hot reload on by default on KTD so it's less error prone for everyone to test patches. (at least the vast majority using KTD) The question is how is it the cleanest. Here in koha-plack. Or with changing the KTD scripts. This ticket has a counter part in the KTD issue tracker: https://gitlab.com/koha-community/koha-testing-docker/-/issues/307
Finally created a pull-req for KTD to add my "fix" to the KTD docs: https://gitlab.com/koha-community/koha-testing-docker/-/merge_requests/492 I still see no easy way to add this to KTD itself, because KTD is calling koha-plack, and we don't want to add hot-reload there.