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

Port the build system to jbuilder + Configurator (?) #365

Closed
aantron opened this issue May 20, 2017 · 8 comments · Fixed by #391
Closed

Port the build system to jbuilder + Configurator (?) #365

aantron opened this issue May 20, 2017 · 8 comments · Fixed by #391
Milestone

Comments

@aantron
Copy link
Collaborator

aantron commented May 20, 2017

cc @rginberg

@diml: We have a question: would current jbuilder be able to build Lwt?

@c-cube
Copy link
Collaborator

c-cube commented May 20, 2017

If you find a good solution for cppo and the docs, I'm interested too.

@rgrinberg
Copy link
Contributor

rgrinberg commented May 20, 2017

Yes, cppo is easy to handle. Especially since we only have a couple of files being preprocessed.

For docs, we need only wait for ocaml/dune#74 to be finished. Which should be done before 1.0 is released.

@ghost
Copy link

ghost commented May 22, 2017

Yh, there are two methods shown in the manual for cppo.

Otherwise yh jbuilder should be able to build Lwt without problems.

@andrewray
Copy link
Contributor

opam pin add -k git lwt https://github.com/andrewray/lwt#jbuild

or clone and

jbuilder runtest

I've done a good chunk of the grunt work, leaving just the difficult bits!

  • No enabling/disabling of the sub-libraries (though the discover tools do run). Everything is assumed on.
    Exactly how this can/should be expressed through jbuilder needs some discussion.

  • I've reorganised the discovery build process a bit, however, it is still basically the same code. One approach might be to modify the discover script to generate sexps (like configurator does)

  • A couple of generated modules are not hidden (Lwt_config, Lwt_unix_jobs_generated at least). I think the directory organisation will have to change to work around that.

  • Lwt_glib/ssl/react are called Lwt.glib/ssl/react. This just needs new opam files at the top level and a couple of minor changes to the jbuild files.

  • I have found 1 issue in utop - Lwt_main isn't automatically found.

  • The ppx is based on ocaml-migrate-parsetree

@aantron
Copy link
Collaborator Author

aantron commented May 23, 2017

leaving just the difficult bits!

😆

  • No enabling/disabling of the sub-libraries (though the discover tools do run). Everything is assumed on.
    Exactly how this can/should be expressed through jbuilder needs some discussion.

Some info:

Lwt_ssl, Lwt_glib, and Lwt_react should always be off in the main build, I just didn't bother editing it to remove them. They were factored out in #335. These (obviously) correspond to --disable-ssl, --disable-glib, and --disable-libev.

We want to do the same thing to the PPX (#338) and Camlp4 (#370).

I don't know yet enough jbuilder to recommend a specific course of action based on this. This is FYI.

  • I've reorganised the discovery build process a bit, however, it is still basically the same code. One approach might be to modify the discover script to generate sexps (like configurator does)

Is it feasible to use Configurator?

  • Lwt_glib/ssl/react are called Lwt.glib/ssl/react. This just needs new opam files at the top level and a couple of minor changes to the jbuild files.

I don't think we necessarily want this. We really are thinking of them as separate libraries, which happen to be in this repo for historical reasons. Then again, maybe we == me, and everybody else wants them as submodules of Lwt :)

@rgrinberg
Copy link
Contributor

I've done a good chunk of the grunt work, leaving just the difficult bits!

Awesome! put up a PR and we'll try and help you with those.

Lwt_ssl, Lwt_glib, and Lwt_react should always be off in the main build,

👍

Is it feasible to use Configurator?

Yes, I think this is essential.

@andrewray
Copy link
Contributor

No enabling/disabling of the sub-libraries (though the discover tools do run). Everything is assumed on.
Exactly how this can/should be expressed through jbuilder needs some discussion.
Some info:

Lwt_ssl, Lwt_glib, and Lwt_react should always be off in the main build, I just didn't bother editing it to remove them. They were factored out in #335. These (obviously) correspond to --disable-ssl, --disable-glib, and --disable-libev.

We want to do the same thing to the PPX (#338) and Camlp4 (#370).

The pull request factors everything into separate packages, then ties them back together and provides a compatibility package. This is quite similar to what js_of_ocaml is doing for jbuilder. It also seems to match best jbuilders workflow - I haven't yet figured out how to otherwise disable individual packages, though perhaps @diml would like to comment.

I don't know yet enough jbuilder to recommend a specific course of action based on this. This is FYI.

I've reorganised the discovery build process a bit, however, it is still basically the same code. One approach might be to modify the discover script to generate sexps (like configurator does)
Is it feasible to use Configurator?

It is, and it's already used to get the glib pkg-config.

I am in two minds about a wholesale re-write of discover.ml, though maybe it can be simplified a bit with configurator. It's the next problem to tackle, to ensure it works on different platforms correctly.

@ghost
Copy link

ghost commented May 23, 2017

Multiple packages is best to avoid recompilation when the user installs of one the optional dependency. Otherwise simply mark libraries as optional:

(library
 ((name lwt_ssl)
   (public_name lwt.ssl)
   (optional)
   (libraries (ssl ...))
   ...))

Then lwt.ssl will only be built and installed if ssl is available. This won't work for glib though, since there is no additional dependency on an OCaml library.

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 a pull request may close this issue.

4 participants