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

Add support for OCAMLTOP_INCLUDE_PATH environment variable #1841

Merged
merged 11 commits into from Oct 10, 2018

Conversation

Projects
None yet
5 participants
@nojb
Copy link
Contributor

commented Jun 15, 2018

See MPR#7808.

We add the possibility of passing extra include directories to the ocaml toplevel by using a specific environment variable OCAMLTOP_INCLUDE_PATH. The effect is the same as if they had been passed using -I options.

@dbuenzli
Copy link
Contributor

left a comment

Wonderful ! Thanks you very much. I do however have concerns about the lookup order you implemented. See my coments.

\begin{options}

\item["OCAMLTOP_INCLUDE_PATH"] Additional directories to search for
compiled object code files (".cmo" and ".cma").

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Jun 15, 2018

Contributor

And cmi files aswell... I think it would be good to indicate the order here. Basically this works like PATH or C_INCLUDE_PATH it searches from left-to-right after the -I from the cli have been searched.

This comment has been minimized.

Copy link
@nojb

nojb Jun 15, 2018

Author Contributor

Good idea, will amend the doc as suggested.

| exception Not_found -> []
| s -> Misc.split_path_contents s
in
Clflags.include_dirs := List.rev_append extra_paths !Clflags.include_dirs

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Jun 15, 2018

Contributor

If !Clflags.include_dirs holds the dirs as found on the cli which according to the manual are used from left to right then I think the right order here should be List.append !Clflags.include_dirs extra_path so that OCAMLTOP_INCLUDE_PATH is only used as a last resort from left to right.

This comment has been minimized.

Copy link
@nojb

nojb Jun 15, 2018

Author Contributor

The variable Clflags.include_dirs stores the directories in inverse order (each successive -I adds its argument to the front of the list) and gets reversed before being used (see https://github.com/ocaml/ocaml/blob/trunk/driver/compmisc.ml#L38), so I think this is OK.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Jun 15, 2018

Contributor

Yes agreed.

| exception Not_found -> []
| s -> Misc.split_path_contents s
in
Clflags.include_dirs := List.rev_append extra_paths !Clflags.include_dirs

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Jun 15, 2018

Contributor

See above.

@@ -126,6 +126,10 @@ The following command-line options are recognized by the "ocaml" command.
\begin{unix}
The following environment variables are also consulted:
\begin{options}
\item["OCAMLTOP_INCLUDE_PATH"] Additional directories to search for
compiled object code files (".cmi", ".cmo" and ".cma"). The directory list
is searched in left-to-right order.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Jun 15, 2018

Contributor

I would still add something like "after the include directories specified on the command line via -I have been searched." to make that cristal clear.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

  • How is this different from passing -I options through OCAMLPARAMS? Just a more convenient syntax?

  • Why is this only for the toplevel?

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

@alainfrisch answers to these questions in the MPR. Besides OCAMLPARAMS is not something that is supposed to be used by end users.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

@alainfrisch answers to these questions in the MPR.

The rationale is not explicit in the MPR ("(I can provide full details if one needs to be convinced).").

Why is this only for the toplevel?

I don't need the full details, but currently, the toplevel and compiler share the same external interface for setting the include path; is there a good reason to break it? Assuming the proposal is useful for the toplevel, isn't it harmless for the compiler as well?

Besides OCAMLPARAMS is not something that is supposed to be used by end users.

I don't understand enough of the use case for the proposal, but wouldn't OCAMLTOP_INCLUDE_PATH be set by some tools (opam?) as well?

Anyway, since the feature is already available, and if it turns out to be useful enough to deserve some additions: what about declaring that OCAMLPARAMS is supposed to be used by end users?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

Anyway, since the feature is already available, and if it turns out to be useful enough to deserve some additions: what about declaring that OCAMLPARAMS is supposed to be used by end users?

One downside of the current design of OCAMLPARAMS is that it is not easy to extend: if, say, opam sets it, it may be overwriting a value set by the user.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

I thought the syntax of OCAMLPARAMS was designed to make it easy to append to it (on either side), but perhaps I'm wrong.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

Also, the problem with the argument is that we might end up with N environment variables achieving the same effect, in order to let each external tool set its own variable without risking interaction with other tools.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

he rationale is not explicit in the MPR

The full details are no explicit... As I said there the rationale is that it's difficult for opam system and non-system compiler installs to make the #use idiom work (which is needed for example to bootstrap ocamlfind) in both ocaml scripts and toplevel (ocaml tool) invocations.

The reason is that on a bare ocaml execution #use only looks in ocamlc -where and it is a problem when the path returned here lives outside of your opam switch since you may not have the right to install things there.

Assuming the proposal is useful for the toplevel, isn't it harmless for the compiler as well?

The compilers don't need to #use. I think that one can reasonably argue that what you do with invocations of ocaml as an interactive user is quite different from your ocaml{c,opt} invocations which are used by build systems. You may e.g. automatically load toplevel enhancing libraries or load pet libraries via your .ocamlinit. So a dedicated env var seems warranted to me.

(That doesn't mean that we could also add an OCAML_INCLUDE_PATH that would affect the other or all the tools but that's not the purpose of this PR)

I don't understand enough of the use case for the proposal, but wouldn't OCAMLTOP_INCLUDE_PATH be set by some tools (opam?) as well?

Yes it would be setup or extended by opam switches.

what about declaring that OCAMLPARAMS is supposed to be used by end users?

OCAMLPARAM is hack to add arbitrary arguments to the compiler toolchain. I don't think this is the right device for the problem at hand. I see OCAMLPARAM purely as a tool to e.g. force build system to create .cmt files or debug sidecar file in dedicated opam switches for when build systems are not cooperative.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

Ok, fair enough. I don't fully understand the need, but if enough people are convinced and nobody else is concerned by the proliferation of similar but slightly different environment variables, I won't argue against the PR.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

Ok I still prefer when @alainfrisch is convinced. So if that can help I'll tell the whole long and boring story (@samoht and @AltGr apologies for the fiction). Even if he doesn't read it it will document the crap that we have now in the opam-repository which @Samoth and myself are largely guilty of by not doing this PR earlier.

Let's start with the beginning (and root of the problem ;-)). The OCaml system essentially leaves the business of managing and loading libraries to an external mechanism -- sure there's +dir but it's a bit limited. For many years now this mechanism has been provided by the ocamlfind project. In order to bootstrap the support it provides to the ocaml binary, ocamlfind installs a file called topfind in ocamlc -where.

This means that ocaml scripts (#!/bin/env ocaml) and interactive toplevel sessions (ocaml) can bootstrap the ocamlfind support by simply issuing a #use "topfind" because ocamlc -where's directory is included by default.

This is all nice and clean: it makes you scripts relocatable, and you can add the directive to your ~/.ocamlinit file regardless of where your OCaml system is actually installed.

Now one day, @samoht and a few other people are bored and design opam on a napkin (or in his garage I don't remember). opam allows you to manage, under a given $PREFIX oddly called a switch, multiple OCaml compiler install and their packages, including ocamlfind itself.

@samoht being resource minded, he allows for a switch to have the OCaml installation live outside of the switch for it to be provided by your regular operating system; a so called system switch. For many reasons it is however better not to let ocamfind be installed by your system, if only because this is actually an ocaml package, the exact thing opam pretends to be the steward of.

So when you install ocamlfind in a switch where should the topfind file go ? In a non-system switch your compiler is installed in $PREFIX so this is obvious, it can go in ocamlc -where. However in a system switch, it can't go there, ocamlc -where is owned by your os package system and likely privileged.

So @samoht designs this solution: we are going to install these top init files in a dedicated directory in the $PREFIX (he chooses $PREFIX/lib/toplevel) and we are simply going to make ocaml include that directory by default.

Since he finds no way to runtime configure the ocaml binary with that path he invents the OCAML_TOPLEVEL_PATH environment variable to hold it. He plans to upstream it but he also needs something that works immediately. To that effect, on opam init he performs the following surgery on your ~/.ocamlinit file:

(* Added by OPAM. *)
let () =
  try Topdirs.dir_directory (Sys.getenv "OCAML_TOPLEVEL_PATH")
  with Not_found -> ()
;;

and adds the variable to your environment with the right directory on $(opam config env) invocation. He's so happy (and busy) and it works so well for everyone when they invoke ocaml that he forgets about upstreaming the support for the runtime configuration of ocaml via OCAML_TOPLEVEL_PATH.

A few years later @dbuenzli is a bit fed up with shell scripting and the distribution of his packages and writes topkg. In topkg you write a simple OCaml script that loads a library (topkg) via ocamlfind. In other words it starts with the following runes:

#!/usr/bin/env ocaml
#use "topfind"
#require "topkg"

The system is kind of a hack but seems to work reasonably well. However soon enough people start complaining that topkg doesn't work in system switches. The problem of course is that .ocamlinit is rightly not read by ocaml scripts, and hence the above snippet to include the contents of OCAML_TOPLEVEL_PATH is not executed, scripts are totally unaware of the variable and #use "topfind" fails miserably.

Since @dbuenzli is a bit impatient with the situation and kind of a dirty man he seeks for the minimal solution working immediately. Blinded by his anger he introduces a terrible solution: the wrapper.

He tweaks the ocamlfind package (not the ocaml package because in opam v1.2.2 compilers were not packages) to install as $PREFIX/bin/ocaml in system switches (the path is available, ocaml itself is in the system bin) the following script:

#!/bin/sh

BINDIR=$(dirname "$(command -v ocamlc)")
"$BINDIR/ocaml" -I "$OCAML_TOPLEVEL_PATH" "$@"

Problem solved and advertised as such. @dbuenzli is so happy and it works so well for everyone that he cowardly shies away from solving the actual problem correctly.

Now @AltGr hears about this being solved at the same time he's busy designing opam v2. Being a tasteful person and somehow falsy trusting me and @samoht not to be pigs he rightly doubts the beauty of the .ocamlinit surgery and drops it for v2 --- which is also entirely consistent with the general direction of opam becoming more general.

A few days ago @dbuenzli introduces new software that also needs to boostrap via a file installed in $PREFIX/lib/toplevel. He's quite happy with the result and tests loading this file in both system and non-system switches and everything works correctly.

However he suddenly learns that .ocamlinit surgery is no longer performed in opam v2 init, so he deletes the opam v1 surgery from his .ocamlinit file and suddenly loading in non-system switches no longer work.

He can still use "topfind" though, a bit puzzled he investigate and realizes that in non-system switches, ocamlfind installs topfind in both ocamlc -where and in $PREFIX/lib/toplevel. However the latter is no longer looked up without the .ocamlinit surgery (and would never be looked up by non-system switch scripts), it is only looked up in system switches by the wrapper. So he realizes that he needs to install his init files both in $PREFIX/lib/toplevel (for system switches) and in $PREFIX/lib/ocaml (for non system switches) which is what his software now does.

At that point deep guilt start to stream in as he realizes that the right way of getting rid of all of these hacks is to go back to @samoht's forgotten desire and provide runtime configuration of ocaml upstream. Full of shame and way too late he finally opens MPR7808 and hopes for forgiveness of his sins.

He is personnaly quite sure that this configuration should not happen via OCAMLPARAM. Whether this should rather occur through a variable OCAML_INCLUDE_PATH that applies to all of the ocaml
tools can be discussed but he's inclined to think that there are things one wants to include that regard only the toplevel and there's no need to pollute the environment of all compilations with those.

\item["OCAMLTOP_INCLUDE_PATH"] Additional directories to search for compiled
object code files (".cmi", ".cmo" and ".cma"). The specified directories are
considered from left to right, after the include directories specified on the
command line via "-I" have been searched.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Jun 18, 2018

Contributor

@nojb a "Since 4.08" would also be nice here.

This comment has been minimized.

Copy link
@nojb

nojb Jun 18, 2018

Author Contributor

Done.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2018

Thanks @dbuenzli for this nice summary(?). I actually read it, and I'm convinced.

@damiendoligez
Copy link
Member

left a comment

Looks good except for the slightly incompatible change.


let split str sep =
let rec split_rec pos =
if pos >= String.length str then [] else begin

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jun 19, 2018

Member

Note that you replaced this function with String.split_on_char, which never returns an empty list. This will make a difference when the environment variable is present but empty. In that case, it amounts to adding the current directory to the search path.

It may be OK to do that, but it's a subtle change that should be carefully considered.

This comment has been minimized.

Copy link
@nojb

nojb Jun 19, 2018

Author Contributor

Good catch!

Even if it is a desirable change, better to consider that in a separate PR. I will amend this one to restore the old behaviour.

This comment has been minimized.

Copy link
@nojb

nojb Jun 19, 2018

Author Contributor

This is now fixed.

| Some c, _ -> c
| None, ("Unix" | "Cygwin") -> ':'
| None, "Win32" -> ';'
| None, _ -> assert false

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Jun 19, 2018

Contributor

Sorry for one more nit but Sys.os_type may end up being different from what you enumerated here (e.g. see here in ocaml-freestanding). I'm not sure Misc.ml could be used in such a context (it's only a runtime system being compiled there) but I think there's nothing bad about being robust to this opportunity and defaulting to e.g. "Unix" rather than fail with an assertion here.

This comment has been minimized.

Copy link
@nojb

nojb Jun 19, 2018

Author Contributor

I don't think it is possible to build the compiler in a system where os_type is not one of the above, but the "default-is-unix" logic is present in other parts of the compiler, so I went ahead and amended as suggested.

@damiendoligez damiendoligez self-assigned this Jun 20, 2018

@nojb nojb force-pushed the nojb:toplevel_include_path_env_var branch from 506535b to df7bf84 Sep 4, 2018

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

Ping.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2018

Is there anything that prevents this PR to be merged ? I think it would be good to start having this in 4.08 if we want to be able to eventually get rid of the opam-repository hacks.

@nojb There's a conflict in the Changes file.

@gasche

gasche approved these changes Oct 10, 2018

Copy link
Member

left a comment

I will make the guess that @damiendoligez has just been too busy with other things to re-check this PR, but that his previous review (whose minor comment has been addressed) is an Approval -- and @dbuenzli's review also gives me confidence. I haven't reviewed the PR myself.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2018

OK, I'll amend Changes and merge.

@nojb nojb force-pushed the nojb:toplevel_include_path_env_var branch from df7bf84 to 795a9a2 Oct 10, 2018

@nojb nojb merged commit 8e80195 into ocaml:trunk Oct 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2018

Thanks!

@nojb nojb deleted the nojb:toplevel_include_path_env_var branch Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.