@gasche, as you requested, this is the first part from #12085. @dra27, @dbuenzli, @shindere, would you also like to take a look?

Note that there is no longer a need for two versions of Location.absolute_path as it now will rewrite both relative and absolute paths.

I have a proposed revision of that specification. I have capture the original specification on github, and then created a pull requests with my proposed changes. See

I'm afraid I don't have the time to look into all this for the forseeable future.

But I doubt you need to add more concepts to that specification. If the tool setting up the prefixes does it in a smart way so that the map is bijective (i.e. distinct source prefixes map on distinct target prefixes) I don't think there's any need for search lists. You should always be able to redirect each target prefix to the location you need at "deploy time".

But I doubt you need to add more concepts to that specification. If the tool setting up the prefixes does it in a smart way so that the map is bijective (i.e. distinct source prefixes map on distinct target prefixes) I don't think there's any need for search lists. You should always be able to redirect each target prefix to the location you need at "deploy time".

What you say is true, but a much more complex mapping is needed in that case. If the product involves n packages, then the map would need n separate entries, one for each package. If search lists are used, then the mapping can be done at a higher level and usually only 3 entries are needed. That would mean it would be possible to setup up the mapping manually, whereas without search lists it would certainly require tools to set it up.

And the generalization to allow search lists is very small (even a small part of this PR), so I think it is worthwhile in order to gain simplicity.

You could think of this as being analogous to the PATH environment variable, which is a search list. Instead of having a PATH environment variable, you could just have a map that specifies where each executable you want is located. But that would be more complex than being able to use PATH. It would be like requiring you to look only in a single directory for any executable. It would work, because you could setup symbolic links from the directory to the real locations (and to some extent that is what /usr/bin is), but it would not be as convenient as having PATH. Usually a person's PATH will be much smaller than the number of execuables reachable from it.

What you say is true, but a much more complex mapping is needed in that case. If the product involves n packages, then the map would need n separate entries, one for each package. If search lists are used,

To make that a little bit more precise, I think it would involve 2 + k mappings, where k are the packages to be installed that are built by your project. Basically you'd have, in right to left order:

1. k mappings (/lib/$P, _build/…/lib/$P) for the k packages built and to be installed by your project (note: lots of projects only install one package).
2. One mapping (/lib, $(opam var lib)) for the installed packages used by your project (and compiled according to this convention).
3. One mapping (/private, _build/…/private/lib), for the artefacts that your project builds but keeps for itself.

(With _build path above refined according to the concrete build structure but that's the gist of the idea)

With that you can simply follow exactly the same lookup specified in BUILD_PATH_PREFIX_MAP specification. To map your reproducible paths to concrete paths.

That would mean it would be possible to setup up the mapping manually, whereas without search lists it would certainly require tools to set it up.

Personally I'd rather have a simpler mechanism than catering for this use case. Because:

1. I'm not keen on having to understand one more lookup specification when things go wrong.
2. I don't expect programmers to setup this manually.

Regarding 2., a significant trigger for programmers to use debuggers are the setup costs. Basically they use them if they are push button, otherwise they'll just reach for printf.

So it should be expected that a tool will set up the mapping for you. Namely the one which perfectly knows all this information: your build system.

To make that a little bit more precise, I think it would involve 2 + k mappings

Good point. Only one mapping is needed to cover the whole opam library. As you point out, k will usually be small. You almost have me convinced. Search lists could reduce the k to 1.

Also, a possible scenario that search lists would handle better would be a situation where there was more than one opam-like repository, e.g. experimental vs. stable. Then at deploy-time, the /lib could map to a search list with /experimental/lib and /stable/lib. Without search lists that would be handled by an additional m mappings, one for each package from the experimental repository.

I should point out that the API with search lists is (almost) backward compatible with the old API. I say almost, because the function to encode a pair in the new API takes a list of targets rather than a single target. But if the idea of including search lists is accepted, we could have a backward-compatible API and an enhanced API if search-lists are desired.

I will leave it up to whomever has the authority to decide, whether to include the search-list feature. My preference is to include it. But I will abide by whatever decision is made. Would @gasche or one of the others maintainers please make a decision on this point?

Speaking of producers of mappings, the mappings Dune produces for the build phase are correct if the developer has their source layout isomorphic to the installation layout (so reproducible paths will look like /workspace_root/package/item). But if the developer does not use that layout, I don't believe that Dune makes any special effort to make the reproducible paths correct. That is something I am investigating.

And, of course, Dune will need to have some features to make the proper deploy-time map, and perhaps invoke the debugger. The emacs or other interfaces will also need to be updated. I will also be investigating that.

@gasche, I see that you are the author of the build_path_prefix_map package. Do you see any merit in adding the search-list capability?

I wrote the support code for the current BUILD_PATH_PREFIX_MAP specification indeed -- it was upstreamed into the compiler, and also published as a separate library in case other OCaml programs would want to implement it.

I am hesitant to follow your road of changing the specification to do more things. Initially the reproducible-builds people sold the specification as a done deal, soon to be supported by gcc/clang, but I am not sure that gcc/clang actually followed this approach in the end. In any case, I liked the idea of using the same approach as others and sharing a common specification. With your suggestion we would go down the route of adopting and extending the specification for the needs of (a small fragment of) the OCaml community only, and we would either have to gather support from a non-existent or at least unknown group of outside users, or be responsible for maintaining it moving forward. If we wanted to do our own thing independently of others, I am not sure that I would use this specification with its weird quoting rules; maybe something simpler could be used.

This is all said without looking at the details of your proposal, which does not sound unreasonable or badly designed. (The idea of quoting an additional special character is better than your previous approach of reusing :.)

Because I haven't invested the time yet to understand your work in details, I am left with the same question that I already asked previously: In a previous PR a suggestion was made to just "revert" an existing BUILD_PATH_PREFIX_MAP mapping, which in the general case may give a one-to-many relation. Is that not enough to support what you call "deployment phase"?
If artifact A is compiled with the mapping (path-a -> root), and artifact B is compiled with the mapping (path-b -> root), then the union of the mappings is (path-a -> root, path-b -> root), and asking ocamldebug to interpret this mapping "in reverse" gives the non-deterministic (or one-to-many) relation (root -> {path-a, path-b}). I don't know if all interesting one-to-many mappings can be expressed in this way (as the reverse of a standard one-to-one mapping) -- maybe you already answered this before, apologies.

Finally, @dbuenzli suggested using different "workspace root" names for different packages to avoid the issue of needing one-to-many mappings: maybe we want to use the build-time mapping path-a -> root(a), path-b -> root(b) (for example: '~/Prog/foo/_build" -> "workspace_root(foo)"), and the deploy-time mapping remains one-to-one, we just need to remap "workspace_root(foo)" to "~/.opam/lib/foo" and "workspace_root(bar)" to "~/.opam/lib/bar".

(Note: when I talk of a mapping foo -> bar, what I have in mind is that applying this mapping rewrites the foo prefix in a path into bar. This is worth pointing out because the BUILD_PATH_PREFIX_MAP format presents this, for reasons that are unknown to me, as the pair (bar,foo) in that order.)

OK. You all have convinced me that it is not worth the effort to introduce search lists and get them accepted into the community. I will revert that part of my change. It will be up to what the spec calls "producers", i.e. those that produce the BUILD_PATH_PREFIX_MAP, (both during the build phase and the deploy phase) to get it right. "Low-level tools", like the compiler and debugger, will just do the mapping requested. For Dune users, Dune will need to add the capability for the deploy phase.

I have removed the search list feature from the current push.

When using Dune, one thing that occurs to me I have lost is the ability (without great effort) to preferentially find the original source files rather than finding the mirror file in the _build directory. The abstract path to a source file will be something like

/workspace_root/apackage/adir/mod.ml

At build time, when I want to do debugging, we will have the original source:

srcdir/apackage/adir/mod.ml and its mirror in the build directory:

srcdir/_build/default/apackage/adir/mod.ml

But the abstract path to a .cma file will typically be:

/workspace_root/apackage/adir/adir.cma

and it will exist in the build directory at:

srcdir/_build/default/apackage/adir/adir.cma

If I preferentially map /workspace_root/apackage to srcdir/apackage, then
it would also map /workspace_root/apackage/adir/adir.cma to
srcdir/apackage/adir/adir.cma, which does not exist.

The workaround would be to have mappings at the individual artifact level,
but that will increase the size of the mapping considerably.

I suppose it is OK to get the location of the sources in the build directory, rather than the originals, though when using the emacs interface, whenever that happens, it asks whether I want to go to the original, which is rather annoying.

Although, I happened to think of a small tweak to the mapping functions that might add power without much complexity. What if we had a variation of Build_path_prefix_map.rewrite_opt that took a predicate? This variation would find the first rewrite that satisfied the predicate. If the predicate was that the rewritten file existed, then the above problem would be solved. First the mapping of the .cma file to the srcdir would be found, but rejected, so the next mapping would be tried.

Here is a draft implementation:

let pred_rewrite_opt prefix_map pred path =
  let is_prefix { target = _; source } =
      String.length source <= String.length path
      && String.equal source (String.sub path 0 (String.length source))
  in
  let subst path { source; target } =
    target ^ (String.sub path (String.length source)
                (String.length path - String.length source)) in
  match
    List.find (function
     | None -> false
     | Some pair -> (is_prefix pair) && pred (subst path pair))

      (* read key/value pairs from right to left, as the spec demands *)
      (List.rev prefix_map)
  with
  | exception Not_found -> None
  | None -> None
  | Some pair -> Some (subst path pair)

What would you think of a small enhancement like that?

The more I think about it, the more important I think it is to have the pred_rewrite_opt function mentioned above. During the "build phase', abstract paths are produced that are not expected to exist. But during the "deploy phase" we are producing paths that we expect to exist. Therefore I think it makes sense to be able to have a mode where mapping pairs that do not produce existing paths are skipped.

This has all of the advantages of the search-list strategy I had without any of the complexities. I think if the writers of the BUILD_PATH_PREFIX_MAP spec had been thinking about the deploy phase as well as the build phase they might have suggested something like this. I'm going to go ahead and add this one function (and a companion in Location), and do some testing with it. We could consider it a "quality of implementation" feature.

I'm also going to temporarily bring over my ocamltest and testsuite changes for the purpose of testing these changes. But the reviewers should ignore the ocamltest and testsuite changes. I will revert those when the review of this PR is complete so that they will not be included.

I am fine with pred_rewrite_opt. This is a form of lookup into the map that is different from the one defined in the specification, but it does not change the syntax or extend the map data structure in any way.

On the other hand:

• The implementation is very redundant with the rewrite_opt function already provided, I would want a single implementation of the lookup logic. (For example rewrite_opt can be an instance of pred_rewrite_opt with a trivial predicate.)
• The implementation calls subst twice on each path, which I find a bit distasteful. You could implement this using List.find_map instead.
• I'm not excited by the name pred_rewrite_opt, maybe find_rewrite?

A more vague question: rewrite_absolute_path_existing does not rewrite the input path at all if there are several possible rewrites, but none of them exists. I don't see a clearly better choice, but I wonder if doing this silently is the right behavior. Intuitively I would distinguish two cases:

1. There is no matching source prefix in the prefix map, so not doing any rewrite is clearly the right behavior.
2. There are matching prefixes in the prefix map, but none of them satisfy the predicate. This sounds like a situation where the BUILD_PATH_PREFIX_MAP value is incorrect or at least does not match the intentions of the caller of rewrite_absolute_path_existing. It might make sense to raise an exception in this case, or at least to signal the situation with a return value different from the one from case (1).

Also: have you ever commented on the proposed feature of asking tools to "invert" a given BUILD_PATH_PREFIX_MAP mapping? (The idea would be that producers and consumers could use the same mapping, but consumers would be asked to 'invert' the mapping.)

This was suggested there and there by @dbuenzli in your original issue, I asked about it there in the larger PR (my question was: have you considered this suggestion and does it solve the question of how to specify one-to-many relations?) and in the present discussion again, but I haven't been able to locate a comment from you about this proposed feature or an answer to my questions about it (does it solve your problem? did you consider it?). I would be curious to hear what you think about it. I wonder if there is some misunderstanding about this suggestion that makes you not consider it, because from a distance it feels like it could be part of a nice solution to the overall problem of "deployment phase support".

I am fine with pred_rewrite_opt. This is a form of lookup into the map that is different from the one defined in the specification, but it does not change the syntax or extend the map data structure in any way.

Thank you.

On the other hand:

• The implementation is very redundant with the rewrite_opt function already provided, I would want a single implementation of the lookup logic. (For example rewrite_opt can be an instance of pred_rewrite_opt with a trivial predicate.)
• The implementation calls subst twice on each path, which I find a bit distasteful. You could implement this using List.find_map instead.

I had thought about that. I was concerned that the version with the predicate might slow down the version used in the build phase. But I would be happy to consolidate them and have the build phase version just use a true predicate.

• I'm not excited by the name pred_rewrite_opt, maybe find_rewrite?

I like find_rewrite.

A more vague question: rewrite_absolute_path_existing does not rewrite the input path at all if there are several possible rewrites, but none of them exists. I don't see a clearly better choice, but I wonder if doing this silently is the right behavior. Intuitively I would distinguish two cases:

1. There is no matching source prefix in the prefix map, so not doing any rewrite is clearly the right behavior.
2. There are matching prefixes in the prefix map, but none of them satisfy the predicate. This sounds like a situation where the BUILD_PATH_PREFIX_MAP value is incorrect or at least does not match the intentions of the caller of rewrite_absolute_path_existing. It might make sense to raise an exception in this case, or at least to signal the situation with a return value different from the one from case (1).

Those thoughts had occurred to me too. During the deployment phase it seems that it is the responsibility of the map producer should be to give a map where the abstract artifacts are found, and if not, that should be an error. I will make a corresponding change.

Also: have you ever commented on the proposed feature of asking tools to "invert" a given BUILD_PATH_PREFIX_MAP mapping? (The idea would be that producers and consumers could use the same mapping, but consumers would be asked to 'invert' the mapping.)

No, I haven't, so I will now. The mapping Dune uses is cwd -> /workspace_root. That will have been used, conceivably by different users on different machines in different countries with different operating systems. In each of those cases cwd had different values. The result is the products in the opam repository. The BUILD_PATH_PREFIX_MAP spec says the maps are not to be saved, and even if they had been, it would be no help as the build environment is different from the deployment environment.

The only case where inverting the map might make sense is for the local developer for the mapping used to build the products in his own development directory, because then the build environment and the deployment environment are the same. But even here, the map is used only in the _build/<context> directory, and it misses the desired reference to the source directory, and also the references to the opam library.

It is true that the logical inversion of cwd -> /workspace_root will be used. It will be

opamlib=/workspace_root:builddir=/workspace_root:sourcedir=/workspace_root

where again, the right-most mapping takes precedence. Using such a map is made possible by the existence of the newly-named find_rewrite. So thank you again.

I hope this adequately answers that suggestion.

I was concerned that the version with the predicate might slow down the version used in the build phase.

Just to clarify, there is nothing performance-sensitive about these functions, so we have to optimize for clarity and maintainability instead of performance. (If we were to find out that there is a cost, or possibly for memory-sharing reasons, we could add a memoization step.)

Just to clarify, there is nothing performance-sensitive about these functions, so we have to optimize for clarity and maintainability instead of performance. (If we were to find out that there is a cost, or possibly for memory-sharing reasons, we could add a memoization step.)

Understood.

@richardlford I think the problem we are having is that you are taking the current map made by dune as cast in stone. I think dune should change the way it maps build artefacts: see this comment.

With the map I suggest in this comment, there is:

1. No need to save it, that would defeat the purpose anyways.
2. The fact that cwd can have different values on different machines (and even your own) is irrelevant.
3. The map setup by dune for compiling reproducible artefacts becomes usable by ocamldebug to map back these paths to the concrete artefacts it needs to look up.

I still fail to see what's the problem with this simple approach.

I still fail to see what's the problem with this simple approach.

You are right. I think the problem is that I thought you were somehow suggesting there was a map that needed to be inverted. Instead, what I now understand is that the build tool, e.g. Dune, is responsible for making the map during the build phase. And that the build tool must also be responsible for building the deployment phase map. That deployment map will be a logical inverse of the build phase map. Then it will likely build a map as you suggested in your comment.

If the user has their source layout isomorphic to the install layout, then Dune can get by with the simple maps I suggested. But if the user has a more complex source layout, then Dune will have more work to create the maps, both for build and deployment phases.

The find_rewrite feature is still needed in order to find sources preferentially in the source directory rather than the build directory.

And, as you suggested, invoking the debugger should be a push-button operation and the build tool should handle the setup. I totally agree.

I have a question about the CI testing environment. I'd like to write some tests that use Dune. I have written an ocamltest action, has_dune, that checks whether Dune is available, and would write the tests guarded by that action.

My question is, does the CI testing environment have Dune? If not, then the tests I write would only be useful locally. Presumably one might also care about which version of dune was available.

If there were other build systems that used BUILD_PATH_PREFIX_MAP we could consider testing with them as well, but for now I'm focused on getting things to work for Dune.

Any opinions on having some build system tests in the OCaml test suite?

The find_rewrite feature is still needed in order to find sources preferentially in the source directory rather than the build directory.

This is not strictly true, since the build tool could make a mapping pair for each individual source file that mapped to its location in the source directory rather than in the build directory. This would be a more specific mapping than the mappings that map directories, so that would work.

But having find_rewrite makes it possible for the build tool to get by with directory mappings, thus simplifying things. But the user need not be concerned with this as that will be handled transparently by the build tool. But the build tool does need to be aware that deployment-phase tools like ocamldebug are using find_rewrite.

I have pushed a commit with changes to reflect the suggestions from @gasche. One additional helper not mentioned above is

val matching_dirs : map -> path -> path list
(** [matching_dirs map absdir] accumulates a list of existing directories,
  [dirs], that are the result of mapping abstract directory, [absdir],
  over all the mapping pairs in [map]. The list [dirs] will be in
  priority order (head as highest priority). If there is success,
  returns [dirs]. If no source in the map was a prefix of [absdir],
  returns [[]]. If at least one source in the map was a prefix of
  [absdir], but none of the mappings were existing directories,
  raise Not_found.
*)

This is needed for handling the directories in the debug information.
An alternate would have been to collect the abstract directories, and then,
when looking something up (like a .cma), make the full abstract path
and use rewriite_exists on that. For now, I decided to go with this
approach.

I'll now proceed to do more testing.

I've squashed and pushed again, with a small change to do a List.rev on the list of debug source directories, so they will be in priority order.

I pushed some of the tests from other PR and added some new unit tests for matching_dirs and rewrite_exists. Reminder: the ocamltest and testsuite changes are not in this PR, but are temporarily here for the purpose of testing.

OK. Based on my testing, I think the compiler and debugger changes are complete and functionally correct. So I think this is ready for a detailed code review.

The changes have been squashed into two commits. One is the compiler and debugger changes. That is the commit to review. The other has ocamltest enhancements and the tests for the functionality. They will not be merged as part of the PR, but are there for your reference, and to verify that the code is passing the tests. When the main part is approved this part will be removed before the merge.

I wanted to do some testing with Dune, but that is something at a higher level. Since there does not appear to be an easy way to do Dune testing in the ocaml/testsuite, I will do that separately in the Dune test suite, and start investigating how to implement a dune debug command.

I don't understand the potential impact of rewriting all paths provided to the compiler, not just relative paths. I don't know if anyone else does, and I'm not sure if there is a testing strategy to learn of issues in advance. (I suspect that we don't know what we want to check, and that complaints if any would come from people with non-standard use-cases we don't know about, not building/running code from the opam repository.) I have gathered my courage to approve the change in any case, with the idea that we would revert or at least change course if we learned of major issues after the release.

I don't know what other build systems use the OCaml compiler and also do BUILD_PATH_PREFIX_MAP mapping, but Dune at least is only doing rewriting of paths in the current build directory. So if ocamlc ask for rewriting for some other absolute paths, nothing will happen. So it seems that rewriting other absolute paths will only have effect if the tool setting BUILD_PATH_PREFIX_MAP intends it to.

One case where ocamlc was missing rewriting was the case where the user passed an absolute path to the source file (ocamltest does this). In that case the absolute path to the source was leaking in the debug event paths.

In a separate conversion with @lpw25, #12106 (comment), I determined that the list of debug directories includes all the directories where the source files are found. The conclusion is that if the debugger just looked up the basename of the source path using the provided directories, then the sources would be found.

So an alternate for the debug events, rather than using BUILD_PATH_PREFIX_MAP mapping, is just to replace the source path with the basename. That would prevent absolute paths from leaking. And even when the debug event path is relative, it often will have an irrelevant part, so it would be good to get rid of the directory part of the debug event path. So I'm going to try that approach for now.

From a distance it sounds like BUILD_PATH_PREFIX_MAP is a more precise approach that would help avoid weird ambiguities and under-specifications -- if several modules in different subdirectories have the same filename for example, but the other approach, less precise, is less work to implement.

@gasche, I squashed, rebased, and force-pushed, but I kept the changes in response to your review separate until you have a chance to look at them. The commits are now the following:

1. The original compiler and debugger commit.
2. Changed for the compiler and debugger in response to your review. The most significant change is to use the new build_path_prefix_map API you suggested.
3. The original ocamltest and testsuite.
4. Changes to ocamltest you suggested, but also changes to my tests to use the new API and avoid the need for ocamldebug to expose internals.

As before, only commits 1 and 2 are part of this PR. Commits 3 and 4 are for reference and will be moved to a separate PR.

As mentioned above, I'm not yet sure about the rewriting of debug events. I believe the basename of the debug event path along with the debug directories should be sufficient to find the sources, but I've not yet gotten that to work.

If you wish, I can split out a separate PR with just the Build_path_prefix_map and Location changes if you think those parts are ready.

Another quick push to fix alldepend problem.

@gasche, continuing to follow your suggestion I split out part 2 which is just the bytecomp changes. They are now in a new PR, #12139.

I converted to draft because it is not intended to merge this PR. Instead PRs are being split off it.

This PR has now been updated and consists of the following parts:

1. bytecomp, parsing, and utils changes. These are from Modify bytecomp for BUILD_PATH_PREFIX_MAP #12139 and will go away from this PR once that is merged.
2. ocamltest and testsuite changes. These are not part of this PR, but are left in for now to show that this code has been tested. Note that I had to revised the tests since the ocamltest syntax changed since these were written, and also now ocamldebug uses the DEPLOY_PATH_PREFIX_MAP environment variable.
3. debugger, man, and manual changes. This is the code to be reviewed.
   Thanks.