Do not automatically add the .opt suffix when calling binaries #3051

kit-ty-kate · 2020-01-22T15:45:14Z

When I tried to fix elpi regarding #3050, I switch the use of the (system "camlp5o ...") function to (run camlp5o ...) but the following error message appeared:

     camlp5o src/parser.pp.mli (exit 2)
(cd _build/default && /home/kit_ty_kate/.opam/4.10/bin/camlp5o.opt -I . -I +camlp5 pa_extend.cmo pa_lexer.cmo src/parser.mli) > _build/default/src/parser.pp.mli
Error while loading "pa_extend.cmo": native-code program cannot do a dynamic load.
     camlp5o src/parser.pp.ml (exit 2)
(cd _build/default && /home/kit_ty_kate/.opam/4.10/bin/camlp5o.opt -I . -I +camlp5 pa_extend.cmo pa_lexer.cmo src/parser.ml) > _build/default/src/parser.pp.ml
Error while loading "pa_extend.cmo": native-code program cannot do a dynamic load.

Since camlp5 installs binaries with the .opt suffix, dune automatically uses them, however the semantics is a bit different than the ones in the ocaml compiler this feature was used for.

I believe this is safe anyway because since OCaml 4.04 the ocaml executables are native by default. See https://github.com/ocaml/ocaml/blob/646d30404e6b5fa0d49aea3860cbf4efe3910601/Changes#L4316

nojb · 2020-01-22T15:53:05Z

I thought we had fixed this in #2543

ejgallego · 2020-01-22T15:59:18Z

There are two ways to fix this:

use bash or some other form of quoting that will prevent the replacement
[preferred] use mkcamlp5 to generate an elpi_pp binary which can then be used to preprocess these files much faster.

kit-ty-kate · 2020-01-22T17:17:41Z

I thought we had fixed this in #2543

I forgot to mention this but I'm using dune master (the same issue appears in dune 2.1.2)

There are two ways to fix this:

* use `bash` or some other form of quoting that will prevent the replacement

* [preferred] use mkcamlp5 to generate an `elpi_pp` binary which can then be used to preprocess these files much faster.

Doesn't the bash method also bring the same issue than the original (system "...")?
For the mkcamlp5 method it's the same issue with the .opt suffix. mkcamlp5.opt will be called, I could change the cmo file into cmx but this wouldn't be correct on all platform anymore.

nojb · 2020-01-22T17:44:42Z

I forgot to mention this but I'm using dune master (the same issue appears in dune 2.1.2)

But have things changed again since the merging of #2543 ? That PR was actually motivated by this issue with camlp5 !

kit-ty-kate · 2020-01-22T21:43:09Z

But have things changed again since the merging of #2543 ? That PR was actually motivated by this issue with camlp5 !

Apparently

rgrinberg · 2020-01-23T07:44:38Z

I thought we had fixed this in #2543

I re-read the diff and I don't see where this was fixed. From what I can tell, the .opt detection was moved to Context.which, but this is still has logic for .opt. In any case, I support just getting rid of this detection. Software that uses dune will install the fastest binaries without a special suffix.

nojb · 2020-01-23T11:52:57Z

I thought we had fixed this in #2543

I re-read the diff and I don't see where this was fixed. From what I can tell, the .opt detection was moved to Context.which, but this is still has logic for .opt.

I thought Context.which was only supposed to be used to resolve compiler tools, otherwise Bin.which was used. This is from memory, though, so I may be wrong...

rgrinberg · 2020-01-23T15:08:23Z

I thought Context.which was only supposed to be used to resolve compiler tools, otherwise Bin.which was used. This is from memory, though, so I may be wrong...

Bin.which does not cache look-ups whileContext.which does. That's the main reason we use Context.which under the hood.

I guess we can fix this and introduce a private which_tool in Context where this .opt look up is done.

ghost · 2020-01-27T12:06:27Z

I guess we can fix this and introduce a private which_tool in Context where this .opt look up is done.

That seems good to me. Let's look for .opt binaries only for tools coming from the compiler distribution.

rgrinberg · 2020-01-28T04:45:38Z

@kit-ty-kate do you have time to update the PR? Otherwise I can take care of it.

kit-ty-kate · 2020-01-28T11:25:16Z

@rgrinberg I updated the PR against master

Signed-off-by: Rudi Grinberg <[email protected]>

rgrinberg · 2020-01-29T05:19:37Z

@kit-ty-kate I meant updating the PR so that .opt detection is limited to internal tools. In any case, I updated the PR myself. Thanks to all!

rgrinberg · 2020-01-29T06:01:35Z

A review from @nojb or @diml is necessary, since I'm now one of the authors of the PR.

nojb

LGTM

ghost · 2020-01-29T11:12:41Z

Me too!

@shonfeder

…lugin, dune-private-libs and dune-glob (2.2.0) CHANGES: - `dune test` is now a command alias for `dune runtest`. This is to make the CLI less idiosyncratic (ocaml/dune#3006, @shonfeder) - Allow to set menhir flags in the `env` stanza using the `menhir_flags` field. (ocaml/dune#2960, fix ocaml/dune#2924, @bschommer) - By default, do not show the full command line of commands executed by `dune` when `dune` is executed inside `dune`. This is to make integration tests more reproducible (ocaml/dune#3042, @diml) - `dune subst` now works even without opam files (ocaml/dune#2955, fixes ocaml/dune#2910, @fangyi-zhou and @diml) - Hint when trying to execute an executable defined in the current directory without using the `./` prefix (ocaml/dune#3041, fixes ocaml/dune#1094, @voodoos). - Extend the list of modifiers that can be nested under `with-accepted-exit-codes` with `chdir`, `setenv`, `ignore-<outputs>`, `with-stdin-from` and `with-<outputs>-to` (ocaml/dune#3027, fixes ocaml/dune#3014, @voodoos) - It is now an error to have a preprocessing dependency on a ppx rewriter library that is not marked as `(kind ppx_rewriter)` (ocaml/dune#3039, @snowleopard). - Fix permissions of files promoted to the source tree when using the shared cache. In particular, make them writable by the user (ocaml/dune#3043, fixes ocaml/dune#3026, @diml) - Only detect internal OCaml tools with `.opt` extensions. Previously, this detection applied to other binaries as well (@kit-ty-kate, @rgrinberg, ocaml/dune#3051). - Give the user a proper error message when they try to promote into a source directory that doesn't exist. (ocaml/dune#3073, fix ocaml/dune#3069, @rgrinberg) - Correctly build vendored packages in `-p` mode. These packages were incorrectly filtered out before. (ocaml/dune#3705, @diml) - Do not install vendored packages (ocaml/dune#3704, @diml) - `make` now prints a message explaining the main targets available (ocaml/dune#3085, fix ocaml/dune#3078, @diml) - Add a `byte_complete` executable mode to build programs as self-contained bytecode programs (ocaml/dune#3076, fixes ocaml/dune#1519, @diml)

@shonfeder

…lugin, dune-private-libs and dune-glob (2.2.0) CHANGES: - `dune test` is now a command alias for `dune runtest`. This is to make the CLI less idiosyncratic (ocaml/dune#3006, @shonfeder) - Allow to set menhir flags in the `env` stanza using the `menhir_flags` field. (ocaml/dune#2960, fix ocaml/dune#2924, @bschommer) - By default, do not show the full command line of commands executed by `dune` when `dune` is executed inside `dune`. This is to make integration tests more reproducible (ocaml/dune#3042, @diml) - `dune subst` now works even without opam files (ocaml/dune#2955, fixes ocaml/dune#2910, @fangyi-zhou and @diml) - Hint when trying to execute an executable defined in the current directory without using the `./` prefix (ocaml/dune#3041, fixes ocaml/dune#1094, @voodoos). - Extend the list of modifiers that can be nested under `with-accepted-exit-codes` with `chdir`, `setenv`, `ignore-<outputs>`, `with-stdin-from` and `with-<outputs>-to` (ocaml/dune#3027, fixes ocaml/dune#3014, @voodoos) - It is now an error to have a preprocessing dependency on a ppx rewriter library that is not marked as `(kind ppx_rewriter)` (ocaml/dune#3039, @snowleopard). - Fix permissions of files promoted to the source tree when using the shared cache. In particular, make them writable by the user (ocaml/dune#3043, fixes ocaml/dune#3026, @diml) - Only detect internal OCaml tools with `.opt` extensions. Previously, this detection applied to other binaries as well (@kit-ty-kate, @rgrinberg, ocaml/dune#3051). - Give the user a proper error message when they try to promote into a source directory that doesn't exist. (ocaml/dune#3073, fix ocaml/dune#3069, @rgrinberg) - Correctly build vendored packages in `-p` mode. These packages were incorrectly filtered out before. (ocaml/dune#3705, @diml) - Do not install vendored packages (ocaml/dune#3704, @diml) - `make` now prints a message explaining the main targets available (ocaml/dune#3085, fix ocaml/dune#3078, @diml) - Add a `byte_complete` executable mode to build programs as self-contained bytecode programs (ocaml/dune#3076, fixes ocaml/dune#1519, @diml)

kit-ty-kate mentioned this pull request Jan 24, 2020

Avoid build failures when the camlp5 binary changes LPCIC/elpi#53

Merged

kit-ty-kate force-pushed the remove-automatic-opt-suffix branch from a414dc8 to 126d2df Compare January 28, 2020 11:20

.opt detection only for internal OCaml tools

4cd24a6

Signed-off-by: Rudi Grinberg <[email protected]>

rgrinberg force-pushed the remove-automatic-opt-suffix branch from 8d9913f to 4cd24a6 Compare January 29, 2020 05:18

rgrinberg requested review from a user and nojb January 29, 2020 06:00

nojb approved these changes Jan 29, 2020

View reviewed changes

rgrinberg merged commit 102565a into ocaml:master Jan 31, 2020

rgrinberg mentioned this pull request Feb 6, 2020

[new release] dune-build-info, dune, dune-configurator, dune-action-plugin, dune-private-libs and dune-glob (2.2.0) ocaml/opam-repository#15795

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not automatically add the .opt suffix when calling binaries #3051

Do not automatically add the .opt suffix when calling binaries #3051

kit-ty-kate commented Jan 22, 2020

nojb commented Jan 22, 2020

ejgallego commented Jan 22, 2020

kit-ty-kate commented Jan 22, 2020

nojb commented Jan 22, 2020

kit-ty-kate commented Jan 22, 2020

rgrinberg commented Jan 23, 2020

nojb commented Jan 23, 2020

rgrinberg commented Jan 23, 2020

ghost commented Jan 27, 2020

rgrinberg commented Jan 28, 2020

kit-ty-kate commented Jan 28, 2020

rgrinberg commented Jan 29, 2020

rgrinberg commented Jan 29, 2020

nojb left a comment

ghost commented Jan 29, 2020

Do not automatically add the .opt suffix when calling binaries #3051

Do not automatically add the .opt suffix when calling binaries #3051

Conversation

kit-ty-kate commented Jan 22, 2020

nojb commented Jan 22, 2020

ejgallego commented Jan 22, 2020

kit-ty-kate commented Jan 22, 2020

nojb commented Jan 22, 2020

kit-ty-kate commented Jan 22, 2020

rgrinberg commented Jan 23, 2020

nojb commented Jan 23, 2020

rgrinberg commented Jan 23, 2020

ghost commented Jan 27, 2020

rgrinberg commented Jan 28, 2020

kit-ty-kate commented Jan 28, 2020

rgrinberg commented Jan 29, 2020

rgrinberg commented Jan 29, 2020

nojb left a comment

Choose a reason for hiding this comment

ghost commented Jan 29, 2020