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

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

Merged
merged 2 commits into from
Jul 25, 2023

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Jul 13, 2023

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

  • changelog
  • test
  • version guard

@Alizter Alizter requested a review from rgrinberg July 13, 2023 01:00
@@ -68,6 +68,9 @@ module Action_expander : sig
(* Evaluate a path in a position of dependency, such as in [(cat <dep>)] *)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comment is kind of funny now.

@@ -241,6 +244,14 @@ end = struct
Value.to_path_in_build_or_external v
~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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seemed sensible to me.

@Alizter Alizter force-pushed the ps/branch/test__expand___deps__in__cat_ branch from cb18441 to c702049 Compare July 13, 2023 01:11
@Alizter Alizter mentioned this pull request Jul 13, 2023
2 tasks
@ocaml-benchmarks
Copy link

#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%

@@ -304,6 +315,23 @@ end = struct
let fn = Expander.expand_path env sw in
register_dep fn ~f:Option.some env acc

let register_deps x ~f env acc =
Copy link
Member

Choose a reason for hiding this comment

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

Too much copy pasting between this and register_dep

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've left only register_deps now.

@Alizter 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
@ocaml-benchmarks
Copy link

#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
Copy link
Collaborator Author

Alizter commented Jul 14, 2023

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

@Alizter 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
Copy link
Collaborator Author

Alizter commented Jul 17, 2023

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

@Alizter
Copy link
Collaborator Author

Alizter commented Jul 18, 2023

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

@Alizter Alizter mentioned this pull request Jul 18, 2023
5 tasks
Signed-off-by: Ali Caglayan <[email protected]>
@Alizter Alizter force-pushed the ps/branch/test__expand___deps__in__cat_ branch from b587f3c to bcefd80 Compare July 24, 2023 11:25
@rgrinberg rgrinberg force-pushed the ps/branch/test__expand___deps__in__cat_ branch from bcefd80 to eb5659b Compare July 25, 2023 12:46
@rgrinberg rgrinberg added this to the 3.10.0 milestone Jul 25, 2023
@rgrinberg rgrinberg merged commit 5e2e7ec into ocaml:main Jul 25, 2023
@Alizter Alizter deleted the ps/branch/test__expand___deps__in__cat_ branch July 25, 2023 14:23
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 28, 2023
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)
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 31, 2023
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)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants