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

Add a test about dune-sites where plugin links extra libary. #4348

Merged
merged 9 commits into from
Nov 18, 2021

Conversation

Kakadu
Copy link
Contributor

@Kakadu Kakadu commented Mar 12, 2021

For now this test gives a dynlink error about missiing library. See also
#4320

@bobot ^^

@Kakadu
Copy link
Contributor Author

Kakadu commented Mar 19, 2021

@bobot Any change this can be added to 2.9 milestone together with #4212 ?

@Kakadu
Copy link
Contributor Author

Kakadu commented Sep 1, 2021

@bobot Did you manage to find some free time to look at it?

diff --git a/../../../default/test/blackbox-tests/test-cases/sites.t/run.t b/test/blackbox-tests/test-cases/sites.t/run.t.corrected
index 174ce491e..c0da368a0 100644
--- a/../../../default/test/blackbox-tests/test-cases/sites.t/run.t
+++ b/test/blackbox-tests/test-cases/sites.t/run.t.corrected
@@ -1,5 +1,7 @@
 
   $ dune build ./app.exe @install
   $ dune exec ./app.exe
+  Fatal error: exception Dynlink.Error (Dynlink.Cannot_open_dll "Dynlink.Error (Dynlink.Cannot_open_dll \"Failure(\\\"/home/kakadu/.opam/4.12.0+options/lib/ocaml/unix.cmxs: undefined symbol: camlStdlib__callback__register_exception_16\\\")\")")
+  [2]
 

@bobot
Copy link
Collaborator

bobot commented Sep 29, 2021

The problem is that a plugin depends on threads and not the main program. It is a limitation of the OCaml runtime. The only fix currently is at runtime to not try to load threads, and raise a good error instead.

Sorry for the delay.

@bobot
Copy link
Collaborator

bobot commented Sep 29, 2021

I made some modifications directly to the branch: moving the tests, adding a working version

@ejgallego
Copy link
Collaborator

Umm, does this mean that plugins are now always linked with -linkall?

Isn't that a problem in OCaml >= 4.08?

@bobot
Copy link
Collaborator

bobot commented Sep 29, 2021

Indeed I still added the -linkall as for findlib.dynload. @ejgallego Why do you think it is a problem for OCaml >= 4.08?

@ejgallego
Copy link
Collaborator

Indeed I still added the -linkall as for findlib.dynload. @ejgallego Why do you think it is a problem for OCaml >= 4.08?

I always get confused with the meaning of -linkall in different contexts [such as -shared].

Does the use of -linkall here mean that .cmxs files will include all the libraries they refer to?

@ejgallego
Copy link
Collaborator

To be clear, the particular case I worry about is:

  • library Lib
  • plugin A, using Lib
  • plugin B, using Lib

Now, application App loads A and B; if Lib is linked in both, dynlink will fail in OCaml >= 4.08 as doubly linking modules was forbidden in that version. IMHO there should be a test for this.

@Kakadu
Copy link
Contributor Author

Kakadu commented Sep 29, 2021

To be clear, the particular case I worry about is:

* library `Lib`

* plugin `A`, using `Lib`

* plugin `B`, using `Lib`

Now, application App loads A and B; if Lib is linked in both, dynlink will fail in OCaml >= 4.08 as doubly linking modules was forbidden in that version. IMHO there should be a test for this.

@ejgallego I fixed a test in a manner I understood your concerns. Are you less worried now?

@ejgallego
Copy link
Collaborator

@ejgallego I fixed a test in a manner I understood your concerns. Are you less worried now?

Thanks a lot, it is great we are testing this case. I will have a detailed look tomorrow.

@bobot
Copy link
Collaborator

bobot commented Sep 30, 2021

To be clear, -linkall is only used for the static linking of the binary (EDIT:executable) because we must be sure that all the libraries that are dependencies of the binaries have all their modules linked. Thanks @Kakadu for the additional tests !

@ejgallego
Copy link
Collaborator

To be clear, -linkall is only used for the static linking of the binary because we must be sure that all the libraries that are dependencies of the binaries have all their modules linked. Thanks @Kakadu for the additional tests !

Oh, I see; thanks for the comment @bobot ; on the other hand I think that silently adding -linkall (even in the findlib case) is a bug.

I find it pretty weird that just by adding findlib.dynload to my libraries stanza, then all modules in unrelated libraries [with their side effects] are now linked. WDYT?

@bobot
Copy link
Collaborator

bobot commented Sep 30, 2021

I think it is weirder to add findlib.dynload or dune-sites.plugin as dependencies and being ok that it is not usable. I don't see any use case for that.

@ejgallego
Copy link
Collaborator

I think it is weirder to add findlib.dynload or dune-sites.plugin as dependencies and being ok that it is not usable.

Why they would be not usable? Adding -linkall seems to me like a brute-force way to implement them, but it has problems for sure with libraries that may not like -linkall. It should be possible to implement the plugin support without using the big -linkall hammer.

I don't see any use case for that.

A use case for what?

@bobot
Copy link
Collaborator

bobot commented Sep 30, 2021

Adding -linkall seems to me like a brute-force way to implement them, but it has problems for sure with libraries that may not like -linkall.

Which libraries don't like -linkall ? In a way -linkall only simulate that the code use (i.e open) all the modules provided by the libraries. A library that break because you are using too much of it seems weird.

It should be possible to implement the plugin support without using the big -linkall hammer.

Perhaps but I don't know it. There are many difficulties to implement a plugin engine without linking all the modules of the static libraries:

  • It is difficult to know which modules are statically linked
  • It is currently not possible to dynlink a .cmxs where part of the modules are already linked
  • Libraries that does side effect, without using for themselves -linkall, will have to take into account different loading orders.

The library granularity is clearer and simpler.

@ejgallego
Copy link
Collaborator

Which libraries don't like -linkall ?

I can't come up with an example now, but I imagine with large things such as core.async etc..., you don't want to link to all modules unless you know what you are doing.

My point is that we are using -linkall in the executable, so, I have a project, then I add findlib.dynload to the libraries field, now my executable gets huge, and maybe is doing stuff it shouldn't.

Perhaps but I don't know it. There are many difficulties to implement a plugin engine without linking all the modules of the static libraries

What's the problem with just referencing the modules that are used?

@ejgallego
Copy link
Collaborator

I moved the discussion to its own issue #4957

@bobot bobot force-pushed the dune-sites-test branch 2 times, most recently from ff4010d to ac872ff Compare October 1, 2021 09:43
@bobot
Copy link
Collaborator

bobot commented Oct 1, 2021

@Kakadu I rebased the commits and changed the order in which the tests are added in order to see that there is 2 fixes.

@ejgallego Can you review? even if the current state of the art of plugins in OCaml is not statisfying you 😸

@Kakadu
Copy link
Contributor Author

Kakadu commented Oct 1, 2021

I'm having issues with running my old example about sites. The manual mentions a library dune-site.plugins. Where should I get it?

@bobot
Copy link
Collaborator

bobot commented Oct 4, 2021

It is in dune, how are you installing the development version of dune? Perhaps the pin is not pinning every package the dune repository provides.

@Kakadu
Copy link
Contributor Author

Kakadu commented Oct 4, 2021

It is in dune, how are you installing the development version of dune? Perhaps the pin is not pinning every package the dune repository provides.

https://gist.github.com/Kakadu/92c8181f89b13c3e584893c173bd34cb#file-log-txt-L75

Am I doing something wrong? It could be separate issue though...

P.S. I moved my complains to separate issue #4966

@ejgallego ejgallego self-assigned this Oct 4, 2021
@ejgallego ejgallego self-requested a review October 4, 2021 15:45
Dmitrii Kosarev and others added 9 commits November 18, 2021 21:39
For now this test gives a dynlink error about missiing library. See also
ocaml#4320

Signed-off-by: Dmitrii Kosarev <Dmitrii.Kosarev@pm.me>
Signed-off-by: François Bobot <francois.bobot@cea.fr>
Signed-off-by: François Bobot <francois.bobot@cea.fr>
Signed-off-by: François Bobot <francois.bobot@cea.fr>
  thread.cmxs doesn't exists. The tests is more interresting because it fails.

Signed-off-by: François Bobot <francois.bobot@cea.fr>
Signed-off-by: François Bobot <francois.bobot@cea.fr>
Signed-off-by: François Bobot <francois.bobot@cea.fr>
Signed-off-by: Dmitrii Kosarev <Dmitrii.Kosarev@pm.me>
It should make @ejgallego less worried

Signed-off-by: Dmitrii Kosarev <Dmitrii.Kosarev@pm.me>
@bobot bobot added this to the 2.9.2 milestone Nov 18, 2021
@bobot bobot merged commit 3fd02b6 into ocaml:main Nov 18, 2021
$ dune build ./app.exe @install
$ dune exec ./app.exe
The library is being used by two plugins finished initialization
Fatal error: exception Dune_site_plugins__Plugins.Thread_library_required_by_plugin_but_not_required_by_main_executable
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails with a crash for me locally:

   Fatal error: exception Dune_site_plugins__Plugins.Thread_library_required_by_plugin_but_not_required_by_main_executable
+  Raised at Dune_site_plugins__Plugins.load_gen in file "otherlibs/site/src/plugins/plugins.ml", line 200, characters 6-89
+  Called from Stdlib__list.iter in file "list.ml", line 110, characters 12-15
+  Called from Dune_site_plugins__Plugins.load_gen in file "otherlibs/site/src/plugins/plugins.ml", line 204, characters 4-36
+  Called from Stdlib__list.iter in file "list.ml", line 110, characters 12-15
+  Called from Dune_site_plugins__Plugins.load_gen in file "otherlibs/site/src/plugins/plugins.ml", line 204, characters 4-36
+  Called from Stdlib__list.iter in file "list.ml",   [2] line 110, characters 12-15
+  Called from Dune__exe__App in file "app.ml", line 2, characters 9-42
   [2]

Could you do something about it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have something like "backtrace printing is always activated"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'm not sure what "backtrace printing is always activated" means.

Copy link
Contributor Author

@Kakadu Kakadu Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like export OCAMLRUNPARAM=b in ~/.bashrc...

For my your problem is not easily reproducible too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kakadu I see. That may well be it: my ~/.bashrc collects settings from various sources, and I wouldn't be surprised if one of them does what you say.

@bobot
Copy link
Collaborator

bobot commented Nov 19, 2021

@ejgallego Sorry the changes is not well placed, are you going to move it?

kit-ty-kate pushed a commit to ocaml/opam-repository that referenced this pull request Jan 23, 2022
CHANGES:

- Fix missing -linkall flag when linking library dune-sites.plugin
  ( ocaml/dune#4348, @Kakadu, @bobot, reported by @Kakadu)

- No longer reference deprecated Toploop functions when using dune files in
  OCaml syntax. (ocaml/dune#4834, fixes ocaml/dune#4830, @nojb)

- Use the stag format API to be compatible with OCaml 5.0 (ocaml/dune#5351, @emillon).

- Fix post-processing of dune-package (fix ocaml/dune#4389, @strub)
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.

4 participants