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

lib4ti2: switch to BinaryWrappers.jl #506

Merged
merged 2 commits into from Nov 9, 2021
Merged

lib4ti2: switch to BinaryWrappers.jl #506

merged 2 commits into from Nov 9, 2021

Conversation

benlorenz
Copy link
Member

@benlorenz benlorenz commented Nov 8, 2021

This switches the lib4ti2_jll wrappers to a common scratchspace that is used by polymake as well.

Unfortunately this doesn't work as expected right now, somehow singular does not call the wrapper as expected, at least locally I got some error output from singular even though the testsuite still succeeded.

Edit:
Github actions show the same messages but still succeeds with the Singular testuite:

call.Interpreter |    6      6
awk: cmd. line:1: fatal: cannot open file `sing4ti2.mar' for reading (No such file or directory)
    singular error: wrong range[2] in ideal/module _(1)
awk: cmd. line:1: fatal: cannot open file `sing4ti2.gra' for reading (No such file or directory)
    singular error: wrong range[2] in ideal/module _(1)
awk: cmd. line:1: fatal: cannot open file `sing4ti2.hil' for reading (No such file or directory)
    singular error: wrong range[2] in ideal/module _(1)
Test Summary: | Pass  Total
caller.Lib    |   28     28

@benlorenz benlorenz marked this pull request as ready for review November 8, 2021 21:52
@benlorenz
Copy link
Member Author

I did an update to the BinaryWrappers.jl (0.1.0 -> 0.1.1) which uses copies of the wrappers for each 4ti2 program instead of symlinks which fixes the singular errors. I can't find any more errors in the logs after rerunning the tests with that 0.1.1 version, and I added a compat bound for that version now.

Singular seems to insist on calling readlink on whatever it finds with the correct name and calls what is behind the link instead of calling the symlinks directly. I would love to hear why this is necessary since having a bunch of symlinks pointing to a single binary and deciding the operation by inspecting $0 is not that uncommon on Linux.

@fingolfin
Copy link
Member

Indeed, it seems Singular does use readlink (CC @hannes14 @tthsqe12) via code from resources/omFindExec.c. This is then used to implement the system("executable", NAME) function in Singular; here's an excerpt of the Singular library using it:

   // find the name of markov/4ti2-markov
   string s_name=system("executable","markov");
   if (size(s_name)==0) { s_name=system("executable","4ti2-markov"); /* debian*/ }
   j=system("sh",s_name+" sing4ti2 >/dev/null 2>&1");

There are 13 places in the Singular library using this primitive. Eight are related to 4ti2, and five are in surf.lib. As far as I could tell, in all cases, the purpose is to check which of several alternative binaries is available, and invoke that. Now, following the symlinks might be useful to determine if there is actually an underlying and executable file. But then for actual execution, indeed the "original" path (as obtained via lookup using the PATH env var) should be used. So I do think this is indeed a bug / design issue in Singular.

There are more places calling omFindExec, a few of them share the issue, but I did not verify all. Also, I notice that omFindExec has more flaws; e.g. a buffer is passed to it as second argument, but no buffer size, and so buffer overflows in there are easy to produce. It also searches LD_LIBRARY_PATH if nothing is found in PATH, which seems odd?

@hannes14
Copy link
Member

hannes14 commented Nov 9, 2021

Thanks @fingolfin for finding system("executable"..). This is fixed with
Singular/Singular@314f6db
But the main purpose of omFindExec is the finding Singular (and relative to the true binary (after following symlinks) libraries, etc.). If Singular is used as libSingular.so, omFindExec has to find this (check LD_LIBRAY_PATH).

@benlorenz benlorenz linked an issue Nov 9, 2021 that may be closed by this pull request
@benlorenz
Copy link
Member Author

Thanks for the quick fix, from my point of view we could merge it as it is right now (with the workaround via copies instead of symlinks).
I will remove the workaround from the BinaryWrappers.jl once we switch to a newer Singular_jll build but I don't think we need to do this just for these wrappers.

@fingolfin
Copy link
Member

Great, thanks to all!

@fingolfin fingolfin merged commit a954a08 into master Nov 9, 2021
@fingolfin fingolfin deleted the bl/4ti2wrappers branch November 9, 2021 13:02
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.

Test suite errors due to zsolve crashing
3 participants