feature: Silence display in non-user processes #6994

Alizter · 2023-02-04T16:17:25Z

We change the display of various Process.run calls in Dune to be Quiet and non-overridable.

This fixes issues such as those observed in #4066.

bench/bench.ml

snowleopard

If this is a bug fix, can we add a test to prevent future regressions?

As a general comment, as a reviewer I prefer seeing a simple PR that adds some tests first, documenting the current (broken) behaviour, and then a separate PR that fixes the problem and adjusts the tests accordingly. Otherwise, it's hard to know that the fix actually works.

bench/bench.ml

Alizter

@snowleopard I've gone and documented each and every change so that you can better understand what they are doing.

I don't see any point in writing a test for these behaviors as they are not easy to see.

Take for example querying a binary for a version or configuration. There are rare cases where a user might break their setup or the binaries have broken behavior in which Dune will report the failure of the process in --display=short as a failed internal command.

This is not a helpful output for the user, who may be left wondering that they didn't touch that binary to begin with. Therefore we do not show the display coming from running these processes.

bench/bench.ml

bench/micro/dune_bench/scheduler_bench.ml

src/dune_engine/print_diff.ml

src/dune_engine/vcs.ml

test/expect-tests/process_tests.ml

src/dune_rules/cram/cram_exec.ml

src/dune_rules/dune_load.ml

rgrinberg · 2023-02-05T16:41:34Z

How about we add a test just for cram which is the original issue pointed out by gasche? I agree that testing every single instance here isn't helpful

Alizter · 2023-02-05T21:39:47Z

@snowleopard I've added a test in #7003 that demonstrates the behavior I was trying to fix. This PR now clearly improves the situation.

Alizter · 2023-02-06T20:46:25Z

test/blackbox-tests/test-cases/cram/git-diff-fail.t

+We do not observe the leaking of the display of internal processes when running
+dune build. Note that we scrub the actual reported error due to the build tool
+being bogus.
+  $ dune build --root=. --diff-command="exit 1; echo" --display=short @runtest | sed '/^(cd/d' 


@rgrinberg I can't seem to work out how to sanitize the output here. Could you have a go?

@Alizter I pushed something. There are several things going on here:

you're not seeing the cd locally because they rely on inside_ci. There's an example of that in the print-diff.t test. I added that in order to have a suppressable output locally

the cd message is on stderr, so it does not go through the pipe

redirecting stderr to stdout is not enough, because the pipe will "absorb" the error code and [1] is not printed anymore. bash is necessary to fix that unfortunately.

emillon · 2023-02-08T14:47:54Z

This will require a changelog entry. Is the PR ready otherwise?

Alizter · 2023-02-08T14:50:34Z

@emillon The PR is ready, but @snowleopard wanted to have a look before merging. I don't think there is any rush to include this in 3.7 so I am happy to wait a little longer.

snowleopard · 2023-02-11T11:38:51Z

How about we add a test just for cram which is the original issue pointed out by gasche? I agree that testing every single instance here isn't helpful

Yeah, that's exactly what I meant.

@Alizter Thanks for the test and all the explanations! The PR looks good now, though I still feel like adding some of what's been said here into the code may be helpful for future readers. I'll leave this up to you.

rgrinberg · 2023-02-20T03:12:21Z

What's this failure?

--- a/_build/.sandbox/0ede9381ae701c038a06da85470b7f92/default/test/blackbox-tests/test-cases/cram/git-diff-fail.t
+++ b/_build/.sandbox/0ede9381ae701c038a06da85470b7f92/default/test/blackbox-tests/test-cases/cram/git-diff-fail.t.corrected
@@ -20,5 +20,5 @@ dune build. Note that we scrub the actual reported error due to the build tool
 being bogus.
   $ dune build --root=. --diff-command="exit 1; echo" --display=short @runtest | sed '/^(cd/d' 
   File "mytest.t", line 1, characters 0-0:
+  (cd _build/.sandbox/6c537875b13e4fe202d22511ad71f352/default && /bin/sh -c 'exit 1; echo mytest.t mytest.t.corrected')
   Command exited with code 1.
-  [1]

Alizter · 2023-02-20T14:51:02Z

@rgrinberg Etienne fixed the scrubbing, I seemed to have undone the fix when force pushing.

rgrinberg · 2023-02-21T02:47:44Z

Is this PR ready now?

Alizter · 2023-02-21T09:13:56Z

@rgrinberg yes, but I am unsure why the CI is failing. I don't think it is related but I haven't checked in detail.

rgrinberg · 2023-02-21T18:31:04Z

Yes, those failures are unrelated.

rgrinberg · 2023-02-21T18:31:38Z

This needs a CHANGES entry by the way

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

@rgrinberg

CHANGES: - When a rule's action is interrupted, delete any leftover directory targets. This is consistent with how we treat file targets. (@rgrinberg, 7564) - Fix plugin loading with findlib. The functionality was broken in 3.7.0. (ocaml/dune#7556, @anmonteiro) - Introduce a `public_headers` field on libraries. This field is like `install_c_headers`, but it allows to choose the extension and choose the paths for the installed headers. (ocaml/dune#7512, @rgrinberg) - Load the host context `findlib.conf` when cross-compiling (ocaml/dune#7428, fixes ocaml/dune#1701, @rgrinberg, @anmonteiro) - Resolve `ppx_runtime_libraries` in the target context when cross compiling (ocaml/dune#7450, fixes ocaml/dune#2794, @anmonteiro) - Use `$PKG_CONFIG`, when set, to find the `pkg-config` binary (ocaml/dune#7469, fixes ocaml/dune#2572, @anmonteiro) - Preliminary support for Coq compiled intefaces (`.vos` files) enabled via `(mode vos)` in `coq.theory` stanzas. This can be used in combination with `dune coq top` to obtain fast re-building of dependencies (with no checking of proofs) prior to stepping into a file. (ocaml/dune#7406, @rlepigre) - Fix dune crashing on MacOS in watch mode whenever `$PATH` contains `$PWD` (ocaml/dune#7441, fixes ocaml/dune#6907, @rgrinberg) - Fix `dune install` when cross compiling (ocaml/dune#7410, fixes ocaml/dune#6191, @anmonteiro, @rizo) - Find `pps` dependencies in the host context when cross-compiling, (ocaml/dune#7410, fixes ocaml/dune#4156, @anmonteiro) - Dune in watch mode no longer builds concurrent rules in serial (ocaml/dune#7395 @rgrinberg, @jchavarri) - `dune coq top` now correctly respects the project root when called from a subdirectory. However, absolute filenames passed to `dune coq top` are no longer supported (due to being buggy) (ocaml/dune#7357, fixes ocaml/dune#7344, @rlepigre and @Alizter) - Added a `--no-build` option to `dune coq top` for avoiding rebuilds (ocaml/dune#7380, fixes ocaml/dune#7355, @Alizter) - RPC: Ignore SIGPIPE when clients suddenly disconnect (ocaml/dune#7299, ocaml/dune#7319, fixes ocaml/dune#6879, @rgrinberg) - Always clean up the UI on exit. (ocaml/dune#7271, fixes ocaml/dune#7142 @rgrinberg) - Bootstrap: remove reliance on shell. Previously, we'd use the shell to get the number of processors. (ocaml/dune#7274, @rgrinberg) - Bootstrap: correctly detect the number of processors by allowing `nproc` to be looked up in `$PATH` (ocaml/dune#7272, @Alizter) - Speed up file copying on macos by using `clonefile` when available (@rgrinberg, ocaml/dune#7210) - Adds support for loading plugins in toplevels (ocaml/dune#6082, fixes ocaml/dune#6081, @ivg, @richardlford) - Support commands that output 8-bit and 24-bit colors in the terminal (ocaml/dune#7188, @Alizter) - Speed up rule generation for libraries and executables with many modules (ocaml/dune#7187, @jchavarri) - Add `--watch-exclusions` to Dune build options (ocaml/dune#7216, @jonahbeckford) - Do not re-render UI on every frame if the UI doesn't change (ocaml/dune#7186, fix ocaml/dune#7184, @rgrinberg) - Make coq_db creation in scope lazy (@ejgallego, ocaml/dune#7133) - Non-user proccesses such as version control or config checking are now run silently. (ocaml/dune#6994, fixes ocaml/dune#4066, @Alizter) - Add the `--display-separate-messages` flag to separate the error messages produced by commands with a blank line. (ocaml/dune#6823, fixes ocaml/dune#6158, @esope) - Accept the Ordered Set Language for the `modes` field in `library` stanzas (ocaml/dune#6611, @anmonteiro). - dune install now respects --display quiet mode (ocaml/dune#7116, fixes ocaml/dune#4573, fixes ocaml/dune#7106, @Alizter) - Stub shared libraries (dllXXX_stubs.so) in Dune-installed libraries could not be used as dependencies of libraries in the workspace (eg when compiling to bytecode and/or Javascript). This is now fixed. (ocaml/dune#7151, @nojb) - Allow the main module of a library with `(stdlib ...)` to depend on other libraries (ocaml/dune#7154, @anmonteiro). - Bytecode executables built for JSOO are linked with `-noautolink` and no longer depend on the shared stubs of their dependent libraries (ocaml/dune#7156, @nojb) - Added a new user action `(concurrent )` which is like `(progn )` but runs the actions concurrently. (ocaml/dune#6933, @Alizter) - Allow `(stdlib ...)` to be used with `(wrapped false)` in library stanzas (ocaml/dune#7139, @anmonteiro). - Allow parallel execution of inline tests partitions (ocaml/dune#7012, @hhugo) - Support `(link_flags ...)` in `(cinaps ...)` stanza. (ocaml/dune#7423, fixes ocaml/dune#7416, @nojb) - Allow `(package ...)` in any position within `(rule ...)` stanza (ocaml/dune#7445, @Leonidas-from-XIV) - Always include `opam` files in the generated `.install` file. Previously, it would not be included whenever `(generate_opam_files true)` was set and the `.install` file wasn't yet generated. (ocaml/dune#7547, @rgrinberg)

@rleshchinskiy

CHANGES: - Fix string quoting in the json file written by `--trace-file` (ocaml/dune#7773, @rleshchinskiy) - Read `pkg-config` arguments from the `PKG_CONFIG_ARGN` environment variable (ocaml/dune#1492, ocaml/dune#7734, @anmonteiro) - Correctly set `MANPATH` in `dune exec`. Previously, we would use the `bin/` directory of the context. (ocaml/dune#7655, @rgrinberg) - Allow overriding the `ocaml` binary with findlib configuration (ocaml/dune#7648, @rgrinberg) - merlin: ignore instrumentation settings for preprocessing. (ocaml/dune#7606, fixes ocaml/dune#7465, @Alizter) - When a rule's action is interrupted, delete any leftover directory targets. This is consistent with how we treat file targets. (ocaml/dune#7564, @rgrinberg) - Fix plugin loading with findlib. The functionality was broken in 3.7.0. (ocaml/dune#7556, @anmonteiro) - Introduce a `public_headers` field on libraries. This field is like `install_c_headers`, but it allows to choose the extension and choose the paths for the installed headers. (ocaml/dune#7512, @rgrinberg) - Load the host context `findlib.conf` when cross-compiling (ocaml/dune#7428, fixes ocaml/dune#1701, @rgrinberg, @anmonteiro) - Add a `coqdoc_flags` field to the `coq.theory` stanza allowing the user to pass extra arguments to `coqdoc`. (ocaml/dune#7676, fixes ocaml/dune#7954 @Alizter) - Resolve `ppx_runtime_libraries` in the target context when cross compiling (ocaml/dune#7450, fixes ocaml/dune#2794, @anmonteiro) - Use `$PKG_CONFIG`, when set, to find the `pkg-config` binary (ocaml/dune#7469, fixes ocaml/dune#2572, @anmonteiro) - Modules that were declared in `(modules_without_implementation)`, `(private_modules)` or `(virtual_modules)` but not declared in `(modules)` will cause Dune to emit a warning which will become an error in 3.9. (ocaml/dune#7608, fixes ocaml/dune#7026, @Alizter) - Preliminary support for Coq compiled intefaces (`.vos` files) enabled via `(mode vos)` in `coq.theory` stanzas. This can be used in combination with `dune coq top` to obtain fast re-building of dependencies (with no checking of proofs) prior to stepping into a file. (ocaml/dune#7406, @rlepigre) - Fix dune crashing on MacOS in watch mode whenever `$PATH` contains `$PWD` (ocaml/dune#7441, fixes ocaml/dune#6907, @rgrinberg) - Fix `dune install` when cross compiling (ocaml/dune#7410, fixes ocaml/dune#6191, @anmonteiro, @rizo) - Find `pps` dependencies in the host context when cross-compiling, (ocaml/dune#7410, fixes ocaml/dune#4156, @anmonteiro) - Dune in watch mode no longer builds concurrent rules in serial (ocaml/dune#7395 @rgrinberg, @jchavarri) - Dune can now detect Coq theories from outside the workspace. This allows for composition with installed theories (not necessarily installed with Dune). (ocaml/dune#7047, @Alizter, @ejgallego) - `dune coq top` now correctly respects the project root when called from a subdirectory. However, absolute filenames passed to `dune coq top` are no longer supported (due to being buggy) (ocaml/dune#7357, fixes ocaml/dune#7344, @rlepigre and @Alizter) - Added a `--no-build` option to `dune coq top` for avoiding rebuilds (ocaml/dune#7380, fixes ocaml/dune#7355, @Alizter) - RPC: Ignore SIGPIPE when clients suddenly disconnect (ocaml/dune#7299, ocaml/dune#7319, fixes ocaml/dune#6879, @rgrinberg) - Always clean up the UI on exit. (ocaml/dune#7271, fixes ocaml/dune#7142 @rgrinberg) - Bootstrap: remove reliance on shell. Previously, we'd use the shell to get the number of processors. (ocaml/dune#7274, @rgrinberg) - Bootstrap: correctly detect the number of processors by allowing `nproc` to be looked up in `$PATH` (ocaml/dune#7272, @Alizter) - Speed up file copying on macos by using `clonefile` when available (@rgrinberg, ocaml/dune#7210) - Adds support for loading plugins in toplevels (ocaml/dune#6082, fixes ocaml/dune#6081, @ivg, @richardlford) - Support commands that output 8-bit and 24-bit colors in the terminal (ocaml/dune#7188, @Alizter) - Speed up rule generation for libraries and executables with many modules (ocaml/dune#7187, @jchavarri) - Add `--watch-exclusions` to Dune build options (ocaml/dune#7216, @jonahbeckford) - Do not re-render UI on every frame if the UI doesn't change (ocaml/dune#7186, fix ocaml/dune#7184, @rgrinberg) - Make coq_db creation in scope lazy (@ejgallego, ocaml/dune#7133) - Non-user proccesses such as version control or config checking are now run silently. (ocaml/dune#6994, fixes ocaml/dune#4066, @Alizter) - Add the `--display-separate-messages` flag to separate the error messages produced by commands with a blank line. (ocaml/dune#6823, fixes ocaml/dune#6158, @esope) - Accept the Ordered Set Language for the `modes` field in `library` stanzas (ocaml/dune#6611, @anmonteiro). - dune install now respects --display quiet mode (ocaml/dune#7116, fixes ocaml/dune#4573, fixes ocaml/dune#7106, @Alizter) - Stub shared libraries (dllXXX_stubs.so) in Dune-installed libraries could not be used as dependencies of libraries in the workspace (eg when compiling to bytecode and/or Javascript). This is now fixed. (ocaml/dune#7151, @nojb) - Allow the main module of a library with `(stdlib ...)` to depend on other libraries (ocaml/dune#7154, @anmonteiro). - Bytecode executables built for JSOO are linked with `-noautolink` and no longer depend on the shared stubs of their dependent libraries (ocaml/dune#7156, @nojb) - Added a new user action `(concurrent )` which is like `(progn )` but runs the actions concurrently. (ocaml/dune#6933, @Alizter) - Allow `(stdlib ...)` to be used with `(wrapped false)` in library stanzas (ocaml/dune#7139, @anmonteiro). - Allow parallel execution of inline tests partitions (ocaml/dune#7012, @hhugo) - Support `(link_flags ...)` in `(cinaps ...)` stanza. (ocaml/dune#7423, fixes ocaml/dune#7416, @nojb) - Allow `(package ...)` in any position within `(rule ...)` stanza (ocaml/dune#7445, @Leonidas-from-XIV) - Always include `opam` files in the generated `.install` file. Previously, it would not be included whenever `(generate_opam_files true)` was set and the `.install` file wasn't yet generated. (ocaml/dune#7547, @rgrinberg) - Fix regression where Merlin was unable to handle filenames with uppercase letters under Windows. (ocaml/dune#7577, @nojb) - On nix+macos, pass `-f` to the codesign hook to avoid errors when the binary is already signed (ocaml/dune#7183, fixes ocaml/dune#6265, @greedy) - Fix bug where RPC clients built with dune-rpc-lwt would crash when closing their connection to the server (ocaml/dune#7581, @gridbugs) - Introduce mdx stanza 0.4 requiring mdx >= 2.3.0 which updates the default list of files to include `*.mld` files (ocaml/dune#7582, @Leonidas-from-XIV) - Fix RPC server on Windows (used for OCaml-LSP). (ocaml/dune#7666, @nojb) - Coq language versions less 0.8 are deprecated, and will be removed in an upcoming Dune version. All users are required to migrate to `(coq lang 0.8)` which provides the right semantics for theories that have been globally installed, such as those coming from opam (@ejgallego, @Alizter) - Bump minimum version of the dune language for the melange syntax extension from 3.7 to 3.8 (ocaml/dune#7665, @jchavarri)

Alizter added this to the 3.7.0 milestone Feb 4, 2023

Alizter requested review from rgrinberg and snowleopard February 4, 2023 16:17

Alizter mentioned this pull request Feb 4, 2023

minor usability issues with cram tests #4066

Closed

Alizter linked an issue Feb 4, 2023 that may be closed by this pull request

minor usability issues with cram tests #4066

Closed

snowleopard reviewed Feb 5, 2023

View reviewed changes

bench/bench.ml Show resolved Hide resolved

snowleopard requested changes Feb 5, 2023

View reviewed changes

Alizter commented Feb 5, 2023

View reviewed changes

bench/bench.ml Show resolved Hide resolved

Alizter commented Feb 5, 2023

View reviewed changes

Alizter force-pushed the ps/rr/silence_display_in_non_user_processes branch from bc72601 to 732954e Compare February 5, 2023 21:37

Alizter force-pushed the ps/rr/silence_display_in_non_user_processes branch 2 times, most recently from 44aa794 to bc68d34 Compare February 6, 2023 16:37

Alizter commented Feb 6, 2023

View reviewed changes

emillon force-pushed the ps/rr/silence_display_in_non_user_processes branch from c14df0f to 10f46a0 Compare February 8, 2023 14:36

rgrinberg modified the milestones: 3.7.0, 3.8.0 Feb 9, 2023

snowleopard approved these changes Feb 11, 2023

View reviewed changes

Alizter force-pushed the ps/rr/silence_display_in_non_user_processes branch from 10f46a0 to 52de2fa Compare February 19, 2023 17:18

Alizter mentioned this pull request Feb 21, 2023

feature: add terminal ui backend based on NoTTY #6996

Merged

Alizter force-pushed the ps/rr/silence_display_in_non_user_processes branch 2 times, most recently from f730d70 to 1786d6c Compare February 21, 2023 02:16

Silence display in non-user processes

8b0d91b

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

Alizter force-pushed the ps/rr/silence_display_in_non_user_processes branch from 1786d6c to 8b0d91b Compare February 21, 2023 18:40

rgrinberg merged commit 5463cd8 into ocaml:main Feb 21, 2023

Alizter deleted the ps/rr/silence_display_in_non_user_processes branch February 21, 2023 19:45

emillon mentioned this pull request Apr 18, 2023

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

Closed

emillon mentioned this pull request May 23, 2023

[new release] dune (15 packages) (3.8.0) ocaml/opam-repository#23814

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Silence display in non-user processes #6994

feature: Silence display in non-user processes #6994

Alizter commented Feb 4, 2023

snowleopard left a comment •

edited

Loading

Alizter left a comment

rgrinberg commented Feb 5, 2023

Alizter commented Feb 5, 2023

Alizter Feb 6, 2023

emillon Feb 8, 2023

Alizter Feb 8, 2023

emillon commented Feb 8, 2023

Alizter commented Feb 8, 2023

snowleopard commented Feb 11, 2023

rgrinberg commented Feb 20, 2023 •

edited by Alizter

Loading

Alizter commented Feb 20, 2023

rgrinberg commented Feb 21, 2023

Alizter commented Feb 21, 2023

rgrinberg commented Feb 21, 2023

rgrinberg commented Feb 21, 2023

feature: Silence display in non-user processes #6994

feature: Silence display in non-user processes #6994

Conversation

Alizter commented Feb 4, 2023

snowleopard left a comment • edited Loading

Choose a reason for hiding this comment

Alizter left a comment

Choose a reason for hiding this comment

rgrinberg commented Feb 5, 2023

Alizter commented Feb 5, 2023

Alizter Feb 6, 2023

Choose a reason for hiding this comment

emillon Feb 8, 2023

Choose a reason for hiding this comment

Alizter Feb 8, 2023

Choose a reason for hiding this comment

emillon commented Feb 8, 2023

Alizter commented Feb 8, 2023

snowleopard commented Feb 11, 2023

rgrinberg commented Feb 20, 2023 • edited by Alizter Loading

Alizter commented Feb 20, 2023

rgrinberg commented Feb 21, 2023

Alizter commented Feb 21, 2023

rgrinberg commented Feb 21, 2023

rgrinberg commented Feb 21, 2023

snowleopard left a comment •

edited

Loading

rgrinberg commented Feb 20, 2023 •

edited by Alizter

Loading