Port to jbuilder #374

andrewray · 2017-05-23T10:17:20Z

This splits lwt into multiple opam packages, and ties them back together with an empty lwt package with appropriate dependancies and a special META file to map to the old package names.

EDIT: Note from @aantron:

See the summary below.
This will be merged as Port to jbuilder, ocaml-migrate-parsetree (alternate history of #374) #391.

rgrinberg

Added some comments. This looks an awesome and a massive effort forward. I've made some suggestions to tweak the opam files and will give this a more thorough test later.

A couple of general points:

How come the old build system isn't being deleted? Do we still need it for something.
I wonder if lwt-syntax is such a great name. Perhaps something like lwt-camlp4 or even lwt-camlp4-deprecated to signify to people that this package shouldn't really be used.

rgrinberg · 2017-05-23T14:03:56Z

lwt-core.opam

+build: [ [ "jbuilder" "build" "-p" name "-j" jobs ] ]
+build-test: [ [ "jbuilder" "runtest" ] ]
+depends: [
+  "ocamlfind" {build & >= "1.5.0"}


Is findlib still explicitly necessary? jbuilder doesn't really use it anywhere.

Not sure. Jbuilder says it uses it if available, but I am not sure what difference it atually makes.

If ocamlfind is installed it will prefer $ ocamlfind printconf path rather than just looking into opam directly. IMO, reducing dependence on findlib is a good thing as it's just a needless layer of indirection. But it's a minor issue.

Ok, makes sense. Sounds like I should get rid of it.

rgrinberg · 2017-05-23T14:04:14Z

lwt-core.opam

+license: "LGPL with OpenSSL linking exception"
+dev-repo: "https://github.com/ocsigen/lwt.git"
+build: [ [ "jbuilder" "build" "-p" name "-j" jobs ] ]
+build-test: [ [ "jbuilder" "runtest" ] ]


You should add "-p" name in the build-test as well.

Yeah. I added the package stanza to all the runtest aliases then forgot this bit!

rgrinberg · 2017-05-23T14:04:56Z

lwt-ppx.opam

+depends: [
+  "ocamlfind" {build & >= "1.5.0"}
+  "jbuilder" { build & >= "1.0+beta9" }
+  "lwt-core"                            # is this really needed, or only sensible?


I would remove it if it's not entirely necessary. It will give opam an opportunity to parallelize the builds more.

I disagree, lwt-ppx's META should mention that lwt is needed at runtime, which only make sense if lwt-core is an opam dep.

rgrinberg · 2017-05-23T14:05:05Z

lwt-ppx.opam

+license: "LGPL with OpenSSL linking exception"
+dev-repo: "https://github.com/ocsigen/lwt.git"
+build: [ [ "jbuilder" "build" "-p" name "-j" jobs ] ]
+build-test: [ [ "jbuilder" "runtest" ] ]


This build-test needs to be fixed as well.

rgrinberg · 2017-05-23T14:05:45Z

lwt-preemptive.opam

@@ -0,0 +1,27 @@
+opam-version: "1.2"
+name: "lwt"
+version: "dev"


IMO you should remove the version/name from the opam

Please keep the version, it's better for pinning.

rgrinberg · 2017-05-23T14:07:45Z

src/ppx/.merlin

+PKG compiler-libs.common
+PKG ocaml-migrate-parsetree
+PKG ppx_tools_versioned


.merlin file should be deleted

rgrinberg · 2017-05-23T14:09:16Z

META.lwt.template

@@ -0,0 +1,38 @@
+# JBUILDER_GEN
+
+version = "dev"


version here should be removed I believe

Yup, and possibly substituted eventually by topkg or similar.

andrewray · 2017-05-23T14:37:24Z

How come the old build system isn't being deleted? Do we still need it for something.

I've been using the _oasis file as a reference. It can disappear soon.

I wonder if lwt-syntax is such a great name. Perhaps something like lwt-camlp4 or even lwt-camlp4-deprecated to signify to people that this package shouldn't really be used.

Whatever's prefered. It's disturbingly easy to refactor jbuild files...(in fact I used had lwt-camlp4, then changed it to match the old naming).

aantron · 2017-05-23T17:30:22Z

I wonder if lwt-syntax is such a great name. Perhaps something like lwt-camlp4 or even lwt-camlp4-deprecated to signify to people that this package shouldn't really be used.

Whatever's prefered. It's disturbingly easy to refactor jbuild files...(in fact I used had lwt-camlp4, then changed it to match the old naming).

I agree with @rgrinberg that moving away from lwt.syntax is desirable. I'm not sure how that interacts with legacy code that expects lwt.syntax, and the vague goal of not naming an OPAM package differently from a Findlib package, but no new users should be affected by such difficulties.

rgrinberg · 2017-05-23T18:32:36Z

and the vague goal of not naming an OPAM package differently from a Findlib package, but no new users should be affected by such difficulties.

It's contrary to this particular goal, but IMO it's still worth thinking about. I feel uneasy about this dead weight being carried around with lwt and being named so inconspicuously. @hhugo opted to name the camlp4 extension js_of_ocaml-camlp4 fyi.

andrewray · 2017-05-23T18:58:05Z

NOTE: the packages in the PR are actually the same as v3.0 of lwt, not as described below.

FYI;

new opam pkg	old findlib name
lwt-core	lwt
lwt-unix	lwt.unix
lwt-preemptive	lwt.preemptive
lwt-log	lwt.log
lwt-ppx	lwt.ppx
lwt-syntax	lwt.syntax
lwt-react	lwt.react
lwt-simple-top	lwt.simple-top
lwt_glib	lwt_glib
lwt_ssl	lwt_ssl
lwt_react	lwt_react

In addition, the findlib names for the syntax extension package break down into;

new findlib name	old findlib name
lwt-syntax	lwt-syntax
lwt-syntax.log	lwt.syntax.log
lwt-syntax.options	lwt.syntax.options

Note that in the new scheme, the package called lwt doesn't refer to any code and simply installs a META file which maps the new names to the old names. It should probably come with a depreciation notice.

If you want to change the syntax name to lwt-camlp4[-depreciated], we can do so for now without breaking (too much) code by keeping the appropriate mapping.

I note one further inconsistency which is that I have used lwt-xxx whereas the old external packages use lwt_xxx. I prefer the former, but don't mind that much either way. I think the whole namespace should be as consistent as possible, though, so it should be fixed. Again the META mapping trick should keep things fairly compatible whatever is chosen.

Drup · 2017-05-23T21:55:34Z

If we have a compatibility layer that rewires everything, we might as well take this occasion to change the names that we don't like, such as lwt.syntax.

avsm · 2017-05-24T15:31:12Z

omg, this is amazing! :-) See mirage/mirage#818 (comment)

rgrinberg · 2017-05-25T22:37:17Z

Hmm configurator seems to depend on base. Is it acceptable to have that as a build time dependency for Lwt?

andrewray · 2017-05-25T22:40:52Z

Hmm configurator seems to depend on base. Is it acceptable to have that as a build time dependency for Lwt?

It's gone now anyway.

The discover script is rather more sophisticated than configurator at the moment.

andrewray · 2017-05-25T23:41:39Z

Given @diml's comment I refactored the jbuild files to model the current lwt package organisation. Fewest changes possible and all that. This is easy to change and can be addressed seperately.

I have tested with ocaml 4.02.3, 4.03.0, and 4.04.0 on Ubuntu and it seems OK. Related issues;

the jbuild-workspace feature doesn't work due to the ppx for some reason.
lambda-term doesn't compile on MacOSX for a reason I don't understand yet.
no Windows, BSD or Android testing

The main remaining issue is how to properly expose configuration of lwt.unix to its discover script. The question is how to tell jbuilder about the conf-libev opam package.

A minor unresolved issue is how to hide the Lwt_config and Lwt_unix_jobs_generated modules.

andrewray · 2017-05-25T23:54:15Z

By the way, jbuilder organises sub-packages into their own directories so it should fix #364 (and break some dependent packages which are to some extent broken anyway - see bad_packages.md).

aantron · 2017-05-26T12:17:37Z

Thanks for the ongoing work!

A minor unresolved issue is how to hide the Lwt_config and Lwt_unix_jobs_generated modules.

It would be nice to hide these, but I think not critical. I think we can assume that nobody has modules with these names, and we can check that in OPAM.

fixes the linkall issue on the ppx

jbuilder seems to prefer an opam file per package. An empty opam file `lwt.opam` depends on the usual suspects. It just installs a META files and reexports packages under their expected names.

this should configure unix the same way as oasis did (by default).

parse `ocamlc -config` for parameters that used to come from oasis. adds base and stdio as dependancies

this isn't quite right for glib/ssl/react. maybe better to make the jbuild version use lwt_xxx throughout as that will be most compatible

looks in other packages directories to do this, which seems unclean but I dont know how else to do it.

The changes proposed in previous commits turn out to not be unnecessary.

aantron · 2017-06-08T15:18:56Z

Ok, I think the PPX issue doesn't block this PR then. Thanks for finding and diagnosing it :)

Regarding 4.05, I'll edit the Travis build shortly after merging, and we'll have testing on it again.

Mostly editing whitespace to restore lines to their former state; this makes it easier for git to identify which file was renamed from where. See ocsigen#374 (comment) Also restored constraints on lwt_react, and restored package names.

Mainly to reduce the net diff, but also to keep the OCaml version constraint. We shouldn't rely on jbuilder requiring 4.02.3 as a lower bound. Even though jbuilder is not likely to support 4.02.0, it's better to keep the inherent 4.02.0 constraint of Lwt in its own opam file, for now.

Since we decided not to change it in this PR :) This reduces the net diff slightly. More importantly, it eliminates the merge conflict, allowing the CI builds to run again without having to do a merge or rebase first.

- Restore authors. As far as I know, they were accurate. - Avoid moving the dev-repo field. We can move it later, if needed. - Restore constraint on Lwt in lwt_glib.

Doing this before the squash because it is harmful to do it in a separate commit to master afterwards: it will pollute the blame. [skip ci]

[skip ci]

Previewing rewriting of #374. [skip ci]

@andrewray

This also ports the PPX to ocaml-migrate-parsetree. The work in this commit, and the preceding one, was done by Andrew Ray (@andrewray) in #374. These two commits are just a more blame-friendly version; the detailed history is visible #374. Some number of follow-on commits will probably be needed to fully stabilize and finalize the port. Also, this commit breaks some packages that depend on Lwt incorrectly. We will probably take steps to break them more softly than is being done by this commit alone. Builds on 4.05 are temporarily disabled, due to jbuilder not working with the 4.05 betas. We will address this by building on trunk later. This port disables coverage via Bisect completely (#319). We will restore that in follow-on work; in the meantime, we expect few enough edits to the test suite to be made that we can run the test suite with coverage in a separate branch that does not yet have this commit, and cherry-pick commits over if any development of the test suite occurs. The build system produced by this commit is a bit of a chimera: it adopts jbuilder without adopting modern packaging best practices. We need to do the latter slowly, to avoid breaking projects that depend on Lwt without proper warning (#293). Work along this direction had already started (#301, #338, #370), and this jbuilder port will make it easier to continue. The port also pre-emptively solves a lot of other issues with the build system. See the pull request for any issue links, to be added over the coming days. Maintainer's note: squash and summary done by Anton Bachin (@aantron).

aantron · 2017-06-08T20:05:45Z

Summary of what happened in this PR:

In a massive effort, @andrewray ported Lwt to jbuilder, but without splitting Lwt up into multiple OPAM packages, as originally stated. The Lwt project will do that in follow-on work over the next few months, mainly to avoid breaking projects that depend on Lwt (see #293 regarding Lwt's soft breakage policy).

The Lwt build is now faster: 5s, compared to 15s.
Lwt should now be composable with other jbuilder projects.

This achievement is not to be understated: Lwt is one of the few OCaml projects tested on Cygwin and Win32, in addition to having many legacy quirks that we can only eliminate slowly. Thanks @andrewray!

Several things are temporarily broken by this PR, which we will fix later (and definitely before the next release). See the message of commit 2163fb1 for details.

This PR will be merged as #391, which has a simplified history for master.

@andrewray

This also ports the PPX to ocaml-migrate-parsetree. The work in this commit, and the preceding one, was done by Andrew Ray (@andrewray) in #374. These two commits are just a more blame-friendly version; the detailed history is visible #374. Some number of follow-on commits will probably be needed to fully stabilize and finalize the port. Also, this commit breaks some packages that depend on Lwt incorrectly. We will probably take steps to break them more softly than is being done by this commit alone. Builds on 4.05 are temporarily disabled, due to jbuilder not working with the 4.05 betas. We will address this by building on trunk later. This port disables coverage via Bisect completely (#319). We will restore that in follow-on work; in the meantime, we expect few enough edits to the test suite to be made that we can run the test suite with coverage in a separate branch that does not yet have this commit, and cherry-pick commits over if any development of the test suite occurs. The build system produced by this commit is a bit of a chimera: it adopts jbuilder without adopting modern packaging best practices. We need to do the latter slowly, to avoid breaking projects that depend on Lwt without proper warning (#293). Work along this direction had already started (#301, #338, #370), and this jbuilder port will make it easier to continue. The port also pre-emptively solves a lot of other issues with the build system. See the pull request for any issue links, to be added over the coming days. Maintainer's note: squash and summary done by Anton Bachin (@aantron).

@andrewray

This also ports the PPX to ocaml-migrate-parsetree. The work in this commit, and the preceding one, was done by Andrew Ray (@andrewray) in #374. These two commits are just a more blame-friendly version; the detailed history is visible #374. Some number of follow-on commits will probably be needed to fully stabilize and finalize the port. Also, this commit breaks some packages that depend on Lwt incorrectly. We will probably take steps to break them more softly than is being done by this commit alone. Builds on 4.05 are temporarily disabled, due to jbuilder not working with the 4.05 betas. We will address this by building on trunk later. This port disables coverage via Bisect completely (#319). We will restore that in follow-on work; in the meantime, we expect few enough edits to the test suite to be made that we can run the test suite with coverage in a separate branch that does not yet have this commit, and cherry-pick commits over if any development of the test suite occurs. The build system produced by this commit is a bit of a chimera: it adopts jbuilder without adopting modern packaging best practices. We need to do the latter slowly, to avoid breaking projects that depend on Lwt without proper warning (#293). Work along this direction had already started (#301, #338, #370), and this jbuilder port will make it easier to continue. The port also pre-emptively solves a lot of other issues with the build system. See the pull request for any issue links, to be added over the coming days. Maintainer's note: squash and summary done by Anton Bachin (@aantron).

@andrewray

This also ports the PPX to ocaml-migrate-parsetree. The work in this commit, and the preceding one, was done by Andrew Ray (@andrewray) in #374. These two commits are just a more blame-friendly version; the detailed history is visible #374. Some number of follow-on commits will probably be needed to fully stabilize and finalize the port. Also, this commit breaks some packages that depend on Lwt incorrectly. We will probably take steps to break them more softly than is being done by this commit alone. Builds on 4.05 are temporarily disabled, due to jbuilder not working with the 4.05 betas. We will address this by building on trunk later. This port disables coverage via Bisect completely (#319). We will restore that in follow-on work; in the meantime, we expect few enough edits to the test suite to be made that we can run the test suite with coverage in a separate branch that does not yet have this commit, and cherry-pick commits over if any development of the test suite occurs. The build system produced by this commit is a bit of a chimera: it adopts jbuilder without adopting modern packaging best practices. We need to do the latter slowly, to avoid breaking projects that depend on Lwt without proper warning (#293). Work along this direction had already started (#301, #338, #370), and this jbuilder port will make it easier to continue. The port also pre-emptively solves a lot of other issues with the build system. See the pull request for any issue links, to be added over the coming days. Maintainer's note: squash and summary done by Anton Bachin (@aantron).

aantron · 2017-06-09T00:42:23Z

FYI the ocsigen.org manual seems to have survived this PR :) If any minor issues develop, we can fix them later.

avsm · 2017-06-12T11:36:03Z

I just wanted to express my appreciation for all the hard work that went into this PR. We've been porting a large chunk of the Mirage package repository with a view to using a unified jbuilder build, and Lwt being ported gives us a large boost in this direction. Thank you!

@andrewray

Module Markup_lwt_unix depends on Findlib packages lwt and lwt.unix. However, the only dependency actually listed in _tags was on package lwt. Until recently, Lwt installed all files in a flat directory. Depending on package lwt causes ocamlfind to add a -I option for that directory; since the directory was flat, that option was sufficient to be able to accidentally find the files of lwt.unix as well. Lwt now installs files in a directory hierarchy. In particular, lwt.unix is now installed in a separate subdirectory. The -I option implied by depending on package lwt is therefore no longer accidentally sufficient for using lwt.unix, and this causes an error during compilation of Markup.ml if Lwt is present. So, this commit does the right thing, and adds the proper, explicit dependency on lwt.unix. This bug was originally found in Markup.ml, as well as several other packages, by @andrewray during ocsigen/lwt#374. Fixes #18. See also ocsigen/lwt#401.

The new jbuilder build system, added in #374, does not emit warnings about duplicate topdirs.cmi files.

This was originally part of #374, but I wasn't sure what the right way to depend on ppx_tools_versioned and ocaml-migrate-parsetree is. However, since we directly use modules from OMP in the Lwt PPX, it now seems reasonable to depend on ocaml-migrate-parsetree directly.

See ocsigen/lwt#374

It is for the OASIS build system, and obsolete since the port to Jbuilder (#374). [skip ci]

rgrinberg suggested changes May 23, 2017

View reviewed changes

aantron mentioned this pull request May 26, 2017

Difficulty of building lwt on a fresh system #380

Closed

andrewray added 17 commits May 26, 2017 14:12

almost working ppx for jbuilder; just cant get linkall to apply...

dbb4963

magic cmxs ocamlbuild rule courtest @jdimino

0fa50d2

fixes the linkall issue on the ppx

1st pass at jbuilder

68b161f

add configurator dep

30b8a80

opinionated reorganisation of packages

de3d931

jbuilder seems to prefer an opam file per package. An empty opam file `lwt.opam` depends on the usual suspects. It just installs a META files and reexports packages under their expected names.

add lwt.ppx to lwt. notes on a few bad package builds

291f605

add parsing of ocamlc -config to discover.ml

e721933

this should configure unix the same way as oasis did (by default).

rename syntax to camlp4. Add -p to opam test commands

a988946

missed a syntax->camlp4 conversion in META

a384933

add proper configuration for glib

2da8764

add flag detection for lwt-unix

e9f0907

parse `ocamlc -config` for parameters that used to come from oasis. adds base and stdio as dependancies

new package lwt-xxx, map to old name lwt.xxx

512d277

this isn't quite right for glib/ssl/react. maybe better to make the jbuild version use lwt_xxx throughout as that will be most compatible

remove old build system

e13bac7

add synopsis fields

7b6d9d0

use exists_if in lwt META to only shadow installed packages

94383e4

looks in other packages directories to do this, which seems unclean but I dont know how else to do it.

note build problems on 4.03

b28726b

Refact libraries to match current lwt configuration

b825120

The changes proposed in previous commits turn out to not be unnecessary.

aantron added 4 commits June 8, 2017 10:55

Fix up lwt_react, lwt_ssl, lwt_glib opam files

c075a63

Mostly editing whitespace to restore lines to their former state; this makes it easier for git to identify which file was renamed from where. See ocsigen#374 (comment) Also restored constraints on lwt_react, and restored package names.

Revert change to README.md

0a33c49

Since we decided not to change it in this PR :) This reduces the net diff slightly. More importantly, it eliminates the merge conflict, allowing the CI builds to run again without having to do a merge or rebase first.

More fixing up of lwt_react, lwt_ssl, lwt_glib

7ca76aa

- Restore authors. As far as I know, they were accurate. - Avoid moving the dev-repo field. We can move it later, if needed. - Restore constraint on Lwt in lwt_glib.

aantron approved these changes Jun 8, 2017

View reviewed changes

Nits: remove some trailing whitespace

e0077c3

Doing this before the squash because it is harmful to do it in a separate commit to master afterwards: it will pollute the blame. [skip ci]

aantron pushed a commit that referenced this pull request Jun 8, 2017

Previewing the blame for a squashed #374.

7a5e075

[skip ci]

One last trailing whitespace nit!

9fb2f5d

[skip ci]

aantron pushed a commit that referenced this pull request Jun 8, 2017

Port to jbuilder

1497f24

Previewing rewriting of #374. [skip ci]

aantron mentioned this pull request Jun 8, 2017

Port to jbuilder, ocaml-migrate-parsetree (alternate history of #374) #391

Merged

aantron closed this in #391 Jun 8, 2017

aantron mentioned this pull request Jun 9, 2017

The paths of Lwt's installed cmi files #364

Closed

aantron mentioned this pull request Jun 13, 2017

Fix dependency on lwt.unix and release to OPAM aantron/markup.ml#18

Closed

aantron added a commit that referenced this pull request Jun 14, 2017

Build: remove topdirs.cmi warning suppression

af0ae5b

The new jbuilder build system, added in #374, does not emit warnings about duplicate topdirs.cmi files.

aantron added a commit that referenced this pull request Jun 15, 2017

Build: remove topdirs.cmi warning suppression

7bcc581

The new jbuilder build system, added in #374, does not emit warnings about duplicate topdirs.cmi files.

aantron mentioned this pull request Jul 2, 2017

Restore warnings as errors in CI #431

Closed

aantron mentioned this pull request Jul 19, 2017

Add Lwt 3.1.0 – concurrency library ocaml/opam-repository#9876

Merged

g2p added a commit to g2p/ocaml-xenstore-clients that referenced this pull request Jul 20, 2017

Add missing lwt.unix dependency

f85635e

See ocsigen/lwt#374

g2p mentioned this pull request Jul 20, 2017

Add missing lwt.unix dependency xapi-project/ocaml-xenstore-clients#9

Merged

aantron added a commit that referenced this pull request Jul 25, 2017

Packaging: delete dist.sh

3ca748e

It is for the OASIS build system, and obsolete since the port to Jbuilder (#374). [skip ci]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port to jbuilder #374

Port to jbuilder #374

andrewray commented May 23, 2017 •

edited by aantron

Loading

rgrinberg left a comment

rgrinberg May 23, 2017

andrewray May 23, 2017

rgrinberg May 23, 2017

andrewray May 23, 2017

rgrinberg May 23, 2017

andrewray May 23, 2017

rgrinberg May 23, 2017

Drup May 23, 2017 •

edited

Loading

rgrinberg May 23, 2017

rgrinberg May 23, 2017

Drup May 23, 2017

rgrinberg May 23, 2017

rgrinberg May 23, 2017

andrewray May 23, 2017

andrewray commented May 23, 2017

aantron commented May 23, 2017

rgrinberg commented May 23, 2017

andrewray commented May 23, 2017 •

edited

Loading

Drup commented May 23, 2017

avsm commented May 24, 2017

rgrinberg commented May 25, 2017

andrewray commented May 25, 2017

andrewray commented May 25, 2017

andrewray commented May 25, 2017

aantron commented May 26, 2017

aantron commented Jun 8, 2017

aantron commented Jun 8, 2017

aantron commented Jun 9, 2017

avsm commented Jun 12, 2017

Port to jbuilder #374

Port to jbuilder #374

Conversation

andrewray commented May 23, 2017 • edited by aantron Loading

rgrinberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drup May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewray commented May 23, 2017

aantron commented May 23, 2017

rgrinberg commented May 23, 2017

andrewray commented May 23, 2017 • edited Loading

Drup commented May 23, 2017

avsm commented May 24, 2017

rgrinberg commented May 25, 2017

andrewray commented May 25, 2017

andrewray commented May 25, 2017

andrewray commented May 25, 2017

aantron commented May 26, 2017

aantron commented Jun 8, 2017

aantron commented Jun 8, 2017

aantron commented Jun 9, 2017

avsm commented Jun 12, 2017

andrewray commented May 23, 2017 •

edited by aantron

Loading

Drup May 23, 2017 •

edited

Loading

andrewray commented May 23, 2017 •

edited

Loading