test: expand %{deps} in (cat) by Alizter · Pull Request #8196 · ocaml/dune

Alizter · 2023-07-13T01:00:51Z

We fix a bug in which %{deps} with more than one dep would not expand inside of a cat.

changelog
test
version guard

Alizter · 2023-07-13T01:01:15Z

src/dune_rules/action_unexpanded.ml

@@ -68,6 +68,9 @@ module Action_expander : sig
    (* Evaluate a path in a position of dependency, such as in [(cat <dep>)] *)


This comment is kind of funny now.

Alizter · 2023-07-13T01:01:28Z

src/dune_rules/action_unexpanded.ml

          ~error_loc:(String_with_vars.loc sw) ~dir:t.dir

+      let expand_paths t sw =
+        let+ v, vs = expand t ~mode:At_least_one sw in


This seemed sensible to me.

ocaml-benchmarks · 2023-07-13T03:17:40Z

#8196 (c702049) changes the metrics as follows in comparison to main (a0145b2) when running on fermat (bench/monorepo/bench.Dockerfile):

Benchmark: default

Test: dune monorepo benchmarks

build from scratch changed by 5.0%
null build changed by 4.7%
watch mode: changing file in 'base' library changed by -3.6%
watch mode: changing file in 'file_path' library changed by -5.6%
watch mode: fixing error in file in 'base' library changed by -6.3%
watch mode: fixing error in file in 'file_path' library changed by -3.6%
watch mode: introducing error in file in 'base' library changed by -6.2%
watch mode: introducing error in file in 'file_path' library changed by -14.7%

rgrinberg · 2023-07-13T08:30:05Z

src/dune_rules/action_unexpanded.ml

      let fn = Expander.expand_path env sw in
      register_dep fn ~f:Option.some env acc

+    let register_deps x ~f env acc =


Too much copy pasting between this and register_dep

I've left only register_deps now.

ocaml-benchmarks · 2023-07-13T13:01:22Z

#8196 (200fa2d) changes the metrics as follows in comparison to main (a0145b2) when running on fermat (bench/monorepo/bench.Dockerfile):

Benchmark: default

Test: dune monorepo benchmarks

metric_name	Last PR value	Last main value	Delta
build from scratch	1123.210636	1129.820496	-0.6%
null build	66.686410	65.869692	1.2%
watch mode: changing file in 'base' library	69.752260	69.897738	-0.2%
watch mode: changing file in 'file_path' library	17.663967	18.689723	-5.5%
watch mode: fixing error in file in 'base' library	62.772896	63.562635	-1.2%
watch mode: fixing error in file in 'file_path' library	17.090973	17.633133	-3.1%
watch mode: introducing error in file in 'base' library	8.891082	8.154346	9.0%
watch mode: introducing error in file in 'file_path' library	2.333495	2.827425	-17.5%

Alizter · 2023-07-14T13:42:12Z

Do we want to version guard this? I could see it becoming an issue otherwise.

Alizter · 2023-07-17T18:24:24Z

@rgrinberg I've managed to version guard it now. The test demonstrates this behaviour too.

Alizter · 2023-07-18T13:02:48Z

@emillon Could you also take a look, since you recently implemented the cat of a list feature.

Signed-off-by: Ali Caglayan <alizter@gmail.com>

@Alizter

CHANGES: - Add `dune show rules` as alias of the `dune rules` command. (ocaml/dune#8000, @Alizter) - Fix `%{deps}` to expand properly in `(cat ...)` when containing 2 or more items. (ocaml/dune#8196, @Alizter) - Add `dune show installed-libraries` as an alias of the `dune installed-libraries` command. (ocaml/dune#8135, @Alizter) - Fix the `severity` of error messages sent over RPC which was missing. (ocaml/dune#8193, @Alizter) - Add `dune build --dump-gc-stats FILE` argument to dump garbage collection stats to a named file. (ocaml/dune#8072, @Alizter) - Fix bug with ppx and Reason syntax due to missing dependency in sandboxed action (ocaml/dune#7932, fixes ocaml/dune#7930, @Alizter) - Add `dune describe package-entries` to print all package entries (ocaml/dune#7480, @moyodiallo) - Improve `dune describe external-lib-deps` by adding the internal dependencies for more information. (ocaml/dune#7478, @moyodiallo) - Re-enable background file digests on Windows. The files are now open in a way that prevents race condition around deletion. (ocaml/dune#8262, fixes ocaml/dune#8268, @emillon)

@Alizter

CHANGES: - Add `dune show rules` as alias of the `dune rules` command. (ocaml/dune#8000, @Alizter) - Fix `%{deps}` to expand properly in `(cat ...)` when containing 2 or more items. (ocaml/dune#8196, @Alizter) - Add `dune show installed-libraries` as an alias of the `dune installed-libraries` command. (ocaml/dune#8135, @Alizter) - Fix the `severity` of error messages sent over RPC which was missing. (ocaml/dune#8193, @Alizter) - Add `dune build --dump-gc-stats FILE` argument to dump garbage collection stats to a named file. (ocaml/dune#8072, @Alizter) - Fix bug with ppx and Reason syntax due to missing dependency in sandboxed action (ocaml/dune#7932, fixes ocaml/dune#7930, @Alizter) - Add `dune describe package-entries` to print all package entries (ocaml/dune#7480, @moyodiallo) - Improve `dune describe external-lib-deps` by adding the internal dependencies for more information. (ocaml/dune#7478, @moyodiallo) - Re-enable background file digests on Windows. The files are now open in a way that prevents race condition around deletion. (ocaml/dune#8262, fixes ocaml/dune#8268, @emillon)

@Alizter

CHANGES: - Add `dune show rules` as alias of the `dune rules` command. (ocaml/dune#8000, @Alizter) - Fix `%{deps}` to expand properly in `(cat ...)` when containing 2 or more items. (ocaml/dune#8196, @Alizter) - Add `dune show installed-libraries` as an alias of the `dune installed-libraries` command. (ocaml/dune#8135, @Alizter) - Fix the `severity` of error messages sent over RPC which was missing. (ocaml/dune#8193, @Alizter) - Add `dune build --dump-gc-stats FILE` argument to dump garbage collection stats to a named file. (ocaml/dune#8072, @Alizter) - Fix bug with ppx and Reason syntax due to missing dependency in sandboxed action (ocaml/dune#7932, fixes ocaml/dune#7930, @Alizter) - Add `dune describe package-entries` to print all package entries (ocaml/dune#7480, @moyodiallo) - Improve `dune describe external-lib-deps` by adding the internal dependencies for more information. (ocaml/dune#7478, @moyodiallo) - Re-enable background file digests on Windows. The files are now open in a way that prevents race condition around deletion. (ocaml/dune#8262, fixes ocaml/dune#8268, @emillon)

Alizter requested a review from rgrinberg July 13, 2023 01:00

Alizter commented Jul 13, 2023

View reviewed changes

Alizter force-pushed the ps/branch/test__expand___deps__in__cat_ branch from cb18441 to c702049 Compare July 13, 2023 01:11

Alizter mentioned this pull request Jul 13, 2023

changelog creator #8195

Closed

2 tasks

rgrinberg reviewed Jul 13, 2023

View reviewed changes

Alizter force-pushed the ps/branch/test__expand___deps__in__cat_ branch 2 times, most recently from 290f022 to 200fa2d Compare July 13, 2023 10:38

Alizter force-pushed the ps/branch/test__expand___deps__in__cat_ branch 2 times, most recently from 4f2ae44 to 0ccf8cc Compare July 17, 2023 18:24

Alizter requested review from emillon and rgrinberg July 17, 2023 18:24

Alizter mentioned this pull request Jul 18, 2023

feature: case statements #8229

Closed

5 tasks

test: expand %{deps} in (cat)

23499fe

Signed-off-by: Ali Caglayan <alizter@gmail.com>

Alizter force-pushed the ps/branch/test__expand___deps__in__cat_ branch from b587f3c to bcefd80 Compare July 24, 2023 11:25

fix: expand %{deps} in (cat) properly

eb5659b

Signed-off-by: Ali Caglayan <alizter@gmail.com>

rgrinberg force-pushed the ps/branch/test__expand___deps__in__cat_ branch from bcefd80 to eb5659b Compare July 25, 2023 12:46

rgrinberg added this to the 3.10.0 milestone Jul 25, 2023

rgrinberg merged commit 5e2e7ec into ocaml:main Jul 25, 2023

Alizter deleted the ps/branch/test__expand___deps__in__cat_ branch July 25, 2023 14:23

emillon mentioned this pull request Jul 28, 2023

[new release] dune (15 packages) (3.10.0~alpha1) ocaml/opam-repository#24170

Closed

emillon mentioned this pull request Jul 31, 2023

[new release] dune (15 packages) (3.10.0) ocaml/opam-repository#24185

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: expand %{deps} in (cat)#8196

test: expand %{deps} in (cat)#8196
rgrinberg merged 2 commits intoocaml:mainfrom
Alizter:ps/branch/testexpand_depsincat_

Alizter commented Jul 13, 2023 •

edited

Loading

Uh oh!

Alizter Jul 13, 2023

Uh oh!

Alizter Jul 13, 2023

Uh oh!

ocaml-benchmarks bot commented Jul 13, 2023

Uh oh!

rgrinberg Jul 13, 2023

Uh oh!

Alizter Jul 13, 2023

Uh oh!

ocaml-benchmarks bot commented Jul 13, 2023

Uh oh!

Alizter commented Jul 14, 2023

Uh oh!

Alizter commented Jul 17, 2023

Uh oh!

Alizter commented Jul 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -68,6 +68,9 @@ module Action_expander : sig
		(* Evaluate a path in a position of dependency, such as in [(cat <dep>)] *)

Conversation

Alizter commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alizter Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

Alizter Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

ocaml-benchmarks bot commented Jul 13, 2023

Benchmark: default

Test: dune monorepo benchmarks

Uh oh!

rgrinberg Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

Alizter Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

ocaml-benchmarks bot commented Jul 13, 2023

Benchmark: default

Test: dune monorepo benchmarks

Uh oh!

Alizter commented Jul 14, 2023

Uh oh!

Alizter commented Jul 17, 2023

Uh oh!

Alizter commented Jul 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Alizter commented Jul 13, 2023 •

edited

Loading