Bug 6048 - printoverdues.sh bash odities
Summary: printoverdues.sh bash odities
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: 3.4
Hardware: All All
: PATCH-Sent (DO NOT USE) trivial (vote)
Assignee: Jonathan Druart
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-04 14:13 UTC by Alex Arnaud
Modified: 2013-12-05 20:06 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
fix printoverdues.sh bash odities (715 bytes, patch)
2011-04-04 14:16 UTC, Alex Arnaud
Details | Diff | Splinter Review
Modify all variables set (1.12 KB, patch)
2011-04-07 15:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6048: printoverdues.sh bash odities (1.27 KB, patch)
2011-09-02 03:26 UTC, Robin Sheat
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2011-04-04 14:13:00 UTC

    
Comment 1 Alex Arnaud 2011-04-04 14:16:28 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2011-04-07 15:11:23 UTC Comment hidden (obsolete)
Comment 3 Joe Atzberger 2011-06-08 15:32:28 UTC
$ misc/cronjobs/printoverdues.sh ~/Downloads/
/usr/lib/pymodules/python2.6/pyPdf/pdf.py:52: DeprecationWarning: the sets module is deprecated
  from sets import ImmutableSet
Converting /home/atz/Downloads//2_factor.html to /home/atz/Downloads/2_factor.pdf...
tar: Cowardly refusing to create an empty archive
Try `tar --help' or `tar --usage' for more information.

Looks like problems with the tar targetting.

$directory is never set to anything (should be $1).

This script needs a lot more error checking.  For example, we shouldn't be tarring or printing ANYTHING if xhtml2pdf has failed or returned error.  This is pretty likely to happen since the script requires an entirely new language dependency and a large package in that language.

Command line options are also illogically dependent.  We shouldn't have to specify a CSS file in order to target print lp3.  

However, the actual patch is OK, it's just the script as a whole needs work.  Let me know if you want me to proceed with broader fixes.
Comment 4 Joe Atzberger 2011-06-08 18:39:41 UTC
We shouldn't be tarring the whole directory every time, and CERTAINLY not putting the gzipped tar inside the same directory if we are.  Otherwise we'd end up with recursive backup sets where every new backup contains all the other backup sets.  

This is the wrong way to structure a backup because you will get WORSE compression combining already compressed files and because we would be creating a whole lot of duplicate data by always containing yesterday's file (which contains the day before which contains the day before, etc.).

Ideally, we'd invoke something like logrotate, but short of that, we should only backup the HTMLs and/or PDFs that we operated on during this execution.  If xhtml2pdf wasn't so obtuse, we would be able to pass it multiple source file arguments directly (rather than an uninterpolated pseudo-glob), and therefore also accept them with this script (rather than a dir).  

So the question is: do we want to tar/gzip the HTML, the PDFs or both?  They are ostensibly the same data, so having both is unnecessary, afaict.
Comment 5 Katrin Fischer 2011-08-14 15:12:04 UTC
Joe, 
I don't understand everything from your comments - is this a sign-off but still needs improvement or a failed qa?
Comment 6 Robin Sheat 2011-09-02 03:26:28 UTC
Created attachment 5269 [details] [review]
Bug 6048: printoverdues.sh bash odities

Signed-off-by: Robin Sheat <robin@catalyst.net.nz>

Note: this script really needs a rewrite, but this patch does fix up the
things it's supposed to fix up.
Comment 7 Paul Poulain 2011-10-06 14:51:22 UTC
QA comment :
* assigning this bug  to Jonathan, that wrote the patch
* small script, that fixes some bash complaints
* isolated script, no consequences on the rest of Koha

The script should all be rewritten, but the patch fixes the warnings, so marking passed QA
Comment 8 Chris Cormack 2011-10-07 01:22:03 UTC
Pushed, please test
Comment 9 Jared Camins-Esakov 2012-12-31 00:07:02 UTC
There have been no further reports of problems so I am marking this bug resolved.