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

opt-in to use system libuv #94

Closed
andyli opened this issue Feb 13, 2021 · 14 comments
Closed

opt-in to use system libuv #94

andyli opened this issue Feb 13, 2021 · 14 comments
Milestone

Comments

@andyli
Copy link
Contributor

andyli commented Feb 13, 2021

I'm about to package luv for various Linux distros, mainly because luv is now a dependency of haxe which I'm the maintainer of its Linux packages.

One of the packaging requirements is to exclusively use system packages instead of vendor libraries.

I would suggest to introduce a build-time option to dynamically link system libuv instead of building and static linking vendor libuv?

@aantron
Copy link
Owner

aantron commented Feb 13, 2021

I believe you'd need to replace the first line here

luv/src/c/dune

Lines 12 to 13 in d3b12d6

(foreign_archives uv)
(c_library_flags (:include extra_libs.sexp)))

with -luv in the list in the second line. I'm not immediately sure about the transitive linking — will users of this variant of Luv also need to specify -luv?

I think all the changes you would have to make would be to that dune file. If you can get it working in your environment(s), I'll be happy to figure out how to make your changes conditional so that the build works with both vendored and system libuv.

Luv is fairly aggressive with upgrading libuv (I upgrade upon upstream release), so the headers may be newer than what the installed library actually supports. I suspect that's usually fine, else Luv can be held back. There may be problems with running tests.

I'm open to changing the release process, etc., to make packaging Luv for distributions easier.

@andyli
Copy link
Contributor Author

andyli commented Feb 14, 2021

Thanks for the tips.
I'm able to build a dynamic linked luv with the patch as follows:
https://salsa.debian.org/ocaml-team/ocaml-luv/-/blob/eee010291bcd5e18b7bd6d170ed7a09171168696/debian/patches/system_libuv.diff

I can build haxe unmodified with the dynamic linked luv, and worked as expected.

$ ldd ./haxe
        linux-vdso.so.1 (0x00007ffd79981000)
        libuv.so.1 => /usr/lib/x86_64-linux-gnu/libuv.so.1 (0x00007f232d7d1000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f232d7af000)
        libpcre.so.3 => /lib/x86_64-linux-gnu/libpcre.so.3 (0x00007f232d73c000)
        libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007f232d71f000)
        libmbedtls.so.12 => /usr/lib/x86_64-linux-gnu/libmbedtls.so.12 (0x00007f232d6ef000)
        libmbedx509.so.0 => /usr/lib/x86_64-linux-gnu/libmbedx509.so.0 (0x00007f232d6ce000)
        libmbedcrypto.so.3 => /usr/lib/x86_64-linux-gnu/libmbedcrypto.so.3 (0x00007f232d665000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f232d521000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f232d51b000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f232d356000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f232eb19000)

@aantron
Copy link
Owner

aantron commented Feb 14, 2021

Great. These lines in the dune file change very infrequently (at least so far). It will be slightly messy to integrate them into the dune file (though still possible). Is it easy to maintain these as a separate patch?

@andyli
Copy link
Contributor Author

andyli commented Feb 14, 2021

Is it easy to maintain these as a separate patch?

Not difficult at the moment. In general it is best to keep zero patches, but it can wait.

@aantron aantron added this to the 0.5.7 milestone Feb 14, 2021
@aantron
Copy link
Owner

aantron commented Feb 14, 2021

I'll integrate it.

Looking at some of the changes, I'm skeptical that not using the vendored headers when building Luv's bindings will be portable, because, as I understand it, some systems might have headers installed that are older than what Luv expects. My initial guess would be that Luv should be compiled against the vendored headers, and then simply used with the system library, and the user should use the system headers. I'll integrate the diff as it is now, and it can be patched later.

aantron added a commit that referenced this issue Feb 14, 2021
Resolves #94.

Co-authored-by: Anton Bachin <antonbachin@yahoo.com>
@aantron
Copy link
Owner

aantron commented Feb 14, 2021

@andyli, see the commit linked above. I reduced the diff considerably. The non-vendored build is now triggered by setting LUV_USE_SYSTEM_LIBUV=yes. It seems to work — I inserted a temporary echo into the vendored build, to make sure it gets suppressed by this variable, and I also checked that I get undefined symbol errors when building the tests and linking with my system libuv, which is older than what Luv expects. It might be better, after all, to use the system headers during the Luv build, as that will fail earlier than linking on old libuv.

Can you try this version?

If you prefer a different method than LUV_USE_SYSTEM_LIBUV=yes of triggering the non-vendored build, I can likely change it.

@aantron
Copy link
Owner

aantron commented Feb 14, 2021

Browsing the Haxe repo and libuv-related issues, it seems almost certain to me that Luv will have to be changed so that it can be compiled against older system libuv. I have some approximate plan for doing that.

If that's so, can you do a survey of the distros Haxe releases to, considering older releases with a large installed base, and let me know what is the minimum version of libuv you'd like Luv to support?

For information, the current release of Luv requires libuv 1.40.0. After #95, Luv will require 1.41.0 (released yesterday). The earliest release of Luv, 0.5.0, requires libuv 1.34.0. The earliest unreleased Luv in git requires libuv 1.20.0, but it's not API-compatible with the releases.

@andyli
Copy link
Contributor Author

andyli commented Feb 14, 2021

Just tested bdf7657, it worked very well! 👍

For compiling against older libuv, it would be super helpful if luv supports it out of the box. If not, my plan was just to upgrade/backport libuv in each distro.

The libuv package versions of most distros can be viewed at https://repology.org/project/libuv/versions (those named libuv1)
My plan is to support Ubuntu 16.04, which provides 1.8.0. If that sounds too old, let's aim for Ubuntu 18.04, which provides 1.18.0.

@aantron
Copy link
Owner

aantron commented Feb 14, 2021

Thanks. I opened #96 to track supporting older libuv.

@andyli
Copy link
Contributor Author

andyli commented Feb 14, 2021

Turn out there is one thing missing for my setup to work. I've to remove the include line at

luv/src/c/dune

Line 28 in 139c7e1

(include_dirs vendor/libuv/include))

That's because I don't checkout the libuv submodule and that line will trigger an error.

aantron added a commit that referenced this issue Feb 14, 2021
Part of #94.

Co-authored-by: Andy Li <andy@onthewings.net>
@aantron
Copy link
Owner

aantron commented Feb 14, 2021

I've removed that line for system builds in the above commit, 3edbc51.

@aantron
Copy link
Owner

aantron commented Feb 16, 2021

All of it together is now out in Luv 0.5.7.

@aantron aantron mentioned this issue Feb 16, 2021
@code-ghalib
Copy link

@aantron, thank you for this! This works well!

Just one thing which you may consider (it's not an issue) - should this install target also be conditioned on LUV_USE_SYSTEM_LIBUV=yes?

It might avoid confusion, by not having two (potentially) different uv.h installed on the system. Particularly because the vendored one wasn't used to compile at all if LUV_USE_SYSTEM_LIBUV=yes.

@aantron
Copy link
Owner

aantron commented Feb 16, 2021

@code-ghalib I've made it so that headers don't get installed when not using the vendored libuv in 2b00946. I'll batch this into the next release, but please ping me if you have any issues and need a release sooner.

This net effect now looks almost exactly like what @andyli initially suggested, so it was pointless for me to try to minimize that original diff :P

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

3 participants