PhilterPaper / Perl-PDF-Builder

Extended version of the popular PDF::API2 Perl-based PDF library for creating, reading, and modifying PDF documents
https://www.catskilltech.com/FreeSW/product/PDF%2DBuilder/title/PDF%3A%3ABuilder/freeSW_full
Other
6 stars 7 forks source link

tests: Write files to a temporary directory #153

Closed ppisar closed 3 years ago

ppisar commented 3 years ago

t/tiff.t fails if run from a read-only location:

t/tiff.t .................... convert: unable to open image `test.tif': Permission denied @ error/blob.c/OpenBlob/2924.
Unable to open test.pdf for writing at /usr/share/perl5/vendor_perl/PDF/Builder/Basic/PDF/File.pm line 439.
# Looks like your test exited with 13 just after 8.
t/tiff.t .................... Dubious, test returned 13 (wstat 3328, 0xd00)
Failed 5/13 subtests

That's because the test creates many temporary files. This patch fixes it by placing the files into a temporary directory which is automatically cleaned up on exit.

It will also help when runnig the tests in parallel. The tests won't trip on the same-named files.

PhilterPaper commented 3 years ago

Just one question: I see you are now creating a new directory and the new (temporary) files within it, in order to avoid duplicate names/read-only problems. Is this new directory and its contents guaranteed to disappear when t/tiff.t ends, on ALL operating systems, or is a cleanup (file erase) needed at any time? I see that you removed the explicit cleanup at the end. I can test it with Windoze, but have no idea about the behavior on other OSs. If it's not a sure thing, I assume I could explicitly remove the temp files and then the temp directory?

The original problem, that this might be running in a R/O location, is an interesting one. Is it something that only the t-tests need worry about? I should take a look through at least the t-tests to see if there are other files created. How about examples/ and contrib/ directories -- might anyone be running them in a R/O environment? They all have output files that need to be kept around.

ppisar commented 3 years ago

V Mon, Mar 29, 2021 at 08:32:31AM -0700, Phil Perry napsal(a):

Just one question: I see you are now creating a new directory and the new (temporary) files within it, in order to avoid duplicate names. Is this new directory and its contents guaranteed to disappear when t/tiff.t ends, on ALL operating systems, or is a cleanup (file erase) needed at any time?

File::Temp->newdir() documention reads:

By default the directory is deleted when the object goes out of
scope.

That also means on exit. The cleanup is implemented in a destructor of the File::Temp object.

CLEANUP => 1 at File::Temp::tempdir() reads:

Create a temporary directory using the supplied template, but
attempt to remove it (and all files inside it) when the program
exits. Note that an attempt will be made to remove all files from
the directory even if they were not created by this module
(otherwise why ask to clean it up?)

The CLEANUP => 1 is an implicit option for File::Temp->newdir().

Since File::Temp tests are performed on all operating systems equally, I believe this is true for all operating system.

The original problem, that this might be running in a R/O location, is an interesting one. Is it something that only the t-tests need worry about? I should take a look through at least the t-tests to see if there are other files created. How about examples/ and contrib/ directories -- might anyone be running them in a R/O environment? They all have output files that need to be kept around.

My use case is packaging software into RPM format. Software is installed into a location not writable for an ordinary user (e.g. /usr/share/perl5). That's a no-brainer and everybody is used to it.

I also package a documentation (examples/). There is no need to adjust the documentation. It's not executed from that location. Users know that it's intended for reading. Not for execution. At the end they can copy it somewhere else and execute it from there. Actually complicating the documentation with temporary directories would make the documentation less obvious.

But recently I also started packaging the tests (./t into e.g. /usr/libexec/perl-PDF-Builder). The location is also read-only and when I tried executed them, only the t/tiff.t failed. So I believe that t/tiff is the only test which writes and I corrected it with the patch.

My motivation for packaging the tests is that you can perform the tests (long) after you have installed the software and verify that the installed software still works. That hash reality is that test performed at build time (make test) validates a code in ./blib. Not the code installed after that (with "make install") which is then used in production. Also a user can upgrade (or downgrade) a dependency of the code (e.g. Graphics::TIFF) after the installation and that can affect the code.

But the whole think with temporary directories has also another benefit: You can perform the tests in parallel (HARNESS_OPTIONS=j10 make test) without woring that one test will reuse the same file another test is right now using. That's technically not necessary for PDF-Builder now because the only writing test is t/tiff.t, but that could change in the future. Running the tests in parallel speeds up development because it can leverage multicore processors we have in our computeres and shorten the validation time. I belive that PDF-Builder will benefir from it even if you are not going to run the tests from a read-only location.

PhilterPaper commented 3 years ago

Thank you for the update, and the explanation. It has been merged. I was a bit concerned about deleting temporary files, as I often see error messages when updating other packages that the system was unable to delete this or that file. Maybe they're not using the same mechanism that you are. Anyway, it appears to work fine in Windoze, and I'll assume that you've run it on at least one other flavor of Linux-like system.