pkg lock fails on repos that need dune subst

[Alizter]

Issue

When a dune-project file contains something that will be processed by dune subst we get issues locking.

The following example is directly from https://github.com/ocaml-dune/notty:

(lang dune 1.7)
(name notty)
(version %%VERSION_NUM%%)

The build instructions in the opam file are to do dune subst in the development versions.

However running dune pkg lock runs into this issue:

Internal error, please report upstream including the contents of _build/log.
Description:
  ("Unexpected exception raised while solving dependencies",
   { exception =
       { exn =
           "Failure(\"Invalid character '%' in package version \\\"%%VERSION_NUM%%\\\"\")"
       ; backtrace =
           "Raised at Stdlib.failwith in file \"stdlib.ml\", line 29, characters 17-33\n\
            Called from Stdlib__String.iter in file \"string.ml\", line 76, characters 31-49\n\
            Called from OpamPackage.Version.of_string in file \"opamPackage.ml\", lines 112-117, characters 4-7\n\
            Called from Dune_pkg__Local_package.For_solver.to_opam_file in file \"dune_pkg__Local_package.ml\", line 91, characters 34-83\n\
            Called from Dune_pkg__Opam_solver.solve_lock_dir.(fun) in file \"dune_pkg__Opam_solver.ml\", line 1698, characters 29-72\n\
            Called from Stdlib__Map.Make.map in file \"map.ml\", line 312, characters 19-22\n\
            Called from CamlinternalLazy.do_force_block in file \"camlinternalLazy.ml\", line 49, characters 17-27\n\
            Re-raised at CamlinternalLazy.do_force_block in file \"camlinternalLazy.ml\", line 56, characters 4-11\n\
            Called from CamlinternalLazy.force_lazy_block in file \"camlinternalLazy.ml\" (inlined), line 78, characters 27-67\n\
            Called from Dune_pkg__Opam_solver.Context.candidates in file \"dune_pkg__Opam_solver.ml\", line 335, characters 32-61\n\
            Called from Fiber__Scheduler.exec in file \"fiber__Scheduler.ml\", line 77, characters 8-11\n\
            Re-raised at Stdune__Exn.raise_with_backtrace in file \"stdune__Exn.ml\" (inlined), line 39, characters 27-56\n\
            Called from Stdune__Exn_with_backtrace.reraise in file \"stdune__Exn_with_backtrace.ml\", line 21, characters 33-71\n\
            Called from Fiber__Scheduler.exec in file \"fiber__Scheduler.ml\", line 77, characters 8-11\n\
            "
       }
   })
Raised at Stdune__Code_error.raise in file "stdune__Code_error.ml", line 11,
  characters 30-62
Called from Fiber__Scheduler.exec in file "fiber__Scheduler.ml", line 77,
  characters 8-11

Which is an error raised from internal opam code. We have a workflow to observe this https://github.com/ocaml/dune/actions/runs/20561364995/job/59052361540

This won't affect most users, but there will be a handful of users who will be left scratching their heads when they encounter this error message.

Solution

Now it can be argued that it is up to the user to have run dune subst themselves before locking, however we can probably do a better error message here. I'm not sure its wise to run dune subst when locking so I wouldn't suggest that. I'm open to any other ideas on what can be done in this situation.

[avsm]

Now it can be argued that it is up to the user to have run dune subst themselves before locking

I'm not sure how the user can know how to run dune subst in this case, when it's required by third party packages..

[rgrinberg]

I think we just need a good error messages as this version is indeed invalid. Dune doesn't validate it, but the various opam libraries we're using for the solver do.

My recommendation to the user in this case is just to remove this (version %%VERSION_NUM%%) because it seems redundant. This is a line that is inserted by dune subst into the project file without any templating already. I imagine that the user just wasn't aware of that.

[Alizter]

Now it can be argued that it is up to the user to have run dune subst themselves before locking

I'm not sure how the user can know how to run dune subst in this case, when it's required by third party packages..

I should have been clear in the original issue that if you pull such a package in as a dependency, then it typically contains an opam file with a dune subst step so therefore dune knows to run that before building it so there is no issue.

This particular issue is what happens when you decide to develop the package that requires dune subst. I've checked again, and with a non-pkg setup there is no need to run dune subst, so perhaps we could do something smarter here.

On the other hand what Rudi says is also correct, that it might just be a mistake to have included (version) with a placeholder, since dune subst should have taken care of that too.

The clearer error message in this case would be for dune to tell the user that the (version) field is unneeded since dune subst will handle it correctly in packaging. However, if we can detect that we might as well ignore it.