Skip to content

Conversation

@tobil4sk
Copy link
Contributor

@tobil4sk tobil4sk commented Feb 4, 2025

conf-libuv is now an optional dependency, and if installed then the system libuv will be used instead of the vendored build, by default.

This provides a more convenient way to control how luv is built. This can still be overridden by setting: LUV_USE_SYSTEM_LIBUV=no.

The bash commands and configure scripts required to build luv from source are not very portable, and as seen in #162, they don't work properly on Windows without additional setup. This patch useful for projects that run on windows, as they can simply add conf-libuv to their dependencies to use the system version, without needing manual intervention every time dependencies are installed/updated.

conf-libuv is now an optional dependency, and if installed then by
default, the system libuv will be used instead of the vendored build.

This provides a more convenient way to control how luv is built. This
can still be overridden by setting: LUV_USE_SYSTEM_LIBUV=no
@tobil4sk tobil4sk marked this pull request as draft February 5, 2025 12:29
@tobil4sk
Copy link
Contributor Author

tobil4sk commented Feb 6, 2025

It would be better to move the jbuild plugin stuff to use dune-configurator instead so the intermediate environment variable can be avoided, however, moving to that would require include support in (install (files ...)), and updating to the required dune version breaks %{lib:ctypes:.}. It looks like the only way to deal with the ctypes issue is to use the builtin ctypes integration in dune.

Also, when moving everything to dune-configurator, I can't think of a way to dynamically add (foreign_archives uv) based on this configuration. Variables don't work in foreign_archives, and I can't figure out how to get the ocaml code in src/c/dune to check the dune-configurator generated files.

I think using an environment variable like this is the best solution for now, and if dune support is improved in future then this can be changed to use dune-configurator instead.

@tobil4sk tobil4sk marked this pull request as ready for review February 8, 2025 11:18
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.

1 participant