Port to jbuilder

[rgrinberg]

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]

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]

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 notifications@github.com 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 https://github.com/mirage/ocaml-github/pull/192#issuecomment-288979791, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIe-7F0cQXEsZZquWcbXwYt5BCaEMZ5ks5ro5NygaJpZM4MnqU3 .

[avsm]

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

[rgrinberg]

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

[avsm]

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

[rgrinberg]

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

[avsm]

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

[rgrinberg]

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

[dsheets]

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]

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: https://github.com/janestreet/jbuilder/issues/9

[rgrinberg]

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

[dsheets]

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]

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: https://github.com/janestreet/jbuilder/issues/9#issuecomment-283031113

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]

@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!

[ghost]

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]

The explicit copy rule seems fine to me. As far as I can tell from https://github.com/janestreet/jbuilder/issues/9#issuecomment-283031113, 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]

@dsheets the explicit copying rule is added

[rgrinberg]

All that remains for this PR is travis

[dsheets]

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]

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]

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]

-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]

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

[avsm]

And thank you for the review and merge @dsheets!