SWI-Prolog / packages-jpl

JPL: The Prolog <-> Java interface
BSD 2-Clause "Simplified" License
54 stars 33 forks source link

JPL unit test jpl:prolog_in_java #32

Closed ssardina closed 4 years ago

ssardina commented 5 years ago

The unit test jpl:prolog_in_java seems to give errors but pass anyways:

After building the whole SWI+JPL, from build/:

[ssardina@Thinkpad-X1 build]$ ctest -V -R jpl:prolog_in_java
UpdateCTestConfiguration  from :/home/ssardina/git/soft/prolog/swipl-devel.git/build/DartConfiguration.tcl
UpdateCTestConfiguration  from :/home/ssardina/git/soft/prolog/swipl-devel.git/build/DartConfiguration.tcl
Test project /home/ssardina/git/soft/prolog/swipl-devel.git/build
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 48
    Start 48: jpl:prolog_in_java

48: Test command: /usr/bin/env "SWI_HOME_DIR=../../home" "SWIPL_BOOT_FILE=../../home/boot.prc" "TEST_JPL=../../../packages/jpl/test_jpl.pl" "/usr/lib/jvm/java-8-oracle/bin/java" "-Djava.library.path=." "-classpath" "/usr/share/java/junit.jar:src/java/jpl.jar:src/java/jpltest.jar" "junit.textui.TestRunner" "org.jpl7.test.TestJUnit"
48: Test timeout computed to be: 9.99988e+06
48: ERROR: /home/ssardina/git/soft/prolog/swipl-devel.git/build/home/library/filesex.pl:80:
48:     /home/ssardina/git/soft/prolog/swipl-devel.git/build/home/library/filesex.pl:80: Initialization goal raised exception:
48:     '$open_shared_object'/3: files: cannot open shared object file: No such file or directory
48: ERROR: /home/ssardina/git/soft/prolog/swipl-devel.git/packages/jpl/jpl.pl:4318:
48:     Exported procedure files_ex:link_file/3 is not defined
48: ERROR: /home/ssardina/git/soft/prolog/swipl-devel.git/packages/jpl/jpl.pl:4318:
48:     Exported procedure files_ex:set_time_file/3 is not defined
48: .........................................
48: .........................................
48: ......................................
48: Time: 0.063
48: 
48: OK (120 tests)
48: 
1/1 Test #48: jpl:prolog_in_java ...............   Passed    0.78 sec

The following tests passed:
    jpl:prolog_in_java

100% tests passed, 0 tests failed out of 1

Total Test time (real) =   0.79 sec

One can go into the JPL directory and do the test manually as follows:

ssardina@Thinkpad-X1 jpl]$ pwd
/home/ssardina/git/soft/prolog/swipl-devel.git/build/packages/jpl
[ssardina@Thinkpad-X1 jpl]$ /usr/bin/env "SWI_HOME_DIR=../../home" "SWIPL_BOOT_FILE=../../home/boot.prc" "TEST_JPL=../../../packages/jpl/test_jpl.pl" "/usr/lib/jvm/java-8-oracle/bin/java" "-Djava.library.path=." "-classpath" "/usr/share/java/junit.jar:src/java/jpl.jar:src/java/jpltest.jar" "junit.textui.TestRunner" "org.jpl7.test.TestJUnit"
ERROR: /home/ssardina/git/soft/prolog/swipl-devel.git/build/home/library/filesex.pl:80:
    /home/ssardina/git/soft/prolog/swipl-devel.git/build/home/library/filesex.pl:80: Initialization goal raised exception:
    '$open_shared_object'/3: files: cannot open shared object file: No such file or directory
ERROR: /home/ssardina/git/soft/prolog/swipl-devel.git/packages/jpl/jpl.pl:4318:
    Exported procedure files_ex:set_time_file/3 is not defined
ERROR: /home/ssardina/git/soft/prolog/swipl-devel.git/packages/jpl/jpl.pl:4318:
    Exported procedure files_ex:link_file/3 is not defined
.........................................
.........................................
......................................
Time: 0.057

OK (120 tests)
JanWielemaker commented 5 years ago

You did not or incompletely build the clib package. At least, that is where the missing file should come from and I do not get this error.

ssardina commented 5 years ago

mmm I am pretty sure I compiled all modules, but I would double check everything again. Thanks for giving me a useful pointer to explore.

ssardina commented 4 years ago

I am still having issues with this error when I ran the test. However, clib package seems to be compiled well and passes all tests:

ctest -V -R clib

...
he following tests passed:
    clib:cgi
    clib:crypt
    clib:memfile
    clib:process
    clib:readutil
    clib:socket
    clib:stream
    clib:time
    clib:uri

The file /home/ssardina/git/soft/prolog/swipl-devel.git/build/home/library/filesex.pl does exist:

:- module(files_ex,
          [ set_time_file/3,            % +File, -OldTimes, +NewTimes
            link_file/3,                % +OldPath, +NewPath, +Type
            chmod/2,                    % +File, +Mode
            relative_file_name/3,       % ?AbsPath, +RelTo, ?RelPath
            directory_file_path/3,      % +Dir, +File, -Path
            directory_member/3,         % +Dir, -Member, +Options
            copy_file/2,                % +From, +To
            make_directory_path/1,      % +Directory
            copy_directory/2,           % +Source, +Destination
            delete_directory_and_contents/1, % +Dir
            delete_directory_contents/1 % +Dir
          ]).

This requires further investigation, just wanted to document this here.

JanWielemaker commented 4 years ago

Loading depends on the foreign file packages/clib/files.so Somehow that seems missing or not loadable. I didn't hear about this issue from anyone else which suggests it could be some local twist. Anyway, it is irrelevant to JPL as the tests only seem to use the pure Prolog predicates of this library.

ssardina commented 4 years ago

Sorry, I got carried away while I was trying to design test for the Rational module I am building. I realized the Java unit testing was in the old version 3, so I migrated it all to version 4, which took a while as I had to learn the whole thing (both on the SWI CMake setup and using JUnit). That is issue #40 which is somehow related to this one as well as it is about testing. I want to clean it up fully, I am close.

Regarding this error, yes yes I can see it is not that serious, but I don't like seeing the word "ERROR" in my console... ;-)

Strangely, I do have clib fully compiled under packages/ with the files.so there, and I can even load the problematic module in SWI. Anyway will investigate further later...

[ssardina@Thinkpad-X1 build-junit44]$ swipl
Welcome to SWI-Prolog (threaded, 64 bits, version 8.1.28-DIRTY)
SWI-Prolog comes with ABSOLUTELY NO WARRANTY. This is free software.
Please run ?- license. for legal details.

    CMake built from "/home/ssardina/git/soft/prolog/swipl-devel.git/cmake"

For online help and background, visit https://www.swi-prolog.org
For built-in help, use ?- help(Topic). or ?- apropos(Word).

?- use_module(library(filesex)).
true.

?- ^D
% halt

[ssardina@Thinkpad-X1 build-junit44]$ l packages/clib/files.so 
-rwxrwxr-x 1 ssardina ssardina 108K Apr 14 17:32 packages/clib/files.so
ssardina commented 4 years ago

@JanWielemaker , there has to be something strange here when running SWI from the development dir (not the installed one in the system) on tests that need to use jpl.pl which access the foreign libraries.

The JPL tests run on the build/package/jpl dir, where libjpl.so is placed and LD_LIBRARY_PATH will have . on it, so that is found. But if a test needs files.so it cannot find it.

However, if I manually create a create a lib/ subdir in build/home and out there a symbolic link

files.so -> ../../packages/clib/files.so

then all works!

Also works if I made that symbolic link from package/jpl where the test runs.

You say you don't see any error, but do you run the test with VERBOSE? Because the tests passed, but the error is thrown in the middle. The tests are not so well done to fail when a file fails to load (I am fixing that!), they pass anyway!

It is interesting there is no home/lib dir with pointers to all the .so, but somehow if I make them, SWI will look there...

The error I get is:

62: ERROR:    '$open_shared_object'/3: files: cannot open shared object file: No such file or directory
62: ERROR: /home/ssardina/git/soft/prolog/swipl-devel.git/packages/jpl/jpl.pl:4332:
JanWielemaker commented 4 years ago

Are we talking java-in-prolog or prolog-in-java?

library(filesex) is used by jpl.pl for add_jpl_to_classpath/0, but that part of the library doesn't require the foreign extension, which is why using the library should not be a problem (just leads to a warning).

In the build tree loads home/swipl.rc, which changes the file search paths to find the foreign resources. I guess the tests should not use -F none, which blocks this.

Somehow, ctest -V -R java doesn't raise any warnings for me though.

ssardina commented 4 years ago

Thanks Jan for following it up. I am very curious now...

The problem is jpl:prolog_in_java and indeed when loading library jpl.pl in the add_jpl_to_classpath predicate, but it seems it all comes to that file swipl.rc file and adding the search paths for each package installed: it seems not to be working.

We are not using the -F at all, but we are using -f none. For the tests SWI is being called from build/packages/jpl dir with

SWI_HOME_DIR=../../home SWIPL_BOOT_FILE=../../home/boot.prc

and with the following options:

libswipl.dll -x ../../home/boot.prc -f none -g true -q --home=../../home --no-signals --no-packs

If I ran the local build version manually from CLI in build/it does update all search paths:

src/swipl -x home/boot.prc -f none --home=home/ --no-signals

However, when Prolog is called inside Java it does not. I seem to have found two issues:

  1. As the tests are running so far (from old JPL setup maybe), swirc.pl is not loading at all. Tests go on even I delete that file completely. That may work well when doing simple queries that do not require any search path, but when it tries to load something it may not have the search paths as you intended.

  2. I can force loading that by changing the -f none to -f ../../home/swipl.rc inside the Java calls, is this correct?

  3. Now swipl.rc is loading but paths are not being updated and this seems to be because tests are passing the libswipl.dll and cmake_binary_directory/1 files in those cases due to this line:

    OsExe \== 'libswipl.dll',           % avoid dummy for embedded JPL test

So, at this point swipl.rc does not load by default it seems and search paths are not set for packages if libswipl.dll is used. No idea what "avoid dummy" is meant to mean here.

Investigating....

ssardina commented 4 years ago

OK, I SOLVED IT!! I think I am starting to understand a bit better the whole SWI framework, it's really impressive system!

I studied build_home.pl and realized that I needed to use the executable file src/swipl, have no idea why it was using libswipl.dll, but I guess it comes from old code of someone in Windows and it was the non-CMAKE time..

Anyway, I figured it out and now I setup the following to start SWI within Java:

./../home/../src/swipl -x ../../home/boot.prc -f ../../home/swipl.rc -g true -q --home=../../home --no-signals --no-packs

So I need to explicitly load -f ../../home/swipl.rc and pass the executable ./../home/../src/swipl

Really happy now and did a huge clean up of the unit testing and incorporated many tests I have built when I worked on this a year ago that were not part of the official testing. Now the testing set is a Test Suite in JUnit 4 and works great. All in 5ec0ee5c3c42426691888aca2dead51e0160150f

Now I can focus back on Rational.

JanWielemaker commented 4 years ago

Good catch. Only, the fix may give problems on Windows. The argv[0] doesn't matter too much, except for finding the right file for -F, so using libswipl.dll it will look for libswipl.rc rather than swipl.rc.

I think the clean solution is to go back to the original and add -F swipl to the flags. All other flags have recently been updated (including the new --no-packs to avoid interaction with user packs).

JanWielemaker commented 4 years ago

More junit issues: Macport installs /opt/local/share/java/junit.jar which is in fact junit 4.13. That might be the case on more platforms. Is there an easy way to find out the junit version in some jar?

ssardina commented 4 years ago

More junit issues: Macport installs /opt/local/share/java/junit.jar which is in fact junit 4.13. That might be the case on more platforms. Is there an easy way to find out the junit version in some jar?

Ohh Mac, and Macport...

OK we can add any known path, that's easy and should work fairly robust right?

In principle, one can find jar files first, and open them via unzip to extract META-INF/MANIFEST.MF and it has the Implementation-Version: 4.12. Now all that in CMAKE, should be possible (I guess CMAKE can call scripts), but too much infrastructure for such a standard package right? What if there are many JARs, etc..?

ssardina commented 4 years ago

Good catch. Only, the fix may give problems on Windows. The argv[0] doesn't matter too much, except for finding the right file for -F, so using libswipl.dll it will look for libswipl.rc rather than swipl.rc.

OK this is more serious and I do want a robust & good solution here. (Un)fortunately I don't have Windows (or Mac!)

So regarding argv[0], we really don't need a "flag" to mark the OS, we can get the OS in java and decide what we need to execute. even from Java using System.getProperty("os.name")

As it was, libswipl.dll was passed always, which makes swipl.rc not work properly and then search paths are not correctly updated. (I have no idea how your system was finding the libs nonetheless because it is pretty clear that swipl.rc/ will not find them if we use that.)

I think the clean solution is to go back to the original and add -F swipl to the flags. All other flags have recently been updated (including the new --no-packs to avoid interaction with user packs).

I think I am not following you completely. If you can answer these ones,it would be great:

  1. Should I use -F or -f or is irrelevant user or system wide here?
  2. The executable for Linux will be ../../home/swipl.rc and I can totally do cases for Windows and Mac. Is the Window one ../../home/libswipl.rc? I can find what OS it is running via SWI os.name flag or from Java anyways right?
  3. Finally, what do you mean the other flags have been updated? Are you suggestion to drop them?

If I know those 3 things it is trivial to adapt what I have now in java to "setup" the Prolog JNI interface, which is this:

String[] init_swi_config = new String[] {
//  "libswipl.dll", "-x", startup, "-f", "none",    // libswipl.dll will not update search paths for libraries
                swi_exec, "-x", startup, "-f", "../../home/swipl.rc",
                "-g", "true", "-q",
                "--home="+home, "--no-signals", "--no-packs" };
        Prolog.set_default_init_args(init_swi_config);

Thanks!

JanWielemaker commented 4 years ago

I think the most 'correct' (means robust) argv is

 libswipl.dll -x startup -F swipl -f none -g true -q --home=XXX --no-signals --no-packs

That should work regardless of the OS and should load home/swipl,rc to find the shared objects.

JanWielemaker commented 4 years ago

Ohh Mac, and Macport...

Tell me all about it :cry:

OK we can add any known path, that's easy and should work fairly robust right?

Yeah, I think that is fine.

In principle, one can find jar files first, and open them via unzip to extract META-INF/MANIFEST.MF and it has the Implementation-Version: 4.12. Now all that in CMAKE, should be possible (I guess CMAKE can call scripts), but too much infrastructure for such a standard package right? What if there are many JARs, etc..?

You can call anything from CMake, but you don't want to as one cannot assume a POSIX shell or POSIX tools. For that reason a lot of good stuff is built into cmake itself as cmake -E command arg .... You can use (cmake tar also reads zip)

cmake -E tar tf .../junit.jar

to get the table of content. Reading the MANIFEST.MF seems a little hard, you can only extract the whole thing. If there is a file in junit4 (I guess so) that is fairly unique we can use the above to test for this file. I can help there. Not sure how badly it is needed.

ssardina commented 4 years ago

Regarding the dir for Mac, I have included it in the search so should work. I don't think doing much more fancy guessing inside CMAKE is worth it; what we can do is document this for the JPL developer (make sure one has junit4.jar or some version of it in some of these dirs).

Regarding the more important argv to call Prolog inside Java, there is something big I am missing here. I am of course wary of claiming you are wrong! ;-)

If we fix "libswipl.dll", then the init file swipl.rc, which is effectively build_home.pl under main SWI source, will be bound to not set-up the search paths correctly, because predicate cmake_binary_directory/1 will for sure fail (that I can claim 100%) due to this line.

The failure of cmake_binary_directory/1 will cause the failure of swipl_package/2

Finally, this will cause this code to fail and NOT update the search paths to point to the development libs in build/packages/*.

What am I missing here?

If Windows does need libswipl.dll, then my best take is to use that for Windows and the other for Linux:

public static final String swi_exec =
   System.getProperty("os.name").contains("Windows") ? "libswipl.dll"
               : String.format("%s/../src/swipl", home);
JanWielemaker commented 4 years ago

If we fix "libswipl.dll", then the init file swipl.rc, which is effectively build_home.pl under main SWI source, will be bound to not set-up the search paths correctly, because predicate cmake_binary_directory/1 will for sure fail (that I can claim 100%) due to this line.

Nope. The system config file (base) name is determined using -F, or in the absence thereof as the file name of argv[0] without extension. So, using libswipl.dll it will look for libswipl.rc, which is wrong and why you had problems. Using -F swipl it will look for swipl.rc, so we are fine.

Argv[0] is also used to find the installation home, but in this case we pass this explicitly using --home=Dir. So in fact argv[0] is not used if both -F base and --home=Dir are given.

In a normal run, argv[0] as libswipl.dll works for Windows and on other systems the default generally suffices to find the resources and if this is not the case one typically needs SWI_HOME_DIR or --home=Dir. So, overall libswipl.dll is fine as arg[v] for the JPL use case.

ssardina commented 4 years ago

Nope. The system config file (base) name is determined using -F, or in the absence thereof as the file name of argv[0] without extension. So, using libswipl.dll it will look for libswipl.rc, which is wrong and why you had problems. Using -F swipl it will look for swipl.rc, so we are fine

Argv[0] is also used to find the installation home, but in this case we pass this explicitly using --home=Dir. So in fact argv[0] is not used if both -F base and --home=Dir are given.

All this I get it. By using -F swipl we force loading the script swipl.rc always and with --home we give the home explicitly. All good and argv[0] does not interfere there..

In a normal run, argv[0] as libswipl.dll works for Windows and on other systems the default generally suffices to find the resources and if this is not the case one typically needs SWI_HOME_DIR or --home=Dir. So, overall libswipl.dll is fine as arg[v] for the JPL use case.

So far so good, swipl.rc will be loaded due to -F, I don't dispute that, but my point is that in that script predicate cmake_binary_directory/1 will fail. See third line:

cmake_binary_directory(BinDir) :-
    current_prolog_flag(executable, OsExe),
    OsExe \== 'libswipl.dll',           % avoid dummy for embedded JPL test
    prolog_to_os_filename(Exe, OsExe),

And, If this predicate fails, the packages's dirs are not added to the search path as we need. And this is exactly what happens in my devel install and I can see the Prolog swipl.rc file (which is effectively build_home.pl)

JanWielemaker commented 4 years ago

Ah. I missed that point. Yes, you are right there. Then why is it ok for me? Seems I should get this error unless $CLASSPATH already contains jpl.jar. In that case the first clause of add_jpl_to_classpath/0 does the job and the autoloaded directory_file_path/3 is never called and thus nothing is loaded. Just, CLASSPATH is not in my environment.

Bit of a mystery. Anyway, your fix should be fine. I guess we have no way to detect we are embedded and thus the check whether or not jpl.jar is in classpath is irrelevant?

ssardina commented 4 years ago

My understanding Jan is that this is not about CLASSPATH but about setting the search paths for the C package libraries .so. This is done by add_package/2 which asserts predicates of the sort file_search_path/2 for the many libraries that were compiled and put into build/package/<name of lib>. If these are not found, then Prolog will give errors when trying to use say file.so. Why you don't get those errors is a mystery.

The CLASSPATH is also important when the Prolog code wants to use a Java class that is inside java.jar. And indeed I have changed the script to set CLASSPATH to work.

So, summarizing: the Prolog test code needs:

  1. A good Prolog environment with the search paths to find the package libraries .so
  2. CLASSPATH including jpl.jar for those tests that call classes there.

Makes sense?

Now, what do you mean by "embedded"? You mean when Prolog is called from Java/C? we still want the Prolog engine to have all the search paths correctly because that embedded Prolog could still use SWI libraries right? What am I missing? So I really don't know why build_home.pl has that check OsExe \== 'libswipl.dll. Plus this is ONLY relevant for testing the development system right?

JanWielemaker commented 4 years ago

It all gets a bit fuzzy. CLASSPATH does matter for the files.so issue because if CLASSPATH includes jpl.jar the first clause of add_jpl_to_classpath/0 succeeds and the (autoloaded) call that requires library(filesex) never fires. Yes, that means the test scenario cannot load libraries that rely on foreign extensions, but this was never part of the tests and I doubt it needs to be. If there is no real reason to make the tests depend on other packages I think we shouldn't. So your (1) is not obvious to me. If you have tests where relying on other packages is useful, I agree.

So I really don't know why build_home.pl has that check OsExe \== 'libswipl.dll

I think the reasoning was that the tests where Prolog is loaded from Java (embedded) do (did) not need to have all the paths nicely setup and this was the easiest way to deal with that.

Plus this is ONLY relevant for testing the development system right?

It is relevant for testing, but note there are two scenarios. The normal one where we test before installation and the one where the test suite is installed with the system and the tests are executed in the installed version. The latter was added for testing cross-compiled versions and is somehow also part of the Debian policies.

P.s. Note that with a very small change jpl.pl no longer depends on library(filesex) and thus not on files.so. I think it makes sense to apply that. Would you agree?

JanWielemaker commented 4 years ago

Ok. Pushed a patch that removes dependency on files.so. Enjoy!

ssardina commented 4 years ago

wow wow. That blew my mind! Thanks for the clear explanation, it would have taken even more days to connect the dots. Yes it gets really fuzzy. I have not connected that point as I got obsessed on why SWI was not seeing the libs. And I forgot the original problem was in jpl.pl to find the jpl.jar! Yes if the jar is in the CLASSPATH then the 1st clause will apply and no need to use files.so. (I still need to understand why that does not happen in my system even though I do have jpl.jar in the CLASSPATH)

1. I agree that if jpl.pl can do it without library(filesex), then it should. What would that take though?

2. On the other hand, I think it is cleaner, and without much effort, to get the testing SWI engine to have all paths to the libs, why not? While I don't use any library indeed, I do not rule out that we could use some in future tests. I would not like to have a JPL set-up with a note to developers; 'do not use any libs because it won't work' ;-)

Also, using something like libswipl.dll as a mark to tell Prolog not to set-up the search paths for the libs because we just happen not to use them feels not clean or easy to reconstruct for a newcomer (like it was my case). Not that setting those paths takes a long of time or anything right? I would prefer to remove that from the Prolog code and let the engine set-up a whole SWI framework as compiled.

3. I did not consider the second scenario, I have to verify everything works well there, it may not if the paths are different, which will be!

I particularly would like your views on 2 and what is the reason for not allowing swi to set-up the search paths always: cleaner and more robust right?

OK for tomorrow, here in Autralia is late...

(once this is done, 3 issues will be closed; this, junit4, and rational)

JanWielemaker commented 4 years ago

I tend to agree about (2). I guess it was mostly laziness or remains of even older problems. Just go ahead. I'm a little in doubt about 8.1.29. How close do you think you are?

ssardina commented 4 years ago

I tend to agree about (2). I guess it was mostly laziness or remains of even older problems. Just go ahead. I'm a little in doubt about 8.1.29. How close do you think you are?

I concur. I think this was stuff left from old code with other problems that are not there anymore and that was a quick fix back then. Done!

I am close but not yet finished.

  1. This issue is now resolved and I am happy, it was an old pain and learnt a lot, done.
  2. I am almost finished with the full upgrade of unit testing (#40), big cleanup there, but just fixing what the person reported in the forum. I am waiting for his/her last feedback but I trust it is not resolved finally (dependencies of JUNIT)
  3. Rationals is there too, in another branch, but waiting for the other two fixes to go before.

If you want NOW, maybe no, but in a couple of days yes, maybe even Wed if junit is all done

UPDATE: 1 and 2 are done and in the PR #43 . Once that is merged I will do a PR for Rational branch

JanWielemaker commented 4 years ago

If you want NOW, maybe no, but in a couple of days yes, maybe even Wed if junit is all done

It seems rational to wait for rationals :smile:

ssardina commented 4 years ago

ahaha that's very rational thinking indeed :-)

OK I am cleaning up the git history so we can push a cleaner rebased version to master.

Git is just amazing, but one has to learn how to use it well. I have learnt a lot the past week, really impressive what you can do (I have been using it for years but not with so much care as it deserves here)

Will let you know when the PR is ready

JanWielemaker commented 4 years ago

Git is just amazing

It is. Takes either a lot of tricks to learn or a good intro that explains how it really works, so you know what is it telling you if is says "your HEAD is detached" :smile:

ssardina commented 4 years ago

Indeed, a good intro helps a lot, but it also needs actual hands-on and many pitfalls!

hahah "your HEAD is detached", excellent... mine is "detached and rebased" :-)