Fix jbuilder support without opam #179

bobot · 2017-07-05T13:51:17Z

Doesn't change that opam-installer must be available.

Should fix #174 by:

considering that an opam failing is an opam not present
use the option libdir of opam-installer for the case where libraries should not be installed in $PREFIX/lib

libdir is:

the argument given on the command line of jbuilder install
ocamlfind printconf destdir
opam config lib
OCaml standard library path.

bobot · 2017-07-06T13:05:12Z

Could we add 4.05 to the CI matrix?

dra27 · 2017-07-06T13:07:41Z

It's on my todo list for later!

dra27 · 2017-07-06T15:31:40Z

https://github.com/dra27/jbuilder/tree/ci-4.05 - but in order to test with --dev, I presently have to do something which Jeremie has already rejected (altering the opam file). I'll see if I can find time to replace the entire script (there's no need to use opam at all for the CI)

ghost · 2017-07-13T16:18:25Z

bin/main.ml

+let get_libdir context ~from_command_line =
+  match from_command_line with
+  | Some p -> Future.return (Path.of_string p)
+  | None -> Context.install_ocaml_libdir context


I think the default for --libdir should be $prefix/lib. This is what opam-installer does. Otherwise, if you call jbuilder install --prefix ..., it might install library files at a completely different location

When is Context.install_ocaml_libdir used? I propose:

use --libdir if present

use --prefix if present with $prefix/lib

use Context.install_ocaml_libdir

Seems good to me

Fixed, rebased and squashed

ghost · 2017-07-17T13:13:42Z

src/context.ml

+    | Some s -> Path.absolute s
+    | None ->
+      (* If neither opam neither ocamlfind are present use stdlib dir *)
+      t.stdlib_dir


I think it's a bit surprising that for the search path, jbuilder falls back to $(which ocamlc)/../../lib and for installation it falls back to $(ocamlc -where). I don't claim that I made the right choice for the search path, happy to change it if the current default is wrong, but I think the two should be consistent

Previously people used a lot -I +lib, and it is the place that debian use. As a side note in many tools the default installation prefix is /usr/local and not /usr .

Ok. opam-installer uses /usr/local as default

opam-installer uses /usr/local as default

Do you want jbuilder to use this default?

Not really, the current default is where OCaml is installed which seems fine to me, especially if we install the library files in the stdlib directory

- change default findlib directory (ocamlc -where)

ghost · 2017-07-18T14:55:20Z

src/context.ml

+    else
+      return []
+  in
+  findlib_path >>= fun findlib_path ->


Why did you move this bit? It is now run after ocamlc -config instead of in parallel with it

Because I need the value of stdlib_dir.

ghost · 2017-07-18T14:59:44Z

doc/usage.rst

@@ -248,17 +248,27 @@ to be able to use ``jbuilder install``.
 Destination
 -----------

-The place where the build artifacts are copied, usually referred as
+The place where the build artifacts (except ocaml libraries) are copied, usually referred as


s/ocaml libraries/library files/, we usually copy other stuff as well, such as binaries (ppx, ...)

ghost · 2017-07-18T15:01:07Z

doc/usage.rst

-   ``/usr``
+#. otherwise, take the parent of the directory where ``ocamlc`` was found.
+
+The ocaml libraries build artifacts are copied, referred as


place word missing. Same remark about ocaml libraries

ghost · 2017-07-18T15:01:43Z

doc/usage.rst

+The ocaml libraries build artifacts are copied, referred as
+**libdir**, is determined as follow for a given build context:
+
+#. if an explicit ``--libdir <path>`` argument is passed, use this path


Missing point about --prefix <path>

ghost · 2017-07-18T15:05:19Z

src/context.ml

+    | Some s -> Path.absolute s
+    | None ->
+      (* If neither opam neither ocamlfind are present use stdlib dir *)
+      t.stdlib_dir


Ok. opam-installer uses /usr/local as default

ghost · 2017-07-19T15:17:37Z

BTW, why not simply call opam-installer with the right options? Unless you are installing for multiple workspaces simultaneously, in which case you need opam, jbuilder install just makes a simple call to opam-installer.

That would avoid adding more ad-hoc rules to jbuilder and relying even more on ocamlfind.

bobot · 2017-07-19T15:51:28Z

BTW, why not simply call opam-installer with the right options?

What are the right options? opam-installer doesn't use ocamlfind or ocamlc -where by itself.

--libdir=PATH
           OCaml lib dir. Relative to prefix or absolute. By default $prefix/lib ; sometimes setting
           this to $(ocamlc -where) is preferable.

By default opam-installer is not installing things at the right place. Perhaps I could propose to modify opam-installer instead of jbuilder but the second could already use informations from ocamlfind, the first doesn't.

ghost · 2017-07-19T15:57:01Z

I imagine that it would be:

$ opam-installer --libdir $(ocamlfind printconf destdir) foo.install

bobot · 2017-07-19T16:02:59Z

Oh I see perhaps better what you mean. You are proposing that jbuilder install be removed and people use directly opam-installer? But they will have to handle all the case themselves no?

ghost · 2017-07-19T16:21:11Z

I'm not proposing to remove it, I think it's fine in its current state. I'm just saying that if you want more control, you can simply use opam-installer directly and set the parameters that suit your configuration.

bobot · 2017-07-20T13:06:09Z

I'm just saying that if you want more control, you can simply use opam-installer directly and set the parameters that suit your configuration.

I was not clear with the problem I tried to fix in this MR. My problem is to install other people library that use jbuilder in a context without opam (example ocaml is installed using debian packages).

For example the findlib package of debian already has all the interesting configuration /etc/ocamlfind.conf:

destdir="/usr/local/lib/ocaml/4.02.3"
path="/usr/local/lib/ocaml/4.02.3:/usr/lib/ocaml:/usr/lib/ocaml/METAS"

Is the installation in such setting not something that library developers want out-of-the-box?

ghost · 2017-07-20T13:56:32Z

I suppose.

Many people want to get rid of ocamlfind, the idea being that once we have namespaces we don't need ocamlfind and especially findlib names anymore. So I'm a bit reluctant to rely more on ocamlfind.

For installation, given that we already require an external tool, it doesn't seem too bad to me to just use an external tool altogether. We could have a separate tool to handle installation for non-opam setups.

bobot · 2017-07-20T15:44:55Z

the idea being that once we have namespaces we don't need ocamlfind and especially findlib names anymore.

It is simpler when the distribution could indicate where to install things in an uniform way instead for each project to guess for each distribution where to install things. We still generally need to know where to install things, of course in the namespace proposal ocamlc -config could indicate it, but it is not yet the case (lpw25/namespaces#5).

So I'm a bit reluctant to rely more on ocamlfind.

This PR just use it if it is present, I think it doesn't rely on it.

ghost · 2017-07-21T09:33:15Z

Ok, I suppose this PR only uses ocamlfind if it's present in the PATH and in this case we can assume that it reflects the user setup. I'll read this one more time but I think I'm OK with it in the end.

ghost · 2017-07-25T16:13:11Z

Ok, I merged this by hand and did some minor changes. I changed the rules so that we use ocamlfind printconf destdir if ocamlfind is available but we don't have a completely different set of rules for library files. I find it simpler to document and remember

dra27 mentioned this pull request Jul 6, 2017

CI changes #181

Merged

ghost suggested changes Jul 13, 2017

View reviewed changes

ghost reviewed Jul 17, 2017

View reviewed changes

bobot added 2 commits July 18, 2017 15:03

Install by specifying libdir for opam-installer

721ea27

- change default findlib directory (ocamlc -where)

Consider that a failing opam is an absent opam

f524d93

bobot force-pushed the no_opam branch from 7f3c146 to f524d93 Compare July 18, 2017 13:04

ghost suggested changes Jul 18, 2017

View reviewed changes

Fix documentation for --libdir

48458ee

ghost closed this Jul 25, 2017

bobot mentioned this pull request Jul 27, 2017

jbuilder depends on opam #202

Closed

dra27 mentioned this pull request Dec 18, 2017

Install package without opam-installer in Windows #372

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix jbuilder support without opam #179

Fix jbuilder support without opam #179

bobot commented Jul 5, 2017 •

edited

Loading

bobot commented Jul 6, 2017

dra27 commented Jul 6, 2017

dra27 commented Jul 6, 2017

ghost Jul 13, 2017

bobot Jul 17, 2017

ghost Jul 17, 2017

bobot Jul 18, 2017

ghost Jul 17, 2017

bobot Jul 18, 2017 •

edited

Loading

ghost Jul 18, 2017

bobot Jul 18, 2017

ghost Jul 19, 2017

ghost Jul 18, 2017

bobot Jul 18, 2017

ghost Jul 18, 2017

ghost Jul 18, 2017

ghost Jul 18, 2017

ghost Jul 18, 2017

ghost Jul 18, 2017

ghost commented Jul 19, 2017

bobot commented Jul 19, 2017

ghost commented Jul 19, 2017

bobot commented Jul 19, 2017

ghost commented Jul 19, 2017

bobot commented Jul 20, 2017 •

edited

Loading

ghost commented Jul 20, 2017

bobot commented Jul 20, 2017

ghost commented Jul 21, 2017

ghost commented Jul 25, 2017

Fix jbuilder support without opam #179

Fix jbuilder support without opam #179

Conversation

bobot commented Jul 5, 2017 • edited Loading

bobot commented Jul 6, 2017

dra27 commented Jul 6, 2017

dra27 commented Jul 6, 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

Choose a reason for hiding this comment

bobot Jul 18, 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

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 Jul 19, 2017

bobot commented Jul 19, 2017

ghost commented Jul 19, 2017

bobot commented Jul 19, 2017

ghost commented Jul 19, 2017

bobot commented Jul 20, 2017 • edited Loading

ghost commented Jul 20, 2017

bobot commented Jul 20, 2017

ghost commented Jul 21, 2017

ghost commented Jul 25, 2017

bobot commented Jul 5, 2017 •

edited

Loading

bobot Jul 18, 2017 •

edited

Loading

bobot commented Jul 20, 2017 •

edited

Loading