Update config and lib path settings

[Yi-Zhou-01]

• Set different paths for OPAM complied and source complied Links
• Removed LINKS_CONFIG and LINKS_LIB environment variables

[jamescheney]

Is this intended to address #1132?

[Yi-Zhou-01]

Is this intended to address #1132?

Yes.

[jamescheney]

OK, it's helpful to link the issue somehow (then Github knows to automatically close it once the PR is merged).

[jamescheney]

(I've done this now)

[Yi-Zhou-01]

Oh, thanks!

[dhil]

It looks like the unit testing infrastructure for lenses rely on the implicit resolution of the default configuration file. The right thing to do here would be to pass the name of the config file explicitly to each test. Perhaps we can do that in another PR -- for now I suggest you reinstate the automatic location and loading of the default configuration file, I think that will make this PR pass CI.

[dhil]

@Yi-Zhou-01 we should still fix the loading/selection of the default config in settings.ml. I think you should change the definition of config on line 1130 to something like

let config =
  (* Path to the default config file. *)
  let default_config_file =
    (* Use the precomputed path for the default config, if any. *)
    match Linkspath.config with
    | None ->
      (* If LINKS_CONFIG is defined then use that as the default config. *)
      Utility.getenv "LINKS_CONFIG"
    | Some path ->
        Some (Filename.concat path "config")
  in
  (* Load default is the action attached to the [config] setting. If
     [config_loaded] is false it will try to load whatever argument it
     is given (which will always be the default config). *)
  let load_default = function
    | None -> ()
    | Some file ->
       if not !config_loaded
       then (load_config `System file; config_loaded := true)
  in
  Settings.(option ~default:default_config_file "config"
            |> synopsis "Initialises Links according to the given configuration file"
            |> privilege `System
            |> hint "<file>"
            |> to_string from_string_option
            |> action load_default
            |> convert Utility.some
            |> CLI.(add (long "config")))

This gets rid of most of the magic. The only remaining magic is the variable LINKS_CONFIG which is used by the testing framework. We can get rid of that later.

[dhil]

Before we merge this, it would be good to check with @jamescheney whether you agree with this approach. In a nutshell this patch eliminates most of the "path guessing" by precomputing the paths to examples, stdlib, prelude.links, default config (well, we still a have bit of magic here due to the testing infrastructure relying on it), etc. If Links is installed by opam install, then it computes the paths relative to the current OPAM switch installation directory, if it is built by make (or dune by hand) then it computes paths relative to the source tree. This makes the resolution of paths static (with the exception of the default config) during run time, and hence, there ought to be no surprises w.r.t. which paths/files are being loaded by Links.

[jamescheney]

I'll try to have a closer look tomorrow if I can. From what I can infer from the comments/discussion, this certainly seems like an improvement even if there are still things that would be nice to fix like avoiding the config file magic that testing relies on.

[Yi-Zhou-01]

Oh right, this makes sense.

[dhil]

One thing that just struck me: where is the procedure that sets the LINKS_BUILT_BY_OPAM have you forgotten to include that script?

[Yi-Zhou-01]

One thing that just struck me: where is the procedure that sets the LINKS_BUILT_BY_OPAM have you forgotten to include that script?

Oh right, sorry. Just added it.

[dhil]

Installation of Links via OPAM fails with this patch.

$ opam install ./links.opam
links is now pinned to git+file:///home/dhil/projects/links/my-links#gen-opam (version 0.9.7)
The following actions will be performed:
  ∗ install links 0.9.7*

<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
⬇ retrieved links.0.9.7  (git+file:///home/dhil/projects/links/my-links#gen-opam)
[ERROR] The compilation of links.0.9.7 failed at "LINKS_BUILT_BY_OPAM=1 dune build -p links -j 15".

#=== ERROR while compiling links.0.9.7 ========================================#
# context     2.1.1 | linux/x86_64 | ocaml-base-compiler.4.14.0 | pinned(git+file:///home/dhil/projects/links/my-links#gen-opam#8bfa1236)
# path        ~/.opam/4.14.0/.opam-switch/build/links.0.9.7
# command     ~/.opam/opam-init/hooks/sandbox.sh build LINKS_BUILT_BY_OPAM=1 dune build -p links -j 15
# exit-code   1
# env-file    ~/.opam/log/links-1787152-208c48.env
# output-file ~/.opam/log/links-1787152-208c48.out
### output ###
# bwrap: execvp LINKS_BUILT_BY_OPAM=1: No such file or directory

<><> Error report <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
┌─ The following actions failed
│ λ build links 0.9.7
└─ 
╶─ No changes have been performed

I am not sure how to modify the environment via an OPAM package file. I think the best course of action might be to lift the build rules into the Makefile, e.g.

.PHONY: opam-build
opam-build-links.opam: links.opam
    dune exec preinstall/preinstall.exe -- -libdir $(eval opam var lib)
    $(eval LINKS_BUILT_BY_OPAM=1)
    dune build -p links

[dhil]

There is still something slightly wrong here. The generated paths are not correct when installed via opam install ./links.opam

links> @settings;
  jslibdir                         /home/dhil/.opam/4.14.0/.opam-switch/build/links.0.9.7/lib/js
  ...
  prelude                          /home/dhil/.opam/4.14.0/.opam-switch/build/links.0.9.7/prelude.links
  ...
  stdlib_path                      /home/dhil/.opam/4.14.0/.opam-switch/build/links.0.9.7/lib/stdlib

The paths are linking to the source tree copied by OPAM, it is suggesting to me that the environment variable LINKS_BUILT_BY_OPAM is still not being set correctly, causing gen_linkspath.exe to generate paths relative to the copied source tree.

[dhil]

Maybe relying on an environment variable is too fragile anyways. Maybe we ought to separate the generation of gen_linkspath into its own build rule in the links.opam file and Makefile. Though, a potential consequence of doing so would be that one cannot readily type dune build to build Links from source, one would have to build it either via make ... or opam install ......

That said, I'd still like to understand how to modify the environment that the dune runs in when compiling Links.

[dhil]

I managed to get it to do the right thing by tweaking the opam-build-links.opam rule as follows:

.PHONY: opam-build-links.opam
opam-build-links.opam: links.opam
    $(shell LINKS_BUILT_BY_OPAM=1 dune build -p links)

Now the paths look right to me:

  jslibdir                         /home/dhil/.opam/4.14.0/lib/links/js
...
  prelude                          /home/dhil/.opam/4.14.0/lib/links/prelude.links
...
  stdlib_path                      /home/dhil/.opam/4.14.0/lib/links/stdlib

[dhil]

Thanks! I will merge this shortly and push a few tweaks subsequently.