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

Remove the threaded/unthreaded split in ctypes-foreign. #651

Merged
merged 1 commit into from
Jul 24, 2020
Merged

Remove the threaded/unthreaded split in ctypes-foreign. #651

merged 1 commit into from
Jul 24, 2020

Conversation

yallop
Copy link
Owner

@yallop yallop commented Jul 11, 2020

Since #80, linking programs that use ctypes.foreign against the threads library has been optional: the package system would pick an appropriate implementation of the Foreign module according to whether the downstream program used threads.

This split into separate "unthreaded" and "threaded" implementations has mostly worked smoothly, but it's occasionally caused problems (e.g. #552) and it makes the code more difficult to maintain; all changes to ctypes-foreign (e.g. #595) have to take care to update both the unthreaded and threaded implementations consistently. Furthermore, since the split makes use of some more obscure parts of the build and package systems, the current design makes moving to a new system (#588) more challenging.

It seems likely that most environments that support ctypes-foreign (i.e. that support libffi) also support threads, in which case there's comparatively little value in continuing to support the unthreaded variant.

This PR therefore removes the split, of making ctypes-foreign-unthreaded the only implementation of the ctypes-foreign package. There should be no downstream breakages, but packages which require ctypes-foreign will now implicitly pull in threads as a transitive dependency. (Packages which use only ctypes and ctypes-stubs should be completely unaffected.)

@yallop yallop mentioned this pull request Jul 11, 2020
18 tasks
@yallop
Copy link
Owner Author

yallop commented Jul 24, 2020

There should be no downstream breakages, but packages which require ctypes-foreign will now implicitly pull in threads as a transitive dependency

This may in fact cause downstream breakages, as the Travis build failure suggests:

ocamlfind: [WARNING] Package `threads': Linking problems may arise because of the missing -thread or -vmthread switch

yallop added a commit to yallop/ocaml-ctypes-inverted-stubs-example that referenced this pull request Jul 24, 2020
@yallop yallop merged commit 77478f2 into yallop:master Jul 24, 2020
@yallop yallop deleted the no-unthreaded branch July 24, 2020 13:02
@aalekseyev
Copy link
Contributor

aalekseyev commented Feb 19, 2021

In a program that uses a ctypes.foreign.threaded library directly, I'm seeing dune complaining with a message:

Library "ctypes.foreign.threaded" not found

My understanding is that I should switch to ctypes.foreign, instead. Is that right?

I have no problem doing that, but I'm curious, since you said that there should be no downstream breakages.
Is there supposed to be some mechanism preventing this error that's malfunctioning in my setup?

@yallop
Copy link
Owner Author

yallop commented Feb 20, 2021

My understanding is that I should switch to ctypes.foreign, instead. Is that right?

Yes, that should fix the issue.

I have no problem doing that, but I'm curious, since you said that there should be no downstream breakages.

When I wrote that I expected clients to refer to ctypes.foreign (rather than ctypes.foreign.unthreaded or ctypes.foreign.threaded) and let the build system pick the appropriate implementation. Apparently not all users do things that way.

(We could perhaps add ctypes.foreign.threadedas a kind of alias to ctypes.foreign to avoid the problem.)

@aalekseyev
Copy link
Contributor

Having such an alias could simplify the migration, although ctypes.foreign is clearly a better name.
I'll see if I can tweak our code to work with both 1.16 and 1.18.
Thank you!

@ivg
Copy link
Contributor

ivg commented Mar 8, 2021

Just as feedback, it looks like that even when ctypes.foreign is used a failure in the downstream package is still possible, apparently the culprit is the findlib.dynload package,

/home/runner/.opam/4.11.1+flambda/bin/ocamlfind opt -o _build/bap/generate -linkpkg -package bap,bap-plugins,ctypes.stubs,ctypes.foreign,findlib.dynload,bap-main _build/bap/lib/bindings.cmx _build/bap/stub_generator/generate.cmx
Warning: : [WARNING] Package `threads': Linking problems may arise because of the missing -thread or -vmthread switch
File "/tmp/findlib_initlf31e30.ml", line 1:
Error: No implementations provided for the following modules:
         Thread referenced from /home/runner/.opam/4.11.1+flambda/lib/ctypes/ctypes-foreign.cmxa(Foreign)
         Mutex referenced from /home/runner/.opam/4.11.1+flambda/lib/ctypes/ctypes-foreign.cmxa(Foreign)

ivg added a commit to BinaryAnalysisPlatform/bap-bindings that referenced this pull request Mar 8, 2021
psafont added a commit to psafont/ocaml-pci that referenced this pull request Apr 1, 2021
See yallop/ocaml-ctypes#651 (comment)

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
psafont added a commit to psafont/ocaml-pci that referenced this pull request Apr 1, 2021
See yallop/ocaml-ctypes#651 (comment)

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
psafont added a commit to psafont/ocaml-pci that referenced this pull request Apr 1, 2021
See yallop/ocaml-ctypes#651 (comment)

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
psafont added a commit to psafont/ocaml-pci that referenced this pull request Apr 1, 2021
See yallop/ocaml-ctypes#651 (comment)

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
jestabro added a commit to jestabro/libvyosconfig that referenced this pull request Feb 3, 2022
jestabro added a commit to vyos/libvyosconfig that referenced this pull request Feb 3, 2022
psafont added a commit to psafont/ocaml-pci that referenced this pull request Nov 1, 2022
See yallop/ocaml-ctypes#651 (comment)

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
psafont added a commit to psafont/ocaml-pci that referenced this pull request Nov 1, 2022
See yallop/ocaml-ctypes#651 (comment)

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
jestabro added a commit to jestabro/libvyosconfig that referenced this pull request Aug 28, 2024
Links added in earlier commit, but one instance missed. Cf.
yallop/ocaml-ctypes#651
jestabro added a commit to jestabro/libvyosconfig that referenced this pull request Sep 16, 2024
Links added in earlier commit, but one instance missed. Cf.
yallop/ocaml-ctypes#651
jestabro added a commit to jestabro/libvyosconfig that referenced this pull request Sep 16, 2024
Links added in earlier commit, but one instance missed. Cf.
yallop/ocaml-ctypes#651
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.

3 participants