Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

erlanger
Copy link

@erlanger erlanger commented Feb 5, 2019

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

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.
@erlanger
Copy link
Author

erlanger commented Feb 5, 2019

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
Copy link
Author

erlanger commented Feb 5, 2019

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
Copy link
Member

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
Copy link
Author

erlanger commented Feb 5, 2019

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

Would you like me to submit a patch for it?

@JanWielemaker
Copy link
Member

Please!

@erlanger
Copy link
Author

erlanger commented Feb 5, 2019

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
Copy link
Member

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
Copy link
Author

erlanger commented Feb 10, 2019

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
Copy link
Member

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
Copy link
Author

OK, will get to work on it.

@erlanger
Copy link
Author

erlanger commented Feb 11, 2019

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
Copy link
Member

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
Copy link
Author

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

@erlanger erlanger force-pushed the shlib-deps-and-user-exception branch from d3f9bc1 to 9ce4881 Compare February 12, 2019 16:20
@erlanger
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

JPL c code is a little bit messy

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

@erlanger
Copy link
Author

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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

erlanger commented Mar 6, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants