Closed kloczek closed 3 years ago
DanBloomberg
commented
3 years ago If you have 6 cores you may be able to get away with using -j2. But whenever you use more than 1, it is possible to get in a race condition between the threads (or whatever they are), that causes a failure on these tests, perhaps because the 'compare' run happens before the 'generate' run is finished.
BTW, I don't recognize this error message: line 0: warning: iconv failed to convert degree sign
DanBloomberg
commented
3 years ago closing this issue
kloczek
commented
3 years ago So .. there is no bug here?
DanBloomberg
commented
3 years ago WAI.
With j2 you can get a race condition between the different cores. Depends on how many cores you have. Always best to use -j1 (or just leave it off)
kloczek
commented
3 years ago I have have 24 phicicel cores. Bug still is the bug .. even swiped under the carpet of -j1😋
benoit-pierre
commented
1 year ago You could extend genPathname to rewrite /tmp/… paths to $LEPT_TMPDIR/… (similarly to what is done for Windors) so the results of each tests are isolated:
diff --git a/prog/reg_wrapper.sh b/prog/reg_wrapper.sh
index 38502f5..3511c2b 100755
--- a/prog/reg_wrapper.sh
+++ b/prog/reg_wrapper.sh
@@ -45,4 +45,4 @@ case "${TEST_NAME}" in
fi
esac
-exec ${@%${TEST}} /bin/sh -c "cd \"${srcdir}\" && \"${PWD}/\"${TEST} generate && \"${PWD}/\"${TEST} compare"
+exec ${@%${TEST}} /bin/sh -c "export LEPT_TMPDIR=\"${PWD}/${TEST_NAME}.tmp\"; cd \"${srcdir}\" && \"${PWD}/\"${TEST} generate && \"${PWD}/\"${TEST} compare"
diff --git a/src/utils2.c b/src/utils2.c
index aabf288..2a5c213 100644
--- a/src/utils2.c
+++ b/src/utils2.c
@@ -3108,6 +3108,7 @@ genPathname
return (char *)ERROR_PTR("pathout not made", __func__, NULL);
}
+ char *lept_tmpdir = getenv("LEPT_TMPDIR");
#ifdef _WIN32
is_win32 = TRUE;
#endif /* _WIN32 */
@@ -3116,10 +3117,15 @@ size_t size;
* There is no path rewriting on unix, and on win32, we do not
* rewrite unless the specified directory is /tmp or
* a subdirectory of /tmp */
- if (!is_win32 || dirlen < 4 ||
+ if ((!is_win32 && lept_tmpdir == NULL) || dirlen < 4 ||
(dirlen == 4 && strncmp(cdir, "/tmp", 4) != 0) || /* not in "/tmp" */
(dirlen > 4 && strncmp(cdir, "/tmp/", 5) != 0)) { /* not in "/tmp/" */
stringCopy(pathout, cdir, dirlen);
+ } else if (lept_tmpdir != NULL) {
+ stringCopy(pathout, lept_tmpdir, strlen(lept_tmpdir));
+ /* Add the rest of cdir */
+ if (dirlen > 4)
+ stringCat(pathout, size, cdir + 4);
} else { /* Rewrite for win32 with "/tmp" specified for the directory. */
#ifdef _WIN32
l_int32 tmpdirlen;I also had to fix 3 additional issues:
kernel_reg test is trying to open a file that is not generated by itself (so it only works when tests are not isolated and previous tests have been run):diff --git a/prog/kernel_reg.c b/prog/kernel_reg.c
index 997d0d7..a288fe6 100644
--- a/prog/kernel_reg.c
+++ b/prog/kernel_reg.c
@@ -45,7 +45,7 @@ int main(int argc,
char **argv)
{
char *str;
-l_int32 i, j, same, ok, plottype;
+l_int32 i, j, same, ok;
l_float32 sum, avediff, rmsdiff;
L_KERNEL *kel1, *kel2, *kel3, *kel4, *kelx, *kely;
BOX *box;
@@ -222,8 +222,7 @@ L_REGPARAMS *rp;
pixWrite("/tmp/lept/regout/conv2.png", pixt2, IFF_PNG); /* ditto */
regTestCheckFile(rp, "/tmp/lept/regout/conv2.png"); /* 11 */
- plottype = (rp->display) ? GPLOT_PNG : 0;
- pixCompareGray(pixt, pixt2, L_COMPARE_ABS_DIFF, plottype, NULL,
+ pixCompareGray(pixt, pixt2, L_COMPARE_ABS_DIFF, GPLOT_PNG, NULL,
&avediff, &rmsdiff, NULL);
pixp = pixRead("/tmp/lept/comp/compare_gray0.png");
pixaAddPix(pixa, pixp, L_INSERT);numa3_reg test is writing some results in the wrong directory:diff --git a/prog/numa3_reg.c b/prog/numa3_reg.c
index d15cff1..960d333 100644
--- a/prog/numa3_reg.c
+++ b/prog/numa3_reg.c
@@ -70,7 +70,7 @@ L_REGPARAMS *rp;
pixs = pixRead("test8.jpg");
nasy= pixGetGrayHistogramMasked(pixs, NULL, 0, 0, 1);
numaMakeRankFromHistogram(0.0, 1.0, nasy, 350, &nax, &nay);
- pix1 = gplotGeneralPix2(nax, nay, GPLOT_LINES, "/tmp/lept/numa1/rank1",
+ pix1 = gplotGeneralPix2(nax, nay, GPLOT_LINES, "/tmp/lept/numa3/rank1",
"test rank extractor", "pix val", "rank val");
numaDestroy(&nasy);
numaDestroy(&nax);
@@ -86,7 +86,7 @@ L_REGPARAMS *rp;
numaHistogramGetValFromRank(na, rank, &val);
numaAddNumber(nap, val);
}
- pix2 = gplotGeneralPix1(nap, GPLOT_LINES, "/tmp/lept/numa1/rank2",
+ pix2 = gplotGeneralPix1(nap, GPLOT_LINES, "/tmp/lept/numa3/rank2",
"rank value", NULL, NULL);
pixa = pixaCreate(2);
regTestWritePixAndCheck(rp, pix1, IFF_PNG); /* 0 */
@@ -107,19 +107,19 @@ L_REGPARAMS *rp;
* Numa-morphology *
* -------------------------------------------------------------------*/
na = numaRead("lyra.5.na");
- pix1 = gplotGeneralPix1(na, GPLOT_LINES, "/tmp/lept/numa1/lyra1",
+ pix1 = gplotGeneralPix1(na, GPLOT_LINES, "/tmp/lept/numa3/lyra1",
"Original", NULL, NULL);
na1 = numaErode(na, 21);
- pix2 = gplotGeneralPix1(na1, GPLOT_LINES, "/tmp/lept/numa1/lyra2",
+ pix2 = gplotGeneralPix1(na1, GPLOT_LINES, "/tmp/lept/numa3/lyra2",
"Erosion", NULL, NULL);
na2 = numaDilate(na, 21);
- pix3 = gplotGeneralPix1(na2, GPLOT_LINES, "/tmp/lept/numa1/lyra3",
+ pix3 = gplotGeneralPix1(na2, GPLOT_LINES, "/tmp/lept/numa3/lyra3",
"Dilation", NULL, NULL);
na3 = numaOpen(na, 21);
- pix4 = gplotGeneralPix1(na3, GPLOT_LINES, "/tmp/lept/numa1/lyra4",
+ pix4 = gplotGeneralPix1(na3, GPLOT_LINES, "/tmp/lept/numa3/lyra4",
"Opening", NULL, NULL);
na4 = numaClose(na, 21);
- pix5 = gplotGeneralPix1(na4, GPLOT_LINES, "/tmp/lept/numa1/lyra5",
+ pix5 = gplotGeneralPix1(na4, GPLOT_LINES, "/tmp/lept/numa3/lyra5",
"Closing", NULL, NULL);
pixa = pixaCreate(2);
pixaAddPix(pixa, pix1, L_INSERT);
@@ -157,7 +157,7 @@ L_REGPARAMS *rp;
na3 = numaTransform(na2, 0.0, 1.0 / maxval);
numaFindLocForThreshold(na3, 0, &thresh, NULL);
numaAddNumber(na4, thresh);
- snprintf(buf1, sizeof(buf1), "/tmp/lept/numa1/histoplot-%d", hw);
+ snprintf(buf1, sizeof(buf1), "/tmp/lept/numa3/histoplot-%d", hw);
snprintf(buf2, sizeof(buf2), "halfwidth = %d, skip = 20, thresh = %d",
hw, thresh);
pix1 = gplotGeneralPix1(na3, GPLOT_LINES, buf1, buf2, NULL, NULL);
@@ -167,12 +167,12 @@ L_REGPARAMS *rp;
numaDestroy(&na2);
numaDestroy(&na3);
}
- numaWrite("/tmp/lept/numa1/threshvals.na", na4);
- regTestCheckFile(rp, "/tmp/lept/numa1/threshvals.na"); /* 9 */
- L_INFO("writing /tmp/lept/numa1/histoplots.pdf\n", "numa1_reg");
+ numaWrite("/tmp/lept/numa3/threshvals.na", na4);
+ regTestCheckFile(rp, "/tmp/lept/numa3/threshvals.na"); /* 9 */
+ L_INFO("writing /tmp/lept/numa3/histoplots.pdf\n", "numa3_reg");
pixaConvertToPdf(pixa, 0, 1.0, L_FLATE_ENCODE, 0,
"Effect of smoothing on threshold value",
- "/tmp/lept/numa1/histoplots.pdf");
+ "/tmp/lept/numa3/histoplots.pdf");
numaDestroy(&na1);
numaDestroy(&na4);
pixaDestroy(&pixa);getFilenamesInDirectory needs to call genPathname before realpath, not the other way around:diff --git a/src/sarray1.c b/src/sarray1.c
index c04079d..a9e876e 100644
--- a/src/sarray1.c
+++ b/src/sarray1.c
@@ -1877,12 +1877,8 @@ SARRAY *saout;
SARRAY *
getFilenamesInDirectory(const char *dirname)
{
-#if _POSIX_VERSION >= 200112 || defined(__GLIBC__)
-char *dir;
-#else
char dir[PATH_MAX + 1];
-#endif
-char *realdir, *stat_path, *ignore;
+char *realdir, *stat_path, *gendir;
size_t size;
SARRAY *safiles;
DIR *pdir;
@@ -1898,34 +1894,13 @@ struct stat st;
/* Who would have thought it was this fiddly to open a directory
and get the files inside? fstatat() works with relative
directory paths, and stat() requires using the absolute path.
- realpath works as follows for files and directories:
- * If the file or directory exists, realpath returns its path;
- else it returns NULL.
- * If the second arg to realpath is passed in, the canonical path
- is returned there. Use a buffer of sufficient size.
- We pass in a buffer for the second arg, and check that the
- canonical directory path was made. The existence of the
- directory is checked later, after its actual path is returned by
- genPathname().
- With GNU libc or Posix 2001, if the second arg is NULL, the path
- is malloc'd and returned if the file or directory exists.
*/
-#if _POSIX_VERSION >= 200112 || defined(__GLIBC__)
- dir = realpath(dirname, NULL);
- if (dir == NULL)
- return (SARRAY *)ERROR_PTR("dir not made", __func__, NULL);
-#else
- dir[0] = '\0'; /* init empty in case realpath() fails to write it */
- ignore = realpath(dirname, dir);
- if (dir[0] == '\0')
+ gendir = genPathname(dirname, NULL);
+ realdir = realpath(gendir, dir);
+ LEPT_FREE(gendir);
+ if (realdir == NULL)
return (SARRAY *)ERROR_PTR("dir not made", __func__, NULL);
-#endif
- realdir = genPathname(dir, NULL);
-#if _POSIX_VERSION >= 200112 || defined(__GLIBC__)
- LEPT_FREE(dir);
-#endif
if ((pdir = opendir(realdir)) == NULL) {
- LEPT_FREE(realdir);
return (SARRAY *)ERROR_PTR("pdir not opened", __func__, NULL);
}
safiles = sarrayCreate(0);
@@ -1953,7 +1928,6 @@ struct stat st;
sarrayAddString(safiles, pdirentry->d_name, L_COPY);
}
closedir(pdir);
- LEPT_FREE(realdir);
return safiles;
}
Happy to PR all/some of those changes if you're interested.
DanBloomberg
commented
1 year ago Thank you for doing all that, Pierre!
I've made the changes in 3 of your 4 suggestions. fd5da1c 3c6142b
The getFilenamesInDirectory() simplification is nice. I decided to require the POSIX 2008 non-buggy version of realpath(), where the path is malloc'd. With luck there will not be complaints.
A fix for the regression test fpix1_reg is actually long overdue -- that test was displaying a plot even though no displays are requested. Removed it. This plot image was also linked to the sequencing bug you found in kernel_reg; that's all removed.
The suggestion not implemented is the genPathname() rewrite for windows with a special environment variable. I'm not convinced there is a problem to be solved. BTW, for stringCopy() you must limit the copy using strlen(pathout), the buffer size.
benoit-pierre
commented
1 year ago Thank you for doing all that, Pierre!
I've made the changes in 3 of your 4 suggestions. fd5da1c 3c6142b
Great!
[…]
The suggestion not implemented is the genPathname() rewrite for windows with a special environment variable. I'm not convinced there is a problem to be solved.
It would make running the testsuite much faster (~4x faster on my old desktop, YMMV).
I also think it might fix the problem #561 is trying to address, which I'm pretty sure I hit too here, when adding leptonica support to Meson's WrapDB. I'm happy to contribute that support here BTW, if you want.
BTW, for stringCopy() you must limit the copy using strlen(pathout), the buffer size.
You mean to avoid an overflow if strlen(lept_tmpdir) >= size?
DanBloomberg
commented
1 year ago How can this affect the running time? Have you compared the time on windows vs linux?
stringCopy: just the usual thing of limiting the number of copied bytes to the size of the buffer (not the size of the source string)
benoit-pierre
commented
1 year ago How can this affect the running time?
Because each test temporary directory can be isolated by setting LEPT_TMPDIR in prog/reg_wrapper.sh, thus allowing executing them in parallel.
See #708.
Have you compared the time on windows vs linux?
Ditto, see #708.
stweil
commented
1 year ago The function ioFormatTest internally uses filenames with fixed names which don't allow parallel usage of that function, so this is one reason why running tests in parallel currently can fail.
benoit-pierre
commented
1 year ago Here is the full list of temporary paths common to multiple tests:
/tmp/lept: all tests
/tmp/lept/boxa: boxa1, boxa2, boxa4
/tmp/lept/comp: compare, ioformats, kernel, pixcomp, pngio
/tmp/lept/format: ioformats, pngio
/tmp/lept/format/file.bmp: ioformats, pngio
/tmp/lept/format/file_g3.tif: ioformats, pngio
/tmp/lept/format/file_g4.tif: ioformats, pngio
/tmp/lept/format/file.gif: ioformats, pngio
/tmp/lept/format/file.jp2: ioformats, pngio
/tmp/lept/format/file_jpeg.tif: ioformats, pngio
/tmp/lept/format/file.jpg: ioformats, pngio
/tmp/lept/format/file_lzw.tif: ioformats, pngio
/tmp/lept/format/file_packbits.tif: ioformats, pngio
/tmp/lept/format/file.png: ioformats, pngio
/tmp/lept/format/file.pnm: ioformats, pngio
/tmp/lept/format/file_rle.tif: ioformats, pngio
/tmp/lept/format/file.tif: ioformats, pngio
/tmp/lept/format/file.webp: ioformats, pngio
/tmp/lept/format/file_zip.tif: ioformats, pngio
/tmp/lept/golden: all tests
/tmp/lept/gplot: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.0.cmd: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.0.data.1: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.0.png: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.10.cmd: projection, rankhisto
/tmp/lept/gplot/pix1.10.data.1: projection, rankhisto
/tmp/lept/gplot/pix1.10.png: projection, rankhisto
/tmp/lept/gplot/pix1.11.cmd: projection, rankhisto
/tmp/lept/gplot/pix1.11.data.1: projection, rankhisto
/tmp/lept/gplot/pix1.11.png: projection, rankhisto
/tmp/lept/gplot/pix1.12.cmd: projection, rankhisto
/tmp/lept/gplot/pix1.12.data.1: projection, rankhisto
/tmp/lept/gplot/pix1.12.png: projection, rankhisto
/tmp/lept/gplot/pix1.13.cmd: projection, rankhisto
/tmp/lept/gplot/pix1.13.data.1: projection, rankhisto
/tmp/lept/gplot/pix1.13.png: projection, rankhisto
/tmp/lept/gplot/pix1.1.cmd: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.1.data.1: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.1.png: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.2.cmd: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.2.data.1: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.2.png: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.3.cmd: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.3.data.1: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.3.png: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.4.cmd: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.4.data.1: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.4.png: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.5.cmd: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.5.data.1: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.5.png: crop, projection, rankbin, rankhisto
/tmp/lept/gplot/pix1.6.cmd: projection, rankhisto
/tmp/lept/gplot/pix1.6.data.1: projection, rankhisto
/tmp/lept/gplot/pix1.6.png: projection, rankhisto
/tmp/lept/gplot/pix1.7.cmd: projection, rankhisto
/tmp/lept/gplot/pix1.7.data.1: projection, rankhisto
/tmp/lept/gplot/pix1.7.png: projection, rankhisto
/tmp/lept/gplot/pix1.8.cmd: projection, rankhisto
/tmp/lept/gplot/pix1.8.data.1: projection, rankhisto
/tmp/lept/gplot/pix1.8.png: projection, rankhisto
/tmp/lept/gplot/pix1.9.cmd: projection, rankhisto
/tmp/lept/gplot/pix1.9.data.1: projection, rankhisto
/tmp/lept/gplot/pix1.9.png: projection, rankhisto
/tmp/lept/jb: italic, wordboxes
/tmp/lept/jb/diffcc.cmd: italic, wordboxes
/tmp/lept/jb/diffcc.data.1: italic, wordboxes
/tmp/lept/jb/diffcc.png: italic, wordboxes
/tmp/lept/jb/numcc.cmd: italic, wordboxes
/tmp/lept/jb/numcc.data.1: italic, wordboxes
/tmp/lept/jb/numcc.png: italic, wordboxes
/tmp/lept/plots: boxa2, boxa3, boxa4
/tmp/lept/plots/sides.0.cmd: boxa3, boxa4
/tmp/lept/plots/sides.0.data.1: boxa3, boxa4
/tmp/lept/plots/sides.0.data.2: boxa3, boxa4
/tmp/lept/plots/sides.0.data.3: boxa3, boxa4
/tmp/lept/plots/sides.0.data.4: boxa3, boxa4
/tmp/lept/plots/sides.0.png: boxa3, boxa4
/tmp/lept/plots/sides.1.cmd: boxa3, boxa4
/tmp/lept/plots/sides.1.data.1: boxa3, boxa4
/tmp/lept/plots/sides.1.data.2: boxa3, boxa4
/tmp/lept/plots/sides.1.data.3: boxa3, boxa4
/tmp/lept/plots/sides.1.data.4: boxa3, boxa4
/tmp/lept/plots/sides.1.png: boxa3, boxa4
/tmp/lept/plots/sides.2.cmd: boxa3, boxa4
/tmp/lept/plots/sides.2.data.1: boxa3, boxa4
/tmp/lept/plots/sides.2.data.2: boxa3, boxa4
/tmp/lept/plots/sides.2.data.3: boxa3, boxa4
/tmp/lept/plots/sides.2.data.4: boxa3, boxa4
/tmp/lept/plots/sides.2.png: boxa3, boxa4
/tmp/lept/plots/sides.3.cmd: boxa3, boxa4
/tmp/lept/plots/sides.3.data.1: boxa3, boxa4
/tmp/lept/plots/sides.3.data.2: boxa3, boxa4
/tmp/lept/plots/sides.3.data.3: boxa3, boxa4
/tmp/lept/plots/sides.3.data.4: boxa3, boxa4
/tmp/lept/plots/sides.3.png: boxa3, boxa4
/tmp/lept/plots/sides.4.cmd: boxa3, boxa4
/tmp/lept/plots/sides.4.data.1: boxa3, boxa4
/tmp/lept/plots/sides.4.data.2: boxa3, boxa4
/tmp/lept/plots/sides.4.data.3: boxa3, boxa4
/tmp/lept/plots/sides.4.data.4: boxa3, boxa4
/tmp/lept/plots/sides.4.png: boxa3, boxa4
/tmp/lept/plots/sides.5.cmd: boxa3, boxa4
/tmp/lept/plots/sides.5.data.1: boxa3, boxa4
/tmp/lept/plots/sides.5.data.2: boxa3, boxa4
/tmp/lept/plots/sides.5.data.3: boxa3, boxa4
/tmp/lept/plots/sides.5.data.4: boxa3, boxa4
/tmp/lept/plots/sides.5.png: boxa3, boxa4
/tmp/lept/plots/size.0.cmd: boxa3, boxa4
/tmp/lept/plots/size.0.data.1: boxa3, boxa4
/tmp/lept/plots/size.0.data.2: boxa3, boxa4
/tmp/lept/plots/size.0.png: boxa3, boxa4
/tmp/lept/plots/size.1.cmd: boxa3, boxa4
/tmp/lept/plots/size.1.data.1: boxa3, boxa4
/tmp/lept/plots/size.1.data.2: boxa3, boxa4
/tmp/lept/plots/size.1.png: boxa3, boxa4
/tmp/lept/plots/size.2.cmd: boxa3, boxa4
/tmp/lept/plots/size.2.data.1: boxa3, boxa4
/tmp/lept/plots/size.2.data.2: boxa3, boxa4
/tmp/lept/plots/size.2.png: boxa3, boxa4
/tmp/lept/plots/size.3.cmd: boxa3, boxa4
/tmp/lept/plots/size.3.data.1: boxa3, boxa4
/tmp/lept/plots/size.3.data.2: boxa3, boxa4
/tmp/lept/plots/size.3.png: boxa3, boxa4
/tmp/lept/plots/size.4.cmd: boxa3, boxa4
/tmp/lept/plots/size.4.data.1: boxa3, boxa4
/tmp/lept/plots/size.4.data.2: boxa3, boxa4
/tmp/lept/plots/size.4.png: boxa3, boxa4
/tmp/lept/plots/size.5.cmd: boxa3, boxa4
/tmp/lept/plots/size.5.data.1: boxa3, boxa4
/tmp/lept/plots/size.5.data.2: boxa3, boxa4
/tmp/lept/plots/size.5.png: boxa3, boxa4
/tmp/lept/regout: all tests
/tmp/lept/regout/regtest_output.txt: all tests
/tmp/lept/reg_results.txt: all tests
/tmp/lept/windowed/plotsizes2.png: boxa4, boxa5
Generated after running the tests (using #708) and the following command:
(
for d in prog/*.tmp; do
t=${d##*/};
t=${t%.tmp};
find $d -mindepth 1 -printf '%P\n' | sed "s,^,$t /tmp/,";
done | sort -k2 -t' ';
echo
) | awk -f tmpfiles.awktmpfiles.awk:
{
if (path != $2) {
if (count > 1)
print "`"path"`:", (count == 147 ? "all tests" : tests)
path = $2
count = 0
tests = ""
}
count += 1
tests = (count > 1 ? tests", " : "") $1
}
As long as test suite is started with
-j1everything is OK.