TGAC / KAT

The K-mer Analysis Toolkit (KAT) contains a number of tools that analyse and compare K-mer spectra.
http://www.earlham.ac.uk/kat-tools
GNU General Public License v3.0
201 stars 52 forks source link

Cannot find scripts during unit tests #102

Closed maplesond closed 6 years ago

kiwifb commented 6 years ago

On develop when doing make check and kat isn't installed anywhere yet, I get

 FAIL: test_sect.sh
FAIL: test_gcp.sh
FAIL: test_hist.sh
FAIL: test_comp.sh
PASS: check_unit_tests

and all failed tests present the same log

../lib/include/kat/kat_fs.hpp(152): Throw in function kat::KatFS::KatFS(const char*)
Dynamic exception type: boost::exception_detail::clone_impl<kat::FileSystemException>
std::exception::what: std::exception
[kat::FileSystemError*] = Could not find suitable directory containing KAT scripts at the expected location: /usr/share/kat/scripts
FAIL test_hist.sh (exit status: 4)

configuration was with --disable-pykat-install.

maplesond commented 6 years ago

That's a strange one. I just cloned a new repo and can't reproduce this.

make  check-TESTS
make[2]: Entering directory '/home/dan/dev/KAT/tests'
make[3]: Entering directory '/home/dan/dev/KAT/tests'
PASS: test_sect.sh
PASS: check_unit_tests
PASS: test_hist.sh
PASS: test_gcp.sh
PASS: test_comp.sh
============================================================================
Testsuite summary for kat 2.4.2
============================================================================
# TOTAL: 5
# PASS:  5
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================
make[3]: Leaving directory '/home/dan/dev/KAT/tests'
make[2]: Leaving directory '/home/dan/dev/KAT/tests'

It really shouldn't be looking in /usr/share/kat/scripts for the scripts at this stage. What's the absolute path to the KAT/tests directory on your system?

kiwifb commented 6 years ago

It is /dev/shm/portage/sci-biology/kat-9999/work/KAT-Release-9999/tests. Naming of top folder is my doing.

maplesond commented 6 years ago

That should be fine... providing src and scripts dir are adjacent. Not sure if this will do it but I've made this change: 6ace7810977755e2b0f6acf0889e166ede469f29

Maybe you could quickly give this a go?

kiwifb commented 6 years ago

Sorry for the delay. Real work is in the way - as befitting my timezone - no changes.

kiwifb commented 6 years ago

Not sure how it passes for you. It may be down to something weird in your libtools. I think the problem is that ../src/kat is not the real executable. As with all libtools generated systems src/kat is a launcher script that will set LD_LIBRARY_PATH and other variables right and then execute the real thing which is src/.libs/kat. So if you are not installed line 135 to 139 of kat_fs.hpp need to be different.

I got the clue by looking at the output generated when running a test with strace

getcwd("/dev/shm/portage/sci-biology/kat-9999/work/KAT-Release-9999/tests", 128) = 66
stat("/dev/shm/portage/sci-biology/kat-9999/work/KAT-Release-9999/src/.libs/kat", {st_mode=S_IFREG|0755, st_size=16203016, ...}) = 0
lstat("/", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat("/dev", {st_mode=S_IFDIR|0755, st_size=3580, ...}) = 0
lstat("/dev/shm", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=120, ...}) = 0
lstat("/dev/shm/portage", {st_mode=S_IFDIR|0775, st_size=120, ...}) = 0
lstat("/dev/shm/portage/sci-biology", {st_mode=S_IFDIR|0775, st_size=60, ...}) = 0
lstat("/dev/shm/portage/sci-biology/kat-9999", {st_mode=S_IFDIR|0755, st_size=360, ...}) = 0
lstat("/dev/shm/portage/sci-biology/kat-9999/work", {st_mode=S_IFDIR|0700, st_size=80, ...}) = 0
lstat("/dev/shm/portage/sci-biology/kat-9999/work/KAT-Release-9999", {st_mode=S_IFDIR|0755, st_size=740, ...}) = 0
lstat("/dev/shm/portage/sci-biology/kat-9999/work/KAT-Release-9999/src", {st_mode=S_IFDIR|0755, st_size=740, ...}) = 0
lstat("/dev/shm/portage/sci-biology/kat-9999/work/KAT-Release-9999/src/.libs", {st_mode=S_IFDIR|0755, st_size=60, ...}) = 0
lstat("/dev/shm/portage/sci-biology/kat-9999/work/KAT-Release-9999/src/.libs/kat", {st_mode=S_IFREG|0755, st_size=16203016, ...}) = 0
stat("/usr/share/kat/scripts", 0x7fffa0411a20) = -1 ENOENT (No such file or directory)
futex(0x7fc8d5f811a0, FUTEX_WAKE_PRIVATE, 2147483647) = 0
write(2, "../lib/include/kat/kat_fs.hpp(15"..., 340../lib/include/kat/kat_fs.hpp(153): Throw in function kat::KatFS::KatFS(const char*)
Dynamic exception type: boost::exception_detail::clone_impl<kat::FileSystemException>
std::exception::what: std::exception
[kat::FileSystemError*] = Could not find suitable directory containing KAT scripts at the expected location: /usr/share/kat/scripts
) = 340
exit_group(4)                           = ?
+++ exited with 4 +++
kiwifb commented 6 years ago

Just did a little experiment

--- a/lib/include/kat/kat_fs.hpp
+++ b/lib/include/kat/kat_fs.hpp
@@ -118,6 +118,9 @@ public:

         // First get the executable directory
         path exe_dir(canonicalExe.parent_path());
+        cout << exe_dir.string() << endl;
+        cout << exe_dir.stem().string() << endl;
+        cout << kat_scripts.string() << endl;

and look at the output of one of the test before crashing

/dev/shm/portage/sci-biology/kat-9999/work/KAT-Release-9999/src/.libs

/usr/share/kat/scripts

stem() may not be the right thing to use here.

kiwifb commented 6 years ago

I have review boost documentation on stem. Their example is that on a file photo.jpg it would return photo. It does return the name of a file without the extension (the function extension returning the extension itself). So you are using it totally wrong and on something like .libs of course it returns nothing.

But cout << exe_dir.filename().string() << endl; returns .libs which is what you intended I believe.

kiwifb commented 6 years ago
diff --git a/lib/include/kat/kat_fs.hpp b/lib/include/kat/kat_fs.hpp
index 507d341..aed1b54 100644
--- a/lib/include/kat/kat_fs.hpp
+++ b/lib/include/kat/kat_fs.hpp
@@ -120,7 +120,7 @@ public:
         path exe_dir(canonicalExe.parent_path());

         // If the KAT_SCRIPTS variable is defined then we are either in an installed location, or running unit tests
-        if (exe_dir.stem().string() == "bin") {
+        if (exe_dir.filename().string() == "bin") {
             // Ok, so we are in a installed location.  Figuring out the scripts directory isn't as straight
             // forward as it may seem because we might have installed to a alternate root.  So wind back the
             // exec_prefix to get to root (or alternate root) directory.
@@ -132,12 +132,12 @@ public:
                 altroot = altroot.parent_path();
             }
             this->scriptsDir = altroot / kat_scripts;
-        } else if (exe_dir.stem().string() == "src") {
+        } else if (exe_dir.filename().string() == ".libs") {
             // Presumably if we are here then we are running the kat executable from the source directory
-            if (exists(exe_dir / "kat.cc")) {
-                this->scriptsDir = exe_dir.parent_path() / "scripts";
+            if (exists(exe_dir.parent_path() / "kat.cc")) {
+                this->scriptsDir = exe_dir.parent_path().parent_path() / "scripts";
             }
-        } else if (exe_dir.stem().string() == "tests") {
+        } else if (exe_dir.filename().string() == "tests") {
             // Presumably if we are here then we are running the kat executable from the source directory
             if (exists(exe_dir / "check_main.cc")) {
                 this->scriptsDir = exe_dir.parent_path() / "scripts";

works for me.

maplesond commented 6 years ago

Ahh, I see. Yes, so I think this might be one of the differences between static and dynamic linking the executable. In my case the actual executable is in the source directory not src/.libs. Anyhow, it's easy enough to accommodate both scenarios: ca063e66e858915b92ca5de2b8d1e9dfd835b3d1

You're right about stem. In the way I've used it for directory names it should work fine but it is confusing. Also filename() is a little confusing too as I'm looking for a directory name, not a filename. Fortunately, there is yet another method called leaf() which is does a similar things so hopefully it's clear.

I should also add that there's no need to apologise about delayed replies. I appreciate all the help. I will apologise in advance though, as I am going to be going on holiday from Saturday and am in the process of changing jobs, so I'll probably be out of touch for at least next week. I'll try to get as much done as possible before then.

maplesond commented 6 years ago

Just realised I missed the .libs part in your changes: c4020a969572b241f13aee9c7608dd7b57c92e42

kiwifb commented 6 years ago

I guess I am a very unixy guy. A folder/directory is just a special file :) which is why filename works but I agree it can be confusing. I am still shocked that stem seem to give you good result with static linking.

I hadn't thought static linking would alter the stuff libtools does but it makes sense. I am just wondering why it still puts rpaths for the building location of the libraries in the executable though. Probably in case totally static is not feasible - a common occurrence without a special toolchain (I used to take care of BlueGene/L and BlueGene/P supercomputers - static linking by default, dynamic hard to achieve, the reverse of a normal linux platform).

kiwifb commented 6 years ago

And we are good on develop branch! You can close the issue (I can't because I didn't open it).

maplesond commented 6 years ago

Fantastic. Thanks for the help @kiwifb!

mmokrejs commented 6 years ago

My congratulations to you both and good luck to Daniel with a new job!