Fix many different issues of Sites by bobot · Pull Request #4645 · ocaml/dune

bobot · 2021-05-21T22:06:24Z

Should fix:

Protect sites names against reserved keywords #4219 by adding an underscore for site name that start with an OCaml keyword and finish with underscores
Error without a location: The package c is not found #4325 by adding location in Section_with_site
Dune sites doesn't work when installed by opam #4212 and dune-site does not work with opam #4198 by:
- not promoting the install file during dune build -p <pkg> @install
- creating the install file during a new phase dune install -p <pkg> --create-install-files <pkg> which substitute the files if needed into an intermediate directory
generate_sites_module creates issues when used with multiple packages #4415 by delaying the computation of the content of the generated file

bobot · 2021-05-26T16:21:21Z

@ejgallego Many fixes for the Sites features.

rgrinberg · 2021-05-26T21:54:02Z

src/dune_rules/artifact_substitution.ml

+  | n -> loop Scan0 ~pos:0 ~end_of_data:n
+
+let test_file ~src () =
+  let ic = Io.open_in src in


This should be lazy IMO. It's cleaner to have all the side effects inside fiber.

Could you be more specific, I'm a little lost with Io and Fiber. I would have though that we should not use Io inside fibers but we do it a lot. What should I use?

Consider this:

let test_src = test_file ~src () in let* test_src = test_src ..

You're opening the channel after the first let, but clean up is reserved until the bind.

I would prefer if all the side effects occur after the bind.

I changed also copy_file to respect this.

rgrinberg · 2021-05-26T21:57:21Z

bin/install_uninstall.ml

            ~doc:
              "Make the binaries relocatable (the installation directory can \
               be moved).")
+    and+ create_install_files =


If it's not actually installing, why does it need to be under the install subcommand? Can't this be done just by building a target.

We need to know the install directory for the substitution. It is known only by the install command.

rgrinberg · 2021-05-26T21:58:28Z

src/dune_rules/artifact_substitution.ml

      Fiber.return ())
    (fun () -> copy ~conf ~input_file:src ~input:(input ic) ~output:(output oc))
+
+let test ~input =


This function is too similar to copy. The common code should be factored out.

Indeed done using a mode argument.

Signed-off-by: François Bobot <francois.bobot@cea.fr>

Fixes ocaml#4415 Signed-off-by: François Bobot <francois.bobot@cea.fr>

instead of Path.Build.t Signed-off-by: François Bobot <francois.bobot@cea.fr>

Signed-off-by: François Bobot <francois.bobot@cea.fr>

- Substitute the files in an intermediate directory if needed - Backward compatible Fixes ocaml#4198 ocaml#4212 Signed-off-by: François Bobot <francois.bobot@cea.fr>

Signed-off-by: François Bobot <francois.bobot@cea.fr>

Fixes ocaml#4325 Signed-off-by: François Bobot <francois.bobot@cea.fr>

Signed-off-by: François Bobot <francois.bobot@cea.fr>

a name matching "$ocaml_keyword_*" is converted into name^"_" Fixes ocaml#4219 Signed-off-by: François Bobot <francois.bobot@cea.fr>

So that opening and closing be evaluated directly Signed-off-by: François Bobot <francois.bobot@cea.fr>

ejgallego · 2021-06-03T15:15:32Z

@rgrinberg , would you mind re-reviewing this?

Ideally we should get it merged soon as to have it in 2.9

ejgallego · 2021-06-07T19:42:41Z

@bobot , 2.9 ping

bobot · 2021-06-09T14:55:39Z

Sorry I missed the approval of @rgrinberg .

ejgallego · 2021-06-10T20:40:27Z

@bobot , sorry for not looking at this closely before, but it seems to me that this PR landed in 2.9 but not in main, is that intended?

Note that:

there are no changes entry in this PR
the bugs you mention in the PR summary have not been closed due to the target branch

@emillon

…ator, dune-private-libs, dune and dune-build-info (2.9.0) CHANGES: - Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon) - Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb) - Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos) - Allow to set up merlin in a variant of the default context (ocaml/dune#4145, @TheLortex, @voodoos) - Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon) - Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01) - Fix generation of merlin configuration when using `(include_subdirs unqualified)` on Windows (ocaml/dune#4745, @nojb) - Fix bug for the install of Coq native files when using `(include_subdirs qualified)` (ocaml/dune#4753, @ejgallego) - Allow users to specify install target directories for `doc` and `etc` sections. We add new options `--docdir` and `--etcdir` to both Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723, @ejgallego, thanks to @JasonGross for reporting this issue) - Fix issue where Dune would ignore `(env ... (coq (flags ...)))` declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg) - Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego) - Fix a bug where instrumentation flags would be added even if the instrumentatation was disabled (@nojb, ocaml/dune#4770) - Fix ocaml/dune#4682: option `-p` takes now precedence on environement variable `DUNE_PROFILE` (ocaml/dune#4730, ocaml/dune#4774, @bobot, reported by @dra27 ocaml/dune#4632) - Fix installation with opam of package with dune sites. The `.install` file is now produced by a local `dune install` during the build phase (ocaml/dune#4730, ocaml/dune#4645, @bobot, reported by @kit-ty-kate ocaml/dune#4198) - Fix multiple issues in the sites feature (ocaml/dune#4730, ocaml/dune#4645 @bobot, reported by @Lelio-Brun ocaml/dune#4219, by @Kakadu ocaml/dune#4325, by @toots ocaml/dune#4415)

@emillon

…ator, dune-private-libs, dune and dune-build-info (2.9.0) CHANGES: - Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon) - Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb) - Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos) - Allow to set up merlin in a variant of the default context (ocaml/dune#4145, @TheLortex, @voodoos) - Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon) - Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01) - Fix generation of merlin configuration when using `(include_subdirs unqualified)` on Windows (ocaml/dune#4745, @nojb) - Fix bug for the install of Coq native files when using `(include_subdirs qualified)` (ocaml/dune#4753, @ejgallego) - Allow users to specify install target directories for `doc` and `etc` sections. We add new options `--docdir` and `--etcdir` to both Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723, @ejgallego, thanks to @JasonGross for reporting this issue) - Fix issue where Dune would ignore `(env ... (coq (flags ...)))` declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg) - Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego) - Fix a bug where instrumentation flags would be added even if the instrumentatation was disabled (@nojb, ocaml/dune#4770) - Fix ocaml/dune#4682: option `-p` takes now precedence on environement variable `DUNE_PROFILE` (ocaml/dune#4730, ocaml/dune#4774, @bobot, reported by @dra27 ocaml/dune#4632) - Fix installation with opam of package with dune sites. The `.install` file is now produced by a local `dune install` during the build phase (ocaml/dune#4730, ocaml/dune#4645, @bobot, reported by @kit-ty-kate ocaml/dune#4198) - Fix multiple issues in the sites feature (ocaml/dune#4730, ocaml/dune#4645 @bobot, reported by @Lelio-Brun ocaml/dune#4219, by @Kakadu ocaml/dune#4325, by @toots ocaml/dune#4415)

bobot force-pushed the fix_sites_installation_with_opam branch 3 times, most recently from fa85a54 to 5827743 Compare May 25, 2021 12:45

rgrinberg reviewed May 26, 2021

View reviewed changes

rgrinberg requested changes May 26, 2021

View reviewed changes

bobot force-pushed the fix_sites_installation_with_opam branch from 5827743 to fbf5bc3 Compare May 27, 2021 15:29

bobot requested review from emillon and nojb as code owners May 27, 2021 15:29

bobot added 11 commits May 28, 2021 11:58

[Install/Uninstall] Use an enum instead of a string

3c8fb20

Signed-off-by: François Bobot <francois.bobot@cea.fr>

[Site] generate_site_module fails with -p

4a18bca

Signed-off-by: François Bobot <francois.bobot@cea.fr>

[Sites] delay the computation of content of generate_sites_module

a55ee92

Fixes ocaml#4415 Signed-off-by: François Bobot <francois.bobot@cea.fr>

[Install.Entry] Generalize the functions to Path.t

b2f8624

instead of Path.Build.t Signed-off-by: François Bobot <francois.bobot@cea.fr>

[Artifact_substitution] Allow to test the presence of a placeholder

0588d0e

Signed-off-by: François Bobot <francois.bobot@cea.fr>

[Sites] Fix installation through opam

437a913

- Substitute the files in an intermediate directory if needed - Backward compatible Fixes ocaml#4198 ocaml#4212 Signed-off-by: François Bobot <francois.bobot@cea.fr>

[Sites] Fix an error message for generate_sites_module

26c2dea

Signed-off-by: François Bobot <francois.bobot@cea.fr>

[Sites] Better localisation of error

2b59357

Fixes ocaml#4325 Signed-off-by: François Bobot <francois.bobot@cea.fr>

[Sites] add failing test with sites name which is an OCaml keyword

9ce7d56

Signed-off-by: François Bobot <francois.bobot@cea.fr>

[Sites] sanitize site name in generated modules

35d6f51

a name matching "$ocaml_keyword_*" is converted into name^"_" Fixes ocaml#4219 Signed-off-by: François Bobot <francois.bobot@cea.fr>

[Artifact_substitution] Wrap some channel opening into a fiber

29427c8

So that opening and closing be evaluated directly Signed-off-by: François Bobot <francois.bobot@cea.fr>

bobot force-pushed the fix_sites_installation_with_opam branch from 12cddbd to 29427c8 Compare May 28, 2021 09:59

bobot added this to the 2.9 milestone Jun 3, 2021

rgrinberg approved these changes Jun 4, 2021

View reviewed changes

bobot merged commit 4f2ac8f into ocaml:2.9 Jun 9, 2021

bobot mentioned this pull request Jun 14, 2021

Fix sites installation with opam #4730

Merged

bobot mentioned this pull request Jun 24, 2021

[2.9] fix sites installation with opam #4774

Merged

ejgallego mentioned this pull request Jun 29, 2021

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

Merged

Conversation

bobot commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobot commented May 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejgallego commented Jun 3, 2021

Uh oh!

ejgallego commented Jun 7, 2021

Uh oh!

bobot commented Jun 9, 2021

Uh oh!

ejgallego commented Jun 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bobot commented May 21, 2021 •

edited

Loading