search

ygrek / ocurl

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

Fix compilation on Windows/msvc #22

Closed nojb closed 7 years ago

nojb commented 7 years ago

As the title says I managed to compile on Windows using msvc compilers after making these changes. The main issue is that variable declarations are only allowed after an opening brace.

I am not very familiar with the blocking_section stuff but I looked at the definition and convinced myself that these changes should be safe.

I only compiled, not yet tested.

What do you think ?

nojb commented 7 years ago

Also it would be nice to get rid of the ExtLib dependency in errors.ml.

nojb commented 7 years ago

Also if we manage to fix the msvc compilation we should add that to AppVeyor to make sure it stays fixed :)

nojb commented 7 years ago

I added one more commit removing the ExtLib dependency for errors.ml.

nojb commented 7 years ago

Actually if I am not mistaken, errors.ml is dead code after all.

ygrek commented 7 years ago

re errors.ml : is not dead code, it is used to generate error list in Curl module (but it is not needed for build per se). re Makefile.msvc : looks like it is now relying on a unix shell, while previously it was usable directly from cmd. I am not sure anybody uses such setup anymore, because other tools and libs require shell.. So I guess it is fine to just document this dependency in comment. re locking : apparently bfa428593921ce1073a1ce2a286f294b1c85f653 broke C89 support, changes look good on first sight, will review more thoroughly later

nojb commented 7 years ago

Thanks @ygrek for the comments. OK on errors.ml. Re Makefile.msvc I am not really using it myself (I am building ocurl with a different build system) so no problem if you prefer to revert that change.

nojb commented 7 years ago

Also, for information, there was one use of CAMLlocalN which is not C89-compatible (see https://github.com/ocaml/ocaml/commit/2dd92969d2#commitcomment-6191182).

ygrek commented 7 years ago

it's a shame how microsoft cannot update C compiler to at least 20-year old standard, while pushing F# at the same time..

https://herbsutter.com/2012/05/03/reader-qa-what-about-vc-and-c99/ MS recommended way is to compile with C++ compiler but do not use any C++ features, and this will be easily caugh by CI, might be worth a try?

ygrek commented 7 years ago

I have started CI setup for msvc in https://github.com/ygrek/ocurl/commits/msvc I believe registering local roots before exiting blocking section is not safe (it caches caml_local_roots which can be updated before global lock is taken by our thread). So I am eager to try C++ compiler solution outlined above, or switch back to the previous approach of having one extra wrapper function for each callback solely for managing global lock :(

ygrek commented 7 years ago

branch went in wrong direction, using cygwin package with msvc will not work I guess, need to install native windows package and use Makefile.msvc. Need to investigate if https://github.com/fdopen/opam-repository-mingw will help here.

nojb commented 7 years ago

Thanks for the feedback! Personally, I am in a situation where I am not able to use the C++ compiler, so would prefer a solution that works with the C compiler. I will study the issue and amend this PR as needed.

Also, I have some experience building native msvc OCaml packages - will take a look tomorrow and see if I can help with the msvc CI.

ygrek commented 7 years ago

I am in a situation where I am not able to use the C++ compiler, so would prefer a solution that works with the C compiler.

Hm, but cl.exe is at the same time a c and c++ compiler. I would be curious to know your circumstances.

I can help with the msvc CI.

This is most welcome.

nojb commented 7 years ago

Hm, but cl.exe is at the same time a c and c++ compiler. I would be curious to know your circumstances.

Yes, of course you are right - I don't know what I was thinking last night. I'll try it and report back.

nojb commented 7 years ago

OK, I adapted curl-helper.c so that it can be compiled with the C++ compiler. The diff is very small and it compiles cleanly . The examples seem to work, so I am happy enough with this . What do you think ? (I haven't tested Makefile.msvc but will be revisiting it when I look at the msvc CI).

nojb commented 7 years ago

Well, the same define is passed on the command line in Makefile.msvc so I get a redefinition warning, so it was a quick fix for that. I guess we don't need to pass it on the command line at all and then the #ifndef won't be needed anymore.

nojb commented 7 years ago

Thanks! I will submitting another PR with the msvc CI soon.

ygrek commented 7 years ago

Thanks for working on this.

I guess we don't need to pass it on the command line at all and then the #ifndef won't be needed anymore.

Yes, this will be better, but I don't see this define anywhere in makefiles, it is probably passed by ocaml compiler itself?