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

Optional dependencies concidered harmful #420

Open
kit-ty-kate opened this issue Feb 28, 2023 · 3 comments
Open

Optional dependencies concidered harmful #420

kit-ty-kate opened this issue Feb 28, 2023 · 3 comments

Comments

@kit-ty-kate
Copy link
Contributor

The change in #419 is causing nasty problems at runtime everywhere (users upgrading to tls >= 0.16.0 and trying to run software that previously expected a dependency toward tls to be enough for conduit to use/enable tls at all)

In my opinon, implicit/optional dependencies are harmful to any ecosystem all users and should only be used when strictly necessary, as a last resort. In the case of Conduit, I'm not versed in how it is designed but it feels like the following can be done:

  • Split conduit-lwt-unix's optional dependencies into 3 new sub-packages (conduit-lwt-unix-tls, conduit-lwt-unix-ssl, conduit-lwt-unix-launchd)
  • Add a common semi abstract type in Conduit_lwt_unix so this module knows how deal with the TLS/SSL backend chosen
  • Add a function returning this new type containing all the necessary endpoint that the backend agnostic Conduit_lwt_unix module requires.
@avsm
Copy link
Member

avsm commented Feb 28, 2023

Before we consider solutions, what's actually gone wrong for our users after #419? Is it that they now need to do opam install cohttp-lwt-unix tls-lwt, whereas previously it was just opam install cohttp-lwt-unix tls ?

The original historical reason for supporting both OpenSSL and OCaml TLS in conduit is that the latter wasn't mature enough. But now, the reverse is true: the ssl bindings seem to be less well maintained than OCaml TLS. I'm tempted to suggest dropping support for the OpenSSL in Conduit -- perhaps we should canvas opinions on this from discuss.ocaml.org.

@kit-ty-kate
Copy link
Contributor Author

Before we consider solutions, what's actually gone wrong for our users after #419? Is it that they now need to do opam install cohttp-lwt-unix tls-lwt, whereas previously it was just opam install cohttp-lwt-unix tls ?

yes, or more usually this is already in the chain of dependencies so people don't have to think about it. Similarly tls.lwt was almost always present because most people have already lwt in their dependencies but since it has been moved to its own library tls-lwt (that's a good thing) this breaks (sometime unknowingly) users of this sublibrary. See ocaml/opam-repository#23344 (comment) for two examples in sihl and current_gitlab, ore more recently in ocurrent ocurrent/ocurrent#414 where CI systems that use it just crashed at runtime because no TLS backend was present anymore.

This is just some examples among many other simply showing how things like (optional), depopts, (select ... from ...), while sometimes sadly necessary, lead to non-deterministic behaviours and makes the experience for users more confusing.

@avsm
Copy link
Member

avsm commented Feb 28, 2023

One quick way to fix this with minimum risk to our users would be to simply make tls-lwt a full dependency of conduit, while leaving ssl as a depopt. This way users could still switch libraries, but they'll use tls by default and we can find out if there are any users who really want conduit without any TLS.

shonfeder added a commit to XFFS/oopenai that referenced this issue Mar 2, 2023
Required for cohttp (via conduit): mirage/ocaml-conduit#420
FredaXin pushed a commit to XFFS/oopenai that referenced this issue Mar 9, 2023
Required for cohttp (via conduit): mirage/ocaml-conduit#420
@talex5 talex5 mentioned this issue Jun 14, 2024
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

No branches or pull requests

2 participants