Skip to content

Add findlib toolchain handling for dune install.#3501

Closed
rizo wants to merge 2 commits intoocaml:mainfrom
rizo:rizo/fix-dune-install-x
Closed

Add findlib toolchain handling for dune install.#3501
rizo wants to merge 2 commits intoocaml:mainfrom
rizo:rizo/fix-dune-install-x

Conversation

@rizo
Copy link
Copy Markdown
Contributor

@rizo rizo commented May 22, 2020 •
edited
Loading

Problem

For a given package $PKG, when explicitly providing a findlib toolchain with -x $TOOLCHAIN, dune install does two things that, in my understanding, are incorrect:

  1. it will attempt to install targets for the default context, and not exclusively the targets for $TOOLCHAIN; and
  2. it attempts to install:
    • _build/default.$TOOLCHAIN/$PKG.install instead of
    • _build/default.$TOOLCHAIN/$PKG-$TOOLCHAIN.install.

These two points can be validated currently on master by updating ./test/blackbox-tests/test-cases/cross-compilation/run.t as follows:

   $ env OCAMLFIND_CONF=$PWD/etc/findlib.conf dune install --prefix=_prefix --dry-run -x foo
+  Error: The following <package>.install are missing:
+  - _build/default/p.install
+  - _build/default.foo/p.install
+  Hint: try running: dune build @install
+  [1]

As stated in point 1, even though -x foo was passed, _build/default/p.install is expected to be present. And, as stated in point 2, the install file location is _build/default.foo/p.install, but should be _build/default.foo/p-foo.install. At least given my understanding of dune's path conventions for non-default toolchains.

Solution

See the proposed behaviour in the first commit 479158e for comparison with the current behaviour.

More specifically, in the presence of the -x option, the implementation of dune install:

  • selects only the contexts that match the passed toolchain name; and
  • resolves the package install filenames using the toolchain name.

Alternatives

Regarding 1

Keep the current functionality where dune install -x $TOOLCHAIN ... installs targets from the contexts that don't have the $TOOLCHAIN. As a workaround to only install the desired toolchain targets, dune install -x $TOOLCHAIN --context=default.$TOOLCHAIN can be used, assuming the default context.

I think this would be fine, even though the user has explicitly passed a toolchain. This behaviour is somewhat unexpected, but at least can be tuned.

Regarding 2

The naming convention for install files could be changed in the build folder. Currently the generated files (with --promote-install-files) are:

  • ./_build/default.$TOOLCHAIN/$PKG-$TOOLCHAIN.install and;
  • ./$PKG-$TOOLCHAIN.install

Instead, dune could generate ./_build/default.$TOOLCHAIN/$PKG.install internally. To my knowledge, no collisions are possible, because the default.$TOOLCHAIN context is used as a namespace.

This would also work, but introduces an unnecessary inconsistency for the install filenames.

rizo added 2 commits May 22, 2020 13:11
@rizo
Update the cross-compilation test to correctly infer the installation…
479158e 
… paths.

Signed-off-by: Rizo I <rizo@odis.io>
@rizo
Correct the dune install command to correctly handle the -x option.
145b703 
This is achieved by filtering contexts with the provided findlib toolchain and
using that toolchain for install path detection.

Signed-off-by: Rizo I <rizo@odis.io>
@rizo rizo force-pushed the rizo/fix-dune-install-x branch from 8438b9f to 145b703 Compare May 22, 2020 12:11
@rizo
Copy link
Copy Markdown
Contributor Author

rizo commented May 22, 2020

Related to ocaml/opam#4204 and ocaml/opam#2476.

Am I correct in assuming that there's currently no way to instruct opam to install the $PKG-$TOOLCHAIN.install files?

@ghost
Copy link
Copy Markdown

ghost commented May 26, 2020

This was designed for the ocaml-cross repositories where the package names are suffixed with -$TOOLCHAIN. For instance lwt becomes lwt-windows. /cc @toots

Regarding dune install -x TOOLCHAIN installing only the files for TOOLCHAIN, overall that seems reasonable but the behaviour needs to be consistent between dune build and dune install. More precisely, at a high-level the new interpretation seems to be: if you pass -x TOOLCHAIN, then default.TOOLCHAIN becomes the main build context and default becomes an "auxiliary context" that is only used if needed to run things on the current machine.

For instance, IIRC dune build -x windows blah will build both _build/default/blah and _build/default.windows/blah. With the change you propose, dune build -x windows blah should only build _build/default.windows/blah.

BTW, while the new interpretation seems fine, it is also technically a breaking change, so we need to assess the impact before going through it.

@rizo
Copy link
Copy Markdown
Contributor Author

rizo commented May 26, 2020

Thanks for the explanation, @jeremiedimino!

Indeed, I later noticed that build behaviour is inclusive. I personally prefer the proposed interpretation, where -x filters the context(s) being built. But I'm happy with either: changing the behaviour of build to match the proposed one or correcting the PR to install targets for both contexts.

Let me know what's your preference, I'm happy to implement it.

@ghost
Copy link
Copy Markdown

ghost commented May 28, 2020

Well, I also like the proposed interpretation better, though I usually prefer to be strict about backward compatibility. That being said, I was reading the doc and the doc seems to suggest that if you have no dune-workspace file and you pass -x target, the native default build context is an auxiliary context. Given that plus the bug in dune install you pointed, I think I'm Ok considering that the current behaviour is a bug.

Just ccing @avsm and @samoht to make sure this won't impact mirage; right now you if you do dune build -x foo bah, dune will interpret that as requesting both _build/default/blah and _build/default.foo/blah. We'd like to change it to mean just _build/default.foo/blah. Is that better/worse/the same for the mirage workflow?

@avsm
Copy link
Copy Markdown
Member

avsm commented Jun 1, 2020

I appreciate you checking in on the Mirage usecase. I don't believe this would affect it, since we don't use -x (but instead generate a dune-workspace file with our specific flags). @samoht is leading the charge on this, so I'll let him confirm for sure.

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Jul 28, 2020

@samoht ping?

I'm also in favor of this change. @rizo would you be willing to change $ dune build -x as well?

@avsm
Copy link
Copy Markdown
Member

avsm commented Jul 29, 2020

@samoht is on vacation - I’m 99% sure this is fine, so go ahead and we will work around any (unlikely) issues in Mirage

@rizo
Copy link
Copy Markdown
Contributor Author

rizo commented Jul 29, 2020

I'm happy to give it a shot, @rgrinberg. Will try to submit a PR in a few days.

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Jul 29, 2020 via email

Base automatically changed from master to main January 14, 2021 17:08
@anmonteiro anmonteiro mentioned this pull request Mar 25, 2023
Merged
@rgrinberg rgrinberg closed this Mar 26, 2023
@rgrinberg rgrinberg added this to the 3.8.0 milestone Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.8.0

Development

Successfully merging this pull request may close these issues.

3 participants

@rizo @avsm @rgrinberg