ygrek / ocurl

OCaml bindings to libcurl
https://ygrek.org/p/ocurl
MIT License
59 stars 32 forks source link

build with dune instead of autotools #38

Closed olafhering closed 2 years ago

olafhering commented 4 years ago

Two branches in my fork build with dune instead of autotools (dune-0.9.0 and dune-0.9.1). This appears to work fine.

For some reason discover.ml is slightly slower than configure.ac. Someone who is skilled enough with OCaml will likely spot where the bottleneck is.

ygrek commented 4 years ago

ftr one reason I am not happy about ocurl build system is slowness of configure. Another is that adding new option requires several manual edits with a potential for typos. I understand this change addresses the latter hopefully, but not the former :( What is the motivating reason for this change? Anyway, a PR against master would make it easier to review.

olafhering commented 4 years ago

dune does everything right when building OCaml based projects. oasis does most things right.

The same can not be said for every other approach to build OCaml things. Each one failed for me in one way or another at some point.

Regarding performance, not sure why discover.ml is so much slower while doing essentially the very same thing as configure. Independent of that, perhaps removing most checks and test only for the minimum will already help. There is also pkgconfig which already gives the version.

zapashcanon commented 3 years ago

@olafhering, are you still interested in this ? If yes, could you open a PR so it can be reviewed by anyone willing to do it ?

olafhering commented 3 years ago

Yes. I will see how to improve the change.

I was thinking of removing most, or all, checks and just assume they exist. If pkgconfig is found, use that. Perhaps there are ways to query the curl version via cpp macros.

ygrek commented 3 years ago

ftr current build works well for me, and I am still curious about the specific arguments to change (every change is more breakage than expected, so there has to be some rationale for the benefit of the change).

I was thinking of removing most, or all, checks and just assume they exist

this is clear step-back from existing build setup..

If pkgconfig is found, use that

that's what configure does currently

Perhaps there are ways to query the curl version via cpp macros.

Yes, of course, see curlver.h but how it will help? We don't want to hardcode checks based on version, instead of checking for symbol presence, it is too fragile and laboursome.

olafhering commented 3 years ago

You can not have both.

Either a fast configure, which may just check some version info to provide compatibility (with what anyway?). Or each and every used string is checked, even if it is always present anyway.

For why, see 2020-06-18 above.

ygrek commented 3 years ago

2020-06-18 talks about some unspecified experience of yours with some other projects. I am asking of any specific reasons for ocurl.

nojb commented 3 years ago

Some general comments:

rgrinberg commented 3 years ago

@olafhering would you like to make a PR? we can help out with the changes nojb is proposing.

olafhering commented 3 years ago

Once I have time to work on it, my approach will be a simple pkgconfig(curl) check, manually declare all the expected defines to keep the delta small, and be done with it. For bonus points I may consider some random stale version of curl, like 7.37.0 as included in SUSE:SLE-12:GA, as a base.

It has been a while since I last looked at ocurl.

ygrek commented 2 years ago

56