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

Z3 build scripts assume there's a single ocamlfind path location #5814

Closed
protz opened this issue Feb 4, 2022 · 9 comments
Closed

Z3 build scripts assume there's a single ocamlfind path location #5814

protz opened this issue Feb 4, 2022 · 9 comments
Labels
build/release build and release scripts and process

Comments

@protz
Copy link

protz commented Feb 4, 2022

This assumes the output of ocamlfind printconf path is a single entry:

stubs_install_path = '$$(%s printconf path)/stublibs' % OCAMLFIND

This is incorrect: the user may export OCAMLPATH to inform ocamlfind that there are multiple directories in which to find packages. For instance, on my machine:

jonathan@absinthe:~/Code/z3 (master) $ ocamlfind printconf path
/Users/jonathan/Code/everest/fstar/bin
/Users/jonathan/.opam/default/lib

This generates an incorrect ocamlmklib invocation:

ocamlmklib -o api/ml/z3ml -I api/ml -L. api/ml/z3native_stubs.o api/ml/z3enums.cmo api/ml/z3native.cmo api/ml/z3.cmo -lz3 -lstdc++ -ccopt -L/Users/jonathan/Code/everest/fstar/bin /Users/jonathan/.opam/default/lib/stublibs -dllpath /Users/jonathan/Code/everest/fstar/bin /Users/jonathan/.opam/default/lib/stublibs

which in turn makes the build fail:

ocamlmklib -o api/ml/z3ml  -I api/ml -L. api/ml/z3native_stubs.o api/ml/z3enums.cmo api/ml/z3native.cmo api/ml/z3.cmo  -lz3 -lstdc++  -ccopt -L$(ocamlfind printconf path)/stublibs -dllpath $(ocamlfind printconf path)/stublibs
Don't know what to do with /Users/jonathan/.opam/default/lib/stublibs

Proposed patch:

diff --git a/scripts/mk_util.py b/scripts/mk_util.py
index 042e6af46..e3319c257 100644
--- a/scripts/mk_util.py
+++ b/scripts/mk_util.py
@@ -1995,7 +1995,7 @@ class MLComponent(Component):
 
             LIBZ3 = LIBZ3 + ' ' + ' '.join(map(lambda x: '-cclib ' + x, LDFLAGS.split()))
 
-            stubs_install_path = '$$(%s printconf path)/stublibs' % OCAMLFIND
+            stubs_install_path = '$$(%s printconf path | tail -n 1)/stublibs' % OCAMLFIND
             if not STATIC_LIB:
                 loadpath = '-ccopt -L' + stubs_install_path
                 dllpath = '-dllpath ' + stubs_install_path

Thanks,

Jonathan

@NikolajBjorner NikolajBjorner added the build/release build and release scripts and process label Feb 5, 2022
@NikolajBjorner
Copy link
Contributor

Are you going to add a pull request or asking for more input?

@protz
Copy link
Author

protz commented Feb 5, 2022

If you're ok with my analysis feel free to go ahead and commit the patch :-).

@NikolajBjorner
Copy link
Contributor

@k4rtik - what do you think about the patch from this sketchy character?

@k4rtik
Copy link
Contributor

k4rtik commented Feb 6, 2022

I think we are really calling in @arbipher here to look at this. See #5617 where these changes were last made.

But since I already spent some time looking at it, how would we know that the last entry is the correct entry for stublibs? The patch needs to be made more resilient or the right path determined without reliance on printconf path. I don't know how for either.

@arbipher
Copy link
Contributor

arbipher commented Feb 6, 2022

I agree this may cause problems. I didn't notice ocamlfind printconf path can have multiple result, as shown in your case.
I check with ocamlfind manual and src (List.iter print_endline (Findlib.search_path())). There is no argument to filter just one result for ocamlfind printconf path.

My previous fix uses the path to find stublibs and the path it's used for linking. I would also ask K4rtik's question and then I found the answer. I test it on my own (by export OCAMLPATH and see the result of the command).
I have spent some time to read its source of ocamlfind, the result of the command comes from env_search_path @ sys_search_path (src) where:

  • env_search_path is set by reading OCAMLPATH.
  • sys_search_path is the path defined in the findlib config file. See this comment.

The conf file can be found by ocamlfind printconf conf.

$ cat /home/exx/.opam/4.13.1/lib/findlib.conf
destdir="/home/exx/.opam/4.13.1/lib"
path="/home/exx/.opam/4.13.1/lib"
ocamlc="ocamlc.opt"

I am thinking since the task is to get the path ocamlfind install uses destdir in the conf file, maybe we could use ocamlfind printconf destdir instead. destdir should be just one path (manual).

Then the patch will be this at the line

                              stubs_install_path = '$$(%s printconf destdir)/stublibs' % OCAMLFIND

@k4rtik
Copy link
Contributor

k4rtik commented Feb 6, 2022

Perfect, destdir does seem like an appropriate solution. Excellent analysis, @arbipher!

@protz
Copy link
Author

protz commented Feb 8, 2022

Thanks for the improved solution -- best not to trust patches from sketchy characters indeed!

@NikolajBjorner
Copy link
Contributor

and meanwhile...

image

@arbipher
Copy link
Contributor

arbipher commented Feb 8, 2022

lol, as the current maintainer of the z3-ocaml on opam, I know two faces in the photo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build/release build and release scripts and process
Projects
None yet
Development

No branches or pull requests

4 participants