Track original source location in lib_info/dune_package

[andreypopp]

This allows to communicate original source locations between installed (dune) libraries so we can construct .merlin files with S directives pointing right to them for installed libs. Thus allowing merlin to see original sources.

This is useful when lib is opam-pinned or esy-linked. I think we can set orig_src_dir to None for all other installed packages (based on profile?).

[andreypopp]

This is weird and maybe can be done easier but Path is confusing for me at the moment.

[ghost]

in_source (to_string src_dir) should be the identity, you can replace it by src_dir

[rgrinberg]

The idea is fine, but how do we control which sources to use in the .merlin file? If a package is installed via opam, then the original source dir will be ephemeral.

[andreypopp]

I thought maybe if profile is set to “release” (-p is passed) we don’t populate “orig_src_dir” in dune package metadata.

On Thu, 10 Jan 2019 at 19:45, Rudi Grinberg ***@***.***> wrote: The idea is fine, but how do we control which sources to use in the .merlin file? If a package is installed via opam, then the original source dir will be ephemeral. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1750 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB3glYuDiW0e-A-boEyUD1GUD3DyZJtks5vB24dgaJpZM4Z5bvw> .

-- Andrey Popp / [email protected]

[ghost]

What about controlling this behaviour with a command line switch/environment variable? In practice, we probably don't want to store these paths by default as it will hurt reproducibility, but esy could enable it via the environment variable.

[andreypopp]

What about controlling this behaviour with a command line switch/environment variable?
In practice, we probably don't want to store these paths by default as it will hurt reproducibility, but esy could enable it via the environment variable.

Won't such option be overly specific? Doesn't dune already have dev vs release build modes — we can generate such paths only in dev mode. I think that will be useful for opam-pinned packages as well (they can define build command optionally using dev profile if pinned is true in opam).

[rgrinberg]

I think that will be useful for opam-pinned packages as well (they can define build command optionally using dev profile if pinned is true in opam).

Given that you need to go through editing the opam file, I think that makes the feature not very useful. So it makes it even more of an option that only esy would configure.

[andreypopp]

I proceeded with not emitting orig_src_dir in non dev profile (related: should we make profile type kind = Dev | Release and profile = Profile of kind | Custom of kind * name?).

I had to add a sanitisation step for blackbox test which replaces test's root path with $TESTCASE_ROOT token against which we could match in test expectations.

[andreypopp]

This is a new test case but diff doesn't make it obvious.

[andreypopp]

I'm not sure it's ok this breaks on the next line here.

[rgrinberg]

It just occurred to me that it's possible to implement this feature without touching dune-package at all.

The only reason why dune doesn't know that some external packages are actually local is because they get filtered out by -p. Well it shouldn't be hard to change dune to remember which library definitions it has skipped.

[andreypopp]

The only reason why dune doesn't know that some external packages are actually local is because they get filtered out by -p. Well it shouldn't be hard to change dune to remember which library definitions it has skipped.

What does it mean "actually local" — being a part of the same workspace? Then this isn't the case always in esy: it allows you to link packages from outside:

{
  "resolutions": {
    "@opam/dune": "link-dev:../path/to/dune/checkout"
  }
}

[rgrinberg]

I see. So my suggestion wouldn't work after all. Would it be possible for esy to just pass the linked packages to dune somehow? Changing dune-package is a touchy issue for us, so I'd prefer that we refrained from doing so unless we really had no other way.

[andreypopp]

Changing dune-package is a touchy issue for us, so I'd prefer that we refrained from doing so unless we really had no other way.

Instead of changing dune-package we could place dune-package.dev with dev
related metadata (orig_src_path only at first but there can be potentially
more in the future).

OCAMLPATH (findlib) config is already used to communicate about installed
OCaml artifacts in the esy project (or system/switch wide in case of opam). I
think adding additional package dev metadata there makes sense. But I'm open for suggestions.

[ghost]

BTW, when a package is pinned via opam, the build still happens in a temporary directory, so the value that would be stored here wouldn't be useful.

It seems to me that using an environment variable is the best option here. It's true that this is a very specific setting, but on the other hand most of the time storing the original source directory doesn't make sense. Even if we store it in a separate .dev file, it still doesn't feel right to install such files with temporary paths.

[andreypopp]

It seems to me that using an environment variable is the best option here. It's
true that this is a very specific setting, but on the other hand most of the
time storing the original source directory doesn't make sense. Even if we store
it in a separate .dev file, it still doesn't feel right to install such files
with temporary paths.

Ok, makes sense.

What would be the name/value of such environment variable? Something like (using
s-expressions):

DUNE_LIB_SOURCES='(lwt "/path/to/lwt" fmt "/path/to/fmt")'

[andreypopp]

Hm... on a second thought I'm not sure how esy would be able to construct such
environment variable as it doesn't know about ocamlfind libraies which packages
are shipping.

[andreypopp]

Ok, sorry for the noise, I think now you meant controlling that behaviour as passing a boolean flag via environment. I agree, that could work. What would be the name of such environment variable? DUNE_STORE_ORIG_SOURCE="true" or something else?

[ghost]

Yep, that's what I meant. I suggest DUNE_STORE_ORIG_SOURCE_DIR to be more precise.

[andreypopp]

@diml thanks! Will update this PR with that.

[andreypopp]

This is ready for review.

[rgrinberg]

It's a bit annoying to introduce another environment variable for this. Why can't this setting live in the workspace file?

[andreypopp]

@rgrinberg the only reason I can think of is that users will need to configure dune-workspace if they want to merlin to work correctly in multi package environments. I'd prefer esy to be able to do this automatically somehow but generating dune-workspace isn't very convenient.

[rgrinberg]

I'd prefer esy to be able to do this automatically somehow but generating dune-workspace isn't very convenient.

It will not be necessary [1]

[andreypopp]

@rgrinberg I guess before that we could manually set dune-workspace file. Ok, is the stanza (store_orig_source_dir) ok for dune-workspace then?

[ghost]

Hold on, one big advantage of using specific environment variables is that it makes composition easy. For instance, I believe that it is now generally accepted that OCAMLPARAM is a pain to use and that specific variables would have been better.

@rgrinberg why is it annoying to introduce new environment variables?

[jordwalke]

I have to chime in here to say that some kind of env var does seem better to me (than the alternative) because it's something package authors can't know to configure - only the package manager knows what's linked for the purposes of dev vs. release. I don't have many opinions on exactly what the env var looks like, but this is the one remaining feature blocking a very great monorepo workflow for esy and dune which allows you to "absorb" external packages into your local monorepo temporarily while developing.

[andreypopp]

Thanks for the feedback @diml, I believe I've addressed all points.

[rgrinberg]

The reason why I wanted to started with a workspace option is that it avoids having to remember which vars can be overridden via the environment.

On the other hand, there's nothing wrong with an individual env var even if there a workspace the option.

[ghost]

Yh that's true. In this case, I expect that this variable will be set by another tool most of the time.

[ghost]

@andreypopp thanks, just one last point I missed during the first review, otherwise it looks good.

[andreypopp]

Thanks for another round of feedback @diml, I believe I've address these points too. Also rebased on top of the current master to resolve conflicts.

[ghost]

Thanks, LGTM

[andreypopp]

Thanks!