Trim error messages in watch mode #2120
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Trimming the command line seems good, but "watch mode" doesn't seem like the right criterion to me. It seems to me that the right criterion should be: run by a user or by a CI. I.e., in the CI you want as much info as possible because you often don't have access to the full log, but when run locally you can just look in Googling around, it seems like we can detect whether we are running inside a CI by checking the environment variable There is also a separate issue: for most commands, it is ok to not print the command line because the error message is enough to identify where the problem is. More precisely because the command prints the filename where the error is. This is pretty-much always true for compilers or other build tools, but might not be the case for custom programs users might use through custom |
|
Yeah, I agree that the watch mode heuristic isn't the best. At the end of the day, no heuristic is perfect so I think we should have an explicit option to control this behavior as well. Any suggestions for naming such an option? Meanwhile, I will implement your heuristic for CI.
It seems simpler to just hardcode a few binaries where we can omit the full command. ocamlc, ocamlopt, ocamldep. Why check the output? |
What about
Because other programs we might not know about adopt this convention. If the user makes the effort to write a tool that follows it, it seems nice to provide a nicer output without additional effort on their side. |
rgrinberg
force-pushed
the
improve-message-watch
branch
2 times, most recently
from
a200fef to
a48a95a
Compare
|
@diml please review again, I've implemented the heuristic, the command line option, and the CI detection. |
ghost
reviewed
May 7, 2019
ghost
approved these changes
May 7, 2019
rgrinberg
force-pushed
the
improve-message-watch
branch
from
a48a95a to
01f89c1
Compare
ghost
suggested changes
May 7, 2019
rgrinberg
force-pushed
the
improve-message-watch
branch
from
3228ebb to
1768f05
Compare
|
@diml the option has been renamed and your change has been incorporated |
rgrinberg
force-pushed
the
improve-message-watch
branch
from
1768f05 to
28e266c
Compare
|
I've also disabled this message trimming when |
ghost
approved these changes
May 7, 2019
rgrinberg
force-pushed
the
improve-message-watch
branch
from
b4c0666 to
a3c5646
Compare
Now, dune will not output the full command if the following conditions hold: * The command outputs a location (which is determiend using a "File " prefix) * Dune doesn't think we are running in CI (detected using the CI env var) This behavior may be disabled using --always-show-command-line Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
rgrinberg
force-pushed
the
improve-message-watch
branch
from
a3c5646 to
0b9c460
Compare
rgrinberg
added a commit
to rgrinberg/opam-repository
that referenced
this pull request
May 30, 2019
CHANGES: - Restricted the set of variables available for expansion in the destination filename of `install` stanza to simplify implementation and avoid dependency cycles. (ocaml/dune#2073, @aalekseyev, @diml) - [menhir] call menhir from context root build_dir (ocaml/dune#2067, @ejgallego, review by @diml, @rgrinberg) - [coq] Add `coq.pp` stanza to help with pre-processing of grammar files (ocaml/dune#2054, @ejgallego, review by @rgrinberg) - Add a new more generic form for the *promote* mode: `(promote (until-clean) (into <dir>))` (ocaml/dune#2068, @diml) - Allow to promote only a subset of the targets via `(promote (only <pred>))`. For instance: `(promote (only *.mli))` (ocaml/dune#2068, @diml) - Improve the behavior when a strict subset of the targets of a rule is already in the source tree for projects using the dune language < 1.10 (ocaml/dune#2068, fixes ocaml/dune#2061, @diml) - With lang dune >= 1.10, rules in standard mode are no longer allowed to produce targets that are present in the source tree. This has been a warning for long enough (ocaml/dune#2068, @diml) - Allow %{...} variables in pps flags (ocaml/dune#2076, @mlasson review by @diml and @aalekseyev). - Add a 'cookies' option to ppx_rewriter/deriver flags in library stanzas. This allow to specify cookie requests from variables expanded at each invocation of the preprocessor. (ocaml/dune#2106, @mlasson @diml) - Add more opam metadata and use it to generate `.opam` files. In particular, a `package` field has been added to specify package specific information. (ocaml/dune#2017, ocaml/dune#2091, @avsm, @jonludlam, @rgrinberg) - Clean up the special support for `findlib.dynload`. Before, Dune would simply match on the library name. Now, we only match on the findlib package name when the library doesn't come from Dune. Someone writing a library called `findlib.dynload` with Dune would have to add `(special_builtin_support findlib_dynload)` to trigger the special behavior. (ocaml/dune#2115, @diml) - Install the `future_syntax` preprocessor as `ocaml-syntax-shims.exe` (ocaml/dune#2125, @rgrinberg) - Hide full command on errors and warnings in development and show them in CI. (detected using the `CI` environment variable). Commands for which the invocation might be omitted must output an error prefixed with `File `. Add an `--always-show-command-line` option to disable this behavior and always show the full command. (ocaml/dune#2120, fixes ocaml/dune#1733, @rgrinberg) - In `dune-workspace` files, add the ability to choose the host context and to create duplicates of the default context with different settings. (ocaml/dune#2098, @TheLortex, review by @diml, @rgrinberg and @aalekseyev) - Add support for hg in `dune subst` (ocaml/dune#2135, @diml) - Don't build documentation for implementations of virtual libraries (ocaml/dune#2141, fixes ocaml/dune#2138, @jonludlam) - Fix generation of the `-pp` flag in .merlin (ocaml/dune#2142, @rgrinberg) - Make `dune subst` add a `(version ...)` field to the `dune-project` file (ocaml/dune#2148, @diml) - Add the `%{os_type}` variable, which is a short-hand for `%{ocaml-config:os_type}` (ocaml/dune#1764, @diml) - Allow `enabled_if` fields in `library` stanzas, restricted to the `%{os_type}`, `%{model}`, `%{architecture}`, `%{system}` variables (ocaml/dune#1764, ocaml/dune#2164 @diml, @rgrinberg) - Fix `chdir` on external and source paths. Dune will also fail gracefully if the external or source path does not exist (ocaml/dune#2165, fixes ocaml/dune#2158, @rgrinberg) - Support the `.cc` extension fro C++ sources (ocaml/dune#2195, fixes ocaml/dune#83, @rgrinberg) - Run `ocamlformat` relative to the context root. This improves the locations of errors. (ocaml/dune#2196, fixes ocaml/dune#1370, @rgrinberg) - Fix detection of `README`, `LICENSE`, `CHANGE`, and `HISTORY` files. These would be undetected whenever the project was nested in another workspace. (ocaml/dune#2194, @rgrinberg) - Fix generation of `.merlin` whenever there's more than one stanza with the same ppx preprocessing specification (ocaml/dune#2209 ,fixes ocaml/dune#2206, @rgrinberg) - Fix generation of `.meriln` in the presence of the `copy_files` stanza and preprocessing specifications of other stanazs. (ocaml/dune#2211, fixes ocaml/dune#2206, @rgrinberg)
rgrinberg
added a commit
to rgrinberg/opam-repository
that referenced
this pull request
Jun 4, 2019
CHANGES: - Restricted the set of variables available for expansion in the destination filename of `install` stanza to simplify implementation and avoid dependency cycles. (ocaml/dune#2073, @aalekseyev, @diml) - [menhir] call menhir from context root build_dir (ocaml/dune#2067, @ejgallego, review by @diml, @rgrinberg) - [coq] Add `coq.pp` stanza to help with pre-processing of grammar files (ocaml/dune#2054, @ejgallego, review by @rgrinberg) - Add a new more generic form for the *promote* mode: `(promote (until-clean) (into <dir>))` (ocaml/dune#2068, @diml) - Allow to promote only a subset of the targets via `(promote (only <pred>))`. For instance: `(promote (only *.mli))` (ocaml/dune#2068, @diml) - Improve the behavior when a strict subset of the targets of a rule is already in the source tree for projects using the dune language < 1.10 (ocaml/dune#2068, fixes ocaml/dune#2061, @diml) - With lang dune >= 1.10, rules in standard mode are no longer allowed to produce targets that are present in the source tree. This has been a warning for long enough (ocaml/dune#2068, @diml) - Allow %{...} variables in pps flags (ocaml/dune#2076, @mlasson review by @diml and @aalekseyev). - Add a 'cookies' option to ppx_rewriter/deriver flags in library stanzas. This allow to specify cookie requests from variables expanded at each invocation of the preprocessor. (ocaml/dune#2106, @mlasson @diml) - Add more opam metadata and use it to generate `.opam` files. In particular, a `package` field has been added to specify package specific information. (ocaml/dune#2017, ocaml/dune#2091, @avsm, @jonludlam, @rgrinberg) - Clean up the special support for `findlib.dynload`. Before, Dune would simply match on the library name. Now, we only match on the findlib package name when the library doesn't come from Dune. Someone writing a library called `findlib.dynload` with Dune would have to add `(special_builtin_support findlib_dynload)` to trigger the special behavior. (ocaml/dune#2115, @diml) - Install the `future_syntax` preprocessor as `ocaml-syntax-shims.exe` (ocaml/dune#2125, @rgrinberg) - Hide full command on errors and warnings in development and show them in CI. (detected using the `CI` environment variable). Commands for which the invocation might be omitted must output an error prefixed with `File `. Add an `--always-show-command-line` option to disable this behavior and always show the full command. (ocaml/dune#2120, fixes ocaml/dune#1733, @rgrinberg) - In `dune-workspace` files, add the ability to choose the host context and to create duplicates of the default context with different settings. (ocaml/dune#2098, @TheLortex, review by @diml, @rgrinberg and @aalekseyev) - Add support for hg in `dune subst` (ocaml/dune#2135, @diml) - Don't build documentation for implementations of virtual libraries (ocaml/dune#2141, fixes ocaml/dune#2138, @jonludlam) - Fix generation of the `-pp` flag in .merlin (ocaml/dune#2142, @rgrinberg) - Make `dune subst` add a `(version ...)` field to the `dune-project` file (ocaml/dune#2148, @diml) - Add the `%{os_type}` variable, which is a short-hand for `%{ocaml-config:os_type}` (ocaml/dune#1764, @diml) - Allow `enabled_if` fields in `library` stanzas, restricted to the `%{os_type}`, `%{model}`, `%{architecture}`, `%{system}` variables (ocaml/dune#1764, ocaml/dune#2164 @diml, @rgrinberg) - Fix `chdir` on external and source paths. Dune will also fail gracefully if the external or source path does not exist (ocaml/dune#2165, fixes ocaml/dune#2158, @rgrinberg) - Support the `.cc` extension fro C++ sources (ocaml/dune#2195, fixes ocaml/dune#83, @rgrinberg) - Run `ocamlformat` relative to the context root. This improves the locations of errors. (ocaml/dune#2196, fixes ocaml/dune#1370, @rgrinberg) - Fix detection of `README`, `LICENSE`, `CHANGE`, and `HISTORY` files. These would be undetected whenever the project was nested in another workspace. (ocaml/dune#2194, @rgrinberg) - Fix generation of `.merlin` whenever there's more than one stanza with the same ppx preprocessing specification (ocaml/dune#2209 ,fixes ocaml/dune#2206, @rgrinberg) - Fix generation of `.merlin` in the presence of the `copy_files` stanza and preprocessing specifications of other stanazs. (ocaml/dune#2211, fixes ocaml/dune#2206, @rgrinberg) - Run `refmt` from the context's root directory. This improves error messages in case of syntax errors. (ocaml/dune#2223, @rgrinberg) - In .merlin files, don't pass `-dump-ast` to the `future_syntax` preprocessor. Merlin doesn't seem to like it when binary AST is generated by a `-pp` preprocessor. (ocaml/dune#2236, @aalekseyev) - `dune install` will verify that all files mentioned in all .install files exist before trying to install anything. This prevents partial installation of packages (ocaml/dune#2230, @rgrinberg)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In watch mode, the error messages from the full compilation commands are quite noisy. I think the correct default here is to omit them, at least by default. I'm not sure what's the best way to turn this into a user configurable option as it doesn't really fit into our display hierarchy.