ENHANCED: allow user:exception for missing shlibs, save dependencies

[erlanger]

trafficstars

This patch allows the user to load a shared library from the network or from the saved state by means of writing the user:exception hook.

It also provides qsave_foreign_libraries/4 to retrieve foreign libraries (for a compatible architecture) from the saved state. This makes it easier to implement the user:exception hook, while abstracting from the internal naming of the shared libraries in the saved state.

Accordingly qsave_program/2 is also enhanced to store dependencies for the main shared library.

Test cases are also provided.

Implements what was discussed in #425 and #427

[erlanger]

oops...forgot to update the documentation for qsave_foreign_libraries/4, it now returns a list of dicts with information about the entry rather than just a list of the entry names.

I'll submit a patch later.

[erlanger]

BTW, the swipl -o <xxx> -c <src> exit code is 0 even if qsave_program/2 fails or throws an exception. Is this intended?

[JanWielemaker]

BTW, the swipl -o <xxx> -c <src> exit code is 0 even if qsave_program/2 fails or throws an exception. Is this intended?

Probably not :( Most of this stuff is really old ...

[erlanger]

Probably not :( Most of this stuff is really old ...

Would you like me to submit a patch for it?

[JanWielemaker]

Please!

[erlanger]

Please!

done in d3f9bc1 , but ..perhaps you wanted a separate PR ....

Edit: just wanted to point out that load_files was succeding even if it failed to load a file (it only printed the exception error but succeded). This is also fixed in the commit above, because it also affected swipl ... -c.

[JanWielemaker]

Sorry this is taking so long. Bit busy and I think this pull request is largely good, but there are some things that need some additional thought. One issue I'd like to share now is the use of atom_to_term/2 to add fancy names to the ZIP files. Although I'm not entirely sure, you introduce terms '$shlib'(...) which results in really ugly ZIP file names, right?

I propose to turn these into normal paths, so people can (dis)assemble them using zip. For example, rather than (old) foreign(mylib) we could use /alias/foreign/mylib. Does that make sense?

[erlanger]

Yes, I didn't like the names either... the entry name needs to allow reconstruction of the alias/path and needs to encode the following information:

1. whether it is an alias or a regular path,
2. the name of the original file,
3. whether it is a dependency or the main shlib
4. the architecture so I propose the following:

$shlib/<arch>/$alias/foreign/<foreign_arg>/$main/mainlib.so --> for main library with alias
$shlib/<arch>/$alias/foreign/<foreign_arg>/$deps/libdep.so  --> for dependecies  with alias
$shlib/<arch>/<path>/$main/mainlib.so                       -->  for main library with regular path
$shlib/<arch>/<path>/$deps/libdep.so                        -->  for dependency library with regular path

Of course <path> and <foreign_arg> can contain a / separated path. The '$' is used because those are internal names, and the documentation for 'res://' says that '$' is a reserved character, so we want to use that to prevent clashes with user resources.

What do you think?

[JanWielemaker]

Looks ok to me. Not sure I like all the $. They are probably needed to avoid ambiguity, but they do not make it easier to manage the zip files using shell scripting. There are not many good alternatives though. Let us try and see.

[erlanger]

OK, will get to work on it.

[erlanger]

BTW, from http://www.swi-prolog.org/pldoc/man?section=res-declare

Name is the name of the resource (an atom). A resource name may contain any 
character, except for $ and :, which are reserved for internal usage 
by the resource library. 

We could use : also. Perhaps something like $shlib/<arch>/:alias/foreign/<foreign_arg>/:deps/libdep.so, this is easier from shell scripts. I'll use that instead.

[JanWielemaker]

We could use : also

Windows file names are not allowed to containt : AFAIK, so you cannot unpack the archive ... Let us settle with one odd character (or reserve more from resource declarations).

[erlanger]

Ok. thanks. $shlib/<arch>/alias/<alias>/<foreign_arg>/deps/libdep.so it is.

[erlanger]

Ok, here are the fixes:

• deleted the swipl -c commit, since we already handled that in another PR
• Fixed the file names inside the zip file to make them script friendly
• Produce the test shlib files at build time, so that they are available for test_installation

[JanWielemaker]

Great! Bit busy, so I cannot make promises. This is surely going to be reviewed and merged though. Guess you can continue your Android work with your branch anyway?

[erlanger]

Yes, so far I can continue with my branch. I also have some patches for Jpl so that it loads on the Android JVM, but I don't think they are ready for a PR, the JPL c code is a little bit messy, and I am trying to make the least changes as possible.

[JanWielemaker]

JPL c code is a little bit messy

It would need a big rewrite turning 99% of the macros into functions ...

[erlanger]

BTW, they are doing a major rework in the termux project, so they are not adding SWI Prolog to the main repo for now (they've put it in the unstable repo); so I think we just wait and keep using our current method until it gets into the main repo.

[erlanger]

We have some extra deps in libswipl.so:

$  objdump -p  ./libswipl.so.8.1.1 |grep NEEDED                                                                                                                                                                                            
  NEEDED               libncursesw.so.6      <-------  perhaps we can make these two optional? 
  NEEDED               libformw.so.6         <-------   
  NEEDED               libgmp.so.10
  NEEDED               libz.so.1             <------ I thought we used minizip.c
  NEEDED               libpthread.so.0
  NEEDED               libdl.so.2
  NEEDED               libm.so.6
  NEEDED               librt.so.1
  NEEDED               libc.so.6

[JanWielemaker]

libncursesw.so.6

Is used by pl-term.c. I'm not against moving this stuff to the clib package. Not sure about the Windows stuff in that case. Also check for usage of tty_size/2 throughout the code that must in that case be prepared to deal with the possibility this isn't around. Not really sure what formw does there.

libz.so.1

Minizip provides the archive functionality on top of the compression library libz.

[JanWielemaker]

Sorry this takes so long. As a prove I do not want to let this slip I rebased the branch after fixing merge issues to shlib-deps-and-user-exception on this repo. You can fetch and do a hard reset on to get your version of the branch in sync.

It is a quite complex patch and there are some issues with it. One is the testing. As is, this modifies the source tree. That is not allowed in a proper CMake build. So, these build products must be created in the build directory, most likely in src/Tests/save/... under the build dir. Notably the ssl package contains an example of rather complicated test data production using Prolog. This also needs to get all dependencies right for running the test build. As is, the boot.prc might not be built yet. I also have my doubt trying to use swipl-ld for this. I think it is safer to build the support .so files using the configured toolchains as used for e.g., the packages.

I also have my doubts about the way you deal with errors. Asserting and delaying them seems highly dubious. There is surely room for enhancing error processing during processes like this, which also causes problems in the compiler. Roughly, I think we need three options depending on a flag:

• print messages and continue (now the default for most compiler messages)
• print messages and at the end of some command-line driven action (e.g., -c compilation), exit with status 1 if an error was printed (and optionally if a warning was printed)
• Cause an error to abort the current task, clean up work done and propagate the error to the caller. This requires quite some work to perform the proper cleanups, notably in the compiler.

For now, for qsave, I propose to simply propagate the error for fatal issues and print them otherwise, just as the compiler does.

[erlanger]

Sorry this takes so long. As a prove I do not want to let this slip I rebased the branch after fixing merge issues to shlib-deps-and-user-exception on this repo. You can fetch and do a hard reset on to get your version of the branch in sync.

Thanks, it will be a bit since I get back to work back on this, I lost a bit of the energy I had going.

It is a quite complex patch and there are some issues with it. One is the testing. As is, this modifies the source tree. That is not allowed in a proper CMake build. So, these build products must be created in the build directory, most likely in src/Tests/save/...

ahh, yes, the .so files are created at build time in the source dir...they need to be moved..

I also have my doubts about the way you deal with errors. Asserting and delaying them seems highly dubious.

That was in the original code, so I just followed the pattern to make as few changes as possible. I don't like it much either.

There is surely room for enhancing error processing during processes like this, which also causes problems in the compiler. Roughly, I think we need three options depending on a flag:

Yes, I think these are good ideas.