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

Reproduction case to show that installing deps to the stdlib breaks #10565

Merged

Conversation

Leonidas-from-XIV
Copy link
Collaborator

When a package specifies in its META file that its artifacts are installed into ^ (or +) thus the stdlib and its .install file then installs it into $lib_root/ocaml/... this does not yield a -I flag when compiling (as normally the stdlib is implied).

This works on compilers in OPAM switches, but if the package is installed in _build as part of dune-pkg, then the package files are installed into $pkg_lib_root/ocaml/... but running ocamlc does not pick up the path.

This attempts to reproduce the issue, however the actual issue with num looks a bit different as ocamlc fails to locate big_int.cmi but the cmi is not in any path that is added to -I.

@rgrinberg
Copy link
Member

That's indeed and addressing it is not easy. The fundamental issue is that what num is doing is violating dune's sandbox assumptions by trying to install things into a different package's directory. We could somehow detect this and create a new stdlib directory that includes num's additions for any package that depends on num. Of course that's not easy and is probably unintuitive (though something like this needs to be done for widows anyway).

Have you looked into working with upstream to address this issue in a different way? The easiest way forward would be to prevent num from installing things in the stdlib dir.

@Leonidas-from-XIV
Copy link
Collaborator Author

I agree that this is fairly problematic behavior but given the history of num, being formerly part of the stdlib I can understand that the authors wanted to preserve the behavior that as long as the user installs the num package, code using using the modules from num would work - no need to require findlib or add additional paths to -I; it works the same way as it always did. I can try to convince the num authors to adopt a more dune compatible approach, but it will break all non-findlib users of num. I could see this being a hard sell, given num is mostly a legacy package, kept around for things to not break but not for new software to use it.

An alternative approach I could imagine is that for every package that installs into an ocaml folder we implicitly add -I <pkg-lib>/ocaml into the ocamlc invocation, that way we keep all packages separate. The only case where this will break is if a package uses num but doesn't declare it (since that currently works, as the stdlib is automatically -I'd), but I feel that saying "if you want to use num, add num to your libraries" is an ok compromise.

The other option, as you suggest, with all packages installing to a project-global ocaml stdlib folder that is then automatically added to all builds would make all packages work but it does break a lot of nice properties that we currently have that I would really prefer the other approaches. This is the only one that preserves compatibility for packages that don't depend on num yet use it as if it were in the stdlib.

The final solution is forking num into an overlay and making it install into its own folder, but this doesn't solve the problem for other packages that potentially do the same and likewise breaks all non-findlib users.

@@ -57,4 +57,12 @@ With this project set up, lets depend on it.
$ cat > foo.ml <<EOF
> let () = Nondune.main ()
> EOF
$ dune exec ./foo.exe 2>&1 | sed -e 's#File unavailable: .*ocaml/nondune.cma$#File unavailable: ocaml/nondune.cma#'
$ dune exec ./foo.exe 2>&1 | dune_cmd stdlib-path
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you can:

  • get that from ocamlc -config
  • use BUILD_PATH_PREFIX_MAP
  $ OCAML_STDLIB=$(ocamlc -config|grep standard_library:|cut -d ' ' -f2)
  $ BUILD_PATH_PREFIX_MAP="/OCAML_STDLIB=$OCAML_STDLIB:$BUILD_PATH_PREFIX_MAP"

@rgrinberg
Copy link
Member

Given that this issue is only limited to a single package, I'm inclined to work with the num authors. The other workarounds seem really unappetizing and should be reserved until the workaround is useful for other packages.

We don't necessarily need to create an overlay for num. If we introduce a flag that num could use to detect its running under dune and then not try to install anything in another package's directory, that would be enough. We'll still need to mark all older versions as incompatible though.

@Leonidas-from-XIV
Copy link
Collaborator Author

Given that this issue is only limited to a single package, I'm inclined to work with the num authors.

I am not sure how prevalent this is in the general OCaml package universe. Recent compat packages like camlp-streams install to their own folders. Also, num when built in "modern" mode (OCaml 5+) installs to its own directory as well.

I've opened a PR for the "legacy" mode to install into the package folder as well: ocaml/num#40 (and adjust the META to use it), in this case the num package can be used both as a standard library drop-in, as well as a regular findlib package with its own folder.

If we introduce a flag that num could use to detect its running under dune and then not try to install anything in another package's directory, that would be enough. We'll still need to mark all older versions as incompatible though.

That's what opam-modern does. It's currently triggered by

  [make "PROFILE=release"
        "opam-legacy" {!ocaml:preinstalled & ocaml:version < "5.0.0~~"}
        "opam-modern" {ocaml:preinstalled | ocaml:version >= "5.0.0~~"}]

The PR linked above makes the "opam-legacy" mode a bit more similar to "opam-modern" by removing a bit of special-casing.

Signed-off-by: Marek Kubica <marek@tarides.com>
@Leonidas-from-XIV Leonidas-from-XIV merged commit 72c5505 into ocaml:main May 29, 2024
12 of 13 checks passed
@Leonidas-from-XIV Leonidas-from-XIV deleted the install-file-ignored branch May 29, 2024 09:46
MA0010 pushed a commit to MA0010/dune that referenced this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants