Port to jbuilder #192

rgrinberg · 2017-03-24T04:18:43Z

Some things to note:

The jsoo package isn't tested at all
lambda-term is a dependency for all binaries in jar/ because jbuilder
doesn't allow for binaries in the same dir with different config.

dsheets · 2017-03-24T09:57:05Z

Thanks for trying this out!

On Linux:

# File "gist/jbuild", line 6, characters 3-10:
# Error: Unknown field package

On Windows, it looks like there's a problem with the CI scripts not knowing the opam package name?

Was renaming lib/github_s.mli to lib/github_s.ml necessary for the build?

rgrinberg · 2017-03-24T13:50:22Z

Yes it was necessary. Jbuilder shows ugly warnings on mli only modules. You will need to pin jbuilder to master for the package option to be available. Or wait until @diml makes another release.

…

On Fri, Mar 24, 2017, 5:57 AM David Sheets ***@***.***> wrote: Thanks for trying this out! On Linux: # File "gist/jbuild", line 6, characters 3-10: # Error: Unknown field package On Windows, it looks like there's a problem with the CI scripts not knowing the opam package name? Was renaming lib/github_s.mli to lib/github_s.ml necessary for the build? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#192 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIe-7F0cQXEsZZquWcbXwYt5BCaEMZ5ks5ro5NygaJpZM4MnqU3> .

avsm · 2017-03-31T10:19:01Z

sorry, #193 will conflict with this -- will rebase once thats merged.

rgrinberg · 2017-03-31T13:41:49Z

Why will it conflict? I've only added new files here and haven't touched any of the oasis stuff.

avsm · 2017-03-31T14:58:21Z

Oh I thought it would have removed oasis -- all is good with the world then.

rgrinberg · 2017-05-02T15:22:51Z

@avsm @samoht should we rename the github-js to github-jsoo here as well?

avsm · 2017-05-02T16:05:59Z

@rgrinberg yes -- I think jsoo is fine, to be consistent among our libraries.

rgrinberg · 2017-05-02T16:35:29Z

@dsheets, @avsm this is ready to be considered for merging.

dsheets · 2017-05-02T16:39:08Z

I'd really like to make this transition but I'm still rather disappointed about the mli becoming an ml... can we get jbuilder support for mli only files?

rgrinberg · 2017-05-02T16:42:09Z

I was a bit inaccurate in my previous comment btw. jbuilder does support mli only modules in a primitive way by creating an .mli -> .ml copying rule. We can just rely on this copying rule but I wouldn't like that. It shows an ugly warning and doesn't handle some corner cases. But if you'd like, we can still use it.

See this ticket for why it will not get full mli only support: ocaml/dune#9

rgrinberg · 2017-05-07T05:53:17Z

@dsheets is the .mli only issue a deal breaker for you?

dsheets · 2017-05-08T09:17:57Z

I don't know if it's a deal breaker but I'm interested in any work-around. Why would you dislike the copying rule? What are the edge cases that we are exposed to? How scary is the warning? I know that I could answer these questions myself but I'm extremely short on time recently -- sorry :-/.

rgrinberg · 2017-05-09T17:03:53Z

The warning looks like this:

Warning: Module No_impl in . doesn't have a corresponding .ml file.
Modules without an implementation are not recommended, see this discussion:

  https://github.com/janestreet/jbuilder/issues/9

In the meantime I'm setting up a rule for copying no_impl.mli to no_impl.ml.

The edge case where the copying rule doesn't work is described here: ocaml/dune#9 (comment)

This doesn't affect Github_s but I've yet to see any arguments why we've insisted on having this .mli only module is so important. So I'm not convinced for the need for hacky support. However, I am convinced by diml's comment that mli only modules don't work very well with aliases without more compiler support. And I do intend to transition Github_s to Github.S eventually

dsheets · 2017-05-10T10:13:54Z

@rgrinberg the argument for the mli-only module is to provide a single place where the interface is defined and documented. I don't think we need the module to be mli-only but we do need the module signatures to exist in an mli (not just an bare ml) with ocamldoc comments. The cost of that is a duplication of the module signatures in both mli and ml (without doc comments) which I'm not thrilled about but seems acceptable.

I'm happy to have Github_s live in Github.S and perhaps have Github_core renamed to Github with or without some combination of module aliases to segment the interface from the implementation. This split was originally introduced with @andrewray's JS backend in #36 in order to eliminate signature duplication, afaict.

If you could simply keep the github_s.mli file and add a github_s.ml file which contains the signature, I think we should merge. Thanks for your work on this and sorry for the delay!

avsm

this is awesome, thank you @rgrinberg!

avsm · 2017-05-10T10:22:45Z

Makefile

-.PHONY: all clean install build
-PREFIX ?= /usr/local
+all:
+	jbuilder build @install -j 3


should this be jbuilder build --dev (for stricter warnings) and then it autofigures out the right value of -j (i have 16 cores on my desktop)

--dev adds too many warnings. I'll address those in a separate PR

avsm · 2017-05-10T10:23:38Z

github-unix.opam

@@ -0,0 +1,36 @@
+opam-version: "1.2"
+name: "github"
+version: "2.2.0"


can remove name and version here (the name is wrong anyway)

avsm · 2017-05-10T10:23:57Z

github-jsoo.opam

+]
+depends: [
+  "jbuilder" {build}
+  "cohttp"


probably good to put a minimum cohttp constraint here given its rich and storied history

ghost · 2017-05-10T10:32:48Z

BTW, if you write an explicit rule to copy the file, the warning will go away:

(rule
 ((targets (foo.ml))
   (deps   (foo.mli))
   (action (copy-and-add-line-directive ${<} ${@}))))

dsheets · 2017-05-10T10:35:44Z

github-jsoo.opam

@@ -0,0 +1,32 @@
+opam-version: "1.2"
+name: "github"
+version: "2.2.0"


The name and version are wrong here, too.

dsheets · 2017-05-10T10:39:41Z

The explicit copy rule seems fine to me. As far as I can tell from ocaml/dune#9 (comment), the edge case arises when copying an ml to an mli which is exactly what we aren't doing (unless that was a mistaken assertion). I don't see any edge cases about mli -> ml.

rgrinberg · 2017-05-10T17:19:06Z

@dsheets the explicit copying rule is added

rgrinberg · 2017-05-11T18:23:56Z

All that remains for this PR is travis

dsheets · 2017-05-12T10:24:35Z

This will change the opam name of the library as used in dependencies that currently rely on, e.g., github.unix. I think this requires a major version bump so let's go to 3.0 and update all the current depopt dependencies to top out at less than 3.0. I'll make an opam-repository PR for that. Then, before release of 3.0, we should remove all deprecated features and get #196 merged.

Thanks for contributing this!

dsheets · 2017-05-12T10:45:48Z

META.github.template

+package "unix" (
+  description = "Deprecated. Use github-unix directly"
+  requires = "github-unix"
+)


I think we should just break this and go to 3.0.

it's useful to keep this for a release so that old packages can just have their opam build deps changed, rather than having to re-release or upper-bound them.

but this doesn't hold if you are doing other breaking API changes of course, so a 3.0 is probably more appropriate to not have this.

The problem is that depending on github-unix will implicitly depend on the next release and this release must exist for packages to depend on. We've got some other deprecated cruft so it seems simpler to just break compat for the small number of downstream packages.

dsheets · 2017-05-21T15:06:55Z

I've rebased and added back some warnings but I can't work out how to get the new subpackages to build during development. I get an error like:

jbuilder build -p github-unix
File "lib_test/jbuild", line 5, characters 11-22:
Warning: This field is useless without a (public_names ...) field.
Error: External library "github" not found.
-> required by "unix/jbuild (context default)"
Hint: try: jbuilder external-lib-deps --missing -p github-unix @install
make: *** [github-unix] Error 1

I'm not sure what the warning is about, either. What am I missing?

I updated the Makefile to build all of the packages so that developers don't get nasty surprises during CI testing (see recent failing CI test) but I can't work out how to build github-unix without the github findlib package installed. Halp?

rgrinberg · 2017-05-21T15:32:01Z

Hmm, I'm not sure such a way exists. The point of -p is to hide jbuild defined packages on your file system. So I guess what you're looking for is to build @install for a particular package. Let's see what @diml has to say.

ghost · 2017-05-22T08:46:05Z

-p is meant for opam releases. @install is simply an alias for all the buildable <package>.install files, as explained here. So I'm assuming that what you are looking for is simply:

$ jbuilder build github-unix.install

dsheets · 2017-05-22T11:10:18Z

Thanks @rgrinberg for your contribution, @avsm for your comments, and @diml for your guidance!

avsm · 2017-05-22T12:59:48Z

And thank you for the review and merge @dsheets!

rgrinberg force-pushed the jbuilder branch from 311b4e9 to 233ae8c Compare April 10, 2017 04:30

rgrinberg requested a review from avsm April 10, 2017 04:44

rgrinberg force-pushed the jbuilder branch from 233ae8c to a60a8c6 Compare May 1, 2017 17:16

rgrinberg requested a review from dsheets May 2, 2017 16:32

avsm approved these changes May 10, 2017

View reviewed changes

dsheets reviewed May 10, 2017

View reviewed changes

dsheets mentioned this pull request May 12, 2017

github: restrict deps to < 3.0 for findlib package name changes ocaml/opam-repository#9156

Merged

dsheets reviewed May 12, 2017

View reviewed changes

dsheets force-pushed the jbuilder branch from fa748d7 to 3d063e2 Compare May 19, 2017 14:26

samoht mentioned this pull request May 19, 2017

Switch to jbuilder, prepare to release 1.1.0 mirage/mirage-net#7

Closed

dsheets force-pushed the jbuilder branch from 62f9ba8 to be0653d Compare May 22, 2017 09:14

rgrinberg and others added 7 commits May 22, 2017 11:26

Port to jbuilder

b8acb40

META compatibility

bdf02a8

opam: update version

864d27a

Fix Travis and Appveyor

3c7b579

Add fatal warnings back into the build

b27213f

Enable more compiler errors and fix up some code in lib/

615596a

Enable more compiler errors in non-lib code and fixup errors

0131485

dsheets force-pushed the jbuilder branch from 281d501 to 0131485 Compare May 22, 2017 10:27

dsheets merged commit 36a37e8 into mirage:master May 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port to jbuilder #192

Port to jbuilder #192

rgrinberg commented Mar 24, 2017

dsheets commented Mar 24, 2017

rgrinberg commented Mar 24, 2017 via email

avsm commented Mar 31, 2017

rgrinberg commented Mar 31, 2017

avsm commented Mar 31, 2017

rgrinberg commented May 2, 2017

avsm commented May 2, 2017

rgrinberg commented May 2, 2017

dsheets commented May 2, 2017

rgrinberg commented May 2, 2017 •

edited

Loading

rgrinberg commented May 7, 2017

dsheets commented May 8, 2017

rgrinberg commented May 9, 2017

dsheets commented May 10, 2017

avsm left a comment

avsm May 10, 2017

rgrinberg May 10, 2017

avsm May 10, 2017

avsm May 10, 2017

ghost commented May 10, 2017

dsheets May 10, 2017

dsheets commented May 10, 2017

rgrinberg commented May 10, 2017

rgrinberg commented May 11, 2017

dsheets commented May 12, 2017

dsheets May 12, 2017

avsm May 12, 2017

avsm May 12, 2017

dsheets May 12, 2017

dsheets commented May 21, 2017

rgrinberg commented May 21, 2017

ghost commented May 22, 2017

dsheets commented May 22, 2017

avsm commented May 22, 2017

Port to jbuilder #192

Port to jbuilder #192

Conversation

rgrinberg commented Mar 24, 2017

dsheets commented Mar 24, 2017

rgrinberg commented Mar 24, 2017 via email

avsm commented Mar 31, 2017

rgrinberg commented Mar 31, 2017

avsm commented Mar 31, 2017

rgrinberg commented May 2, 2017

avsm commented May 2, 2017

rgrinberg commented May 2, 2017

dsheets commented May 2, 2017

rgrinberg commented May 2, 2017 • edited Loading

rgrinberg commented May 7, 2017

dsheets commented May 8, 2017

rgrinberg commented May 9, 2017

dsheets commented May 10, 2017

avsm 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

ghost commented May 10, 2017

Choose a reason for hiding this comment

dsheets commented May 10, 2017

rgrinberg commented May 10, 2017

rgrinberg commented May 11, 2017

dsheets commented May 12, 2017

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

dsheets commented May 21, 2017

rgrinberg commented May 21, 2017

ghost commented May 22, 2017

dsheets commented May 22, 2017

avsm commented May 22, 2017

rgrinberg commented May 2, 2017 •

edited

Loading