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 to jbuilder #367

Closed
wants to merge 1 commit into from
Closed

Port to jbuilder #367

wants to merge 1 commit into from

Conversation

rgrinberg
Copy link
Contributor

Removes the previous topkg/ocamlbuild setup in favor of jbuilder/topkg. This
splits tls into 3 packages: tls, tls-lwt, and tls-mirage. Since we're still
using topkg, the publishing workflow should be the same as before.

Users of tls shouldn't notice any breakage, as the META is still preserving the
old tls.{mirage,lwt} as subpackages. However, these will now require the
appropriate opam sub packages to be installed to work.

@rgrinberg
Copy link
Contributor Author

Many months after promising this at the conference. Better late than never.

Removes the previous topkg/ocamlbuild setup in favor of jbuilder/topkg. This
splits tls into 3 packages: tls, tls-lwt, and tls-mirage. Since we're still
using topkg, the publishing workflow should be the same as before.

Users of tls shouldn't notice any breakage, as the META is still preserving the
old tls.{mirage,lwt} as subpackages. However, these will now require the
appropriate opam sub packages to be installed to work.
@hannesm
Copy link
Member

hannesm commented Jul 4, 2017

@rgrinberg uh? i expected an async backend from you.

Regarding porting to yet another build system, please take the earlier discussion into account.

Some questions I have about this PR

  • is the resulting binary equivalent to the one generated by ocamlbuild? if not, why not?
  • is the generated documentation the same? how is documentation generated nowadays?
  • what happened to the compiler warnings from our _tags?
  • I usually run the tests with coverage reporting locally, and have a modified pkg.ml (see hannesm/arp@bc57f70) - how would I do this with the modified build system?
  • I still don't have a clear opinion of the advantages of jbuilder
  • I'll wait for @pqwy to comment

@rgrinberg
Copy link
Contributor Author

uh? i expected an async backend from you.

This is a pre-requesite for this work. Unless we want to pointless rename that Core module for everyone :)

is the resulting binary equivalent to the one generated by ocamlbuild? if not, why not?

It's not equivalent but it will not be observable to users. jbuilder uses module aliases to gain better build performance and produce smaller binaries.

is the generated documentation the same? how is documentation generated nowadays?

Documentation is generated through the new odoc tool. Which promises much more usable cross referencing. Should be as simple as $ jbuilder build @doc to generate it (http://jbuilder.readthedocs.io/en/latest/api-doc.html)

what happened to the compiler warnings from our _tags

Ah yes, I can bring those back. In jbuilder, it's customary to enable the warning in development only by passing --dev to the build subcommand. I'm not sure how to customize those yet however. In any case, I can replicate the old build more closely.

I usually run the tests with coverage reporting locally, and have a modified pkg.ml (see hannesm/arp@bc57f70) - how would I do this with the modified build system?

It's possible to support this but it requires a fair bit of annoying boilerplate at the moment. I could get this to work if you were interested in using jbuilder.

I still don't have a clear opinion of the advantages of jbuilder

To start:

  • builds are now much faster and produce smaller binaries.
  • It's now possible to add async support without breakage
  • dependencies now are much more precise (see for example your ptime comment that I've removed).
  • But most importantly, you're saving your users from the scourage of optional dependencies

@dbuenzli
Copy link

dbuenzli commented Jul 4, 2017

But most importantly, you're saving your users from the scourage of optional dependencies

It's not the first time I hear you telling that. For the sake of technical accuracy, note that topkg+ocamlbuild did that long before jbuilder existed...

@rgrinberg
Copy link
Contributor Author

I know, But I'm hopeful that I won't have to repeat myself for much longer.

For the sake of technical accuracy, note that topkg+ocamlbuild did that long before jbuilder existed

I'm sure it's possible somehow, I did say as much in that post. But now you've made me curious, how does one toggle the visibility of dependencies with topkg/ocamlbuild? Given TLS for example, jbuilder will build tls-lwt against the local tls library but when installed through opam, it will use the tls instance installed with findlib.

@dbuenzli
Copy link

dbuenzli commented Jul 4, 2017

now, But I'm hopeful that I won't have to repeat myself for much longer.

You know multiplying the number of toplevel packages is not a very good solution either. I have many packages whose depopts are simply to small to justify the bureaucracy and metadata duplication that is needed by this solution.

But now you've made me curious, how does one toggle the visibility of dependencies with topkg/ocamlbuild?

You don't. jbuilder is a bit more clean in that aspect.

@rgrinberg
Copy link
Contributor Author

rgrinberg commented Jul 4, 2017

While I don't have a set budget for top level names like you, I agree that today the boilerplate required to have correct and precise metadata is a pain in the neck. I just still think it's worth it to have the correct metadata. I'm also waiting for a better system to specify all this boilerplate.

@dbuenzli
Copy link

dbuenzli commented Jul 4, 2017

While I don't have a set budget for top level names like you,

Interesting, you seem to know more about me than myself.

I agree that today the boilerplate required to have correct and precise metadata is a pain in the neck. I just still think it's worth it to have the correct metadata. I'm also waiting for a better system to specify all this boilerplate.

Wait I have an idea, maybe we could... generate that.

More seriously we are still missing something in opam itself to handle this problem.

@rgrinberg
Copy link
Contributor Author

Feel free to correct me. I just see you talking about them like they're a scarce resource. Maybe you're right though, naming things is pretty hard after all.

@dbuenzli
Copy link

dbuenzli commented Jul 4, 2017

Feel free to correct me. I just see you talking about them like they're a scarce resource.

I don't think I ever said they were a scarce ressource. It does however add some clutter to the system in my opinion. But my real worry is the general unDRYness, metadata duplication and the bureaucracy it incurs.

Anyway this is OT for this PR and we can discuss this elsewhere if you wish so.

@aantron
Copy link

aantron commented Jul 11, 2017

@rgrinberg you can enable more warnings like this: ocsigen/lwt@4557ea1

Note not only the --dev flags, but also the :standard -w +A in the jbuild files.

@rgrinberg
Copy link
Contributor Author

rgrinberg commented Jul 12, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants