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.

Sign up for GitHub

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

Jump to bottom

Add a new line at the end of error reports (#6823 #6823

Merged
merged 1 commit into from
Feb 21, 2023
Merged

Add a new line at the end of error reports (#6823 #6823

merged 1 commit into from
Feb 21, 2023

Conversation

esope
Copy link
Collaborator

@esope esope commented Jan 4, 2023 •
edited
Loading

This is an attempt to fix issue #6158

Remark: If a command (say, ocamlopt) returns several messages (such as warnings), a single line is put at the end, since this is considered as a single message.

@rgrinberg
Copy link
Member

Thanks for trying to address this. All the gratuitous newlines are a bit annoying. Can we print the cut only as a separator between errors?

@snowleopard what do you think of this change in principle? We can make it console backend dependent to avoid any inconvenience it might cause inside at jst for you.

@esope
Copy link
Collaborator Author

esope commented Jan 4, 2023

I agree too that some of the added newlines are annoying, especially terminal ones.
I have not found how to implement the "add new lines only between messages" behavior: it seems there is no easy way to detect which message is the "last" one.
A different approach would be to add a new line before a message. Maybe this would feel less annoying.

@rgrinberg
Copy link
Member

The behavior you describe sounds reasonable. In other words "Prepend a newline if we aren't the first message being printed".

@esope
Copy link
Collaborator Author

esope commented Jan 4, 2023

OK, I will try with this approach when I find some free time, then.

@esope
Copy link
Collaborator Author

esope commented Jan 4, 2023

This seems to bring much more reasonable results.
I am not sure that the use of a reference in a function that is passed to Fiber.Mutex.with_lock is correct. What is the best practice that you use in the dune codebase?
I will add a test as soon as possible.

@rgrinberg
Copy link
Member

It's correct. As long as it doesn't cross a bind, there's nothing to worry about.

However, why did you choose to put this change in build_system.ml? I would expect Dune_util.Report_error.report to be the place to do it.

@esope
Copy link
Collaborator Author

esope commented Jan 5, 2023

OK, I moved the change to Dune_util.Report_error.report.

@rgrinberg rgrinberg requested a review from snowleopard January 5, 2023 15:39
snowleopard
snowleopard requested changes Jan 5, 2023
View reviewed changes
Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

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

I think we should either support file-watching builds, or disable this feature during file-watching builds, because the current implementation seems broken.

@rgrinberg
Copy link
Member

@snowleopard sounds good, but do you think we need a flag for this feature? Or are you happy changing the output internally as well.

@snowleopard
Copy link
Collaborator

@snowleopard sounds good, but do you think we need a flag for this feature? Or are you happy changing the output internally as well.

Yes, it seems nice to keep this feature behind a flag, at least for some time, since we are technically breaking Dune's public interface. For example, various tests calling out to Dune might break.

Internally, I think there is a high chance we would want to enable this feature.

@esope
Copy link
Collaborator Author

esope commented Jan 6, 2023 •
edited
Loading

Thanks for the feedback.
A few questions:

  1. What should be the name of this flag? --separate-error-messages?
  2. Should this be controlled by an option in dune-workspace/dune-project too?
  3. In watch mode only, when this flag is on, what do you think about always prepending the error message with a blank line?

@rgrinberg
Copy link
Member

  1. That name sounds fine.
  2. For now I wouldn't worry about this and we can even just add an environment variable to control this. If this setting is good in practice, we're just going to make it the default.
  3. Why does this extra newline help in watch mode?

@esope
Copy link
Collaborator Author

esope commented Jan 7, 2023 •
edited
Loading

Thanks.
About point 3, this would remove the need for keeping an internal state (a bool ref in this case) when in watch mode. This means that there would be no problem anymore related to memoization or rebuilds. Or am I mistaken?

@esope esope force-pushed the issue_6158 branch 3 times, most recently from 7ba1006 to 4a3c95d Compare January 13, 2023 13:00
@esope
Copy link
Collaborator Author

esope commented Jan 13, 2023

OK, I added the flag --separate-error-messages to control the new behavior of Dune_util.Report.report.
The flag is always off when watch mode is enabled.

@esope esope requested a review from snowleopard January 13, 2023 13:58
snowleopard
snowleopard reviewed Jan 16, 2023
View reviewed changes
bin/common.ml Outdated Show resolved Hide resolved
snowleopard
snowleopard reviewed Jan 16, 2023
View reviewed changes
bin/common.ml Outdated Show resolved Hide resolved
snowleopard
snowleopard reviewed Jan 16, 2023
View reviewed changes
Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

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

Thanks, looks good now! I left a couple of suggestions.

It shouldn't be hard to make this flag work for the file-watching mode, I think. Perhaps, Memo.Run.t can be stored alongside the bool ref to decide when that ref gets stale and needs to be reset.

@esope
Copy link
Collaborator Author

esope commented Jan 16, 2023

Thanks for the feedback, @snowleopard . I implemented your suggested fixes.

snowleopard
snowleopard reviewed Jan 17, 2023
View reviewed changes
Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

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

Thanks! I'll leave it to @rgrinberg to approve and merge. (As far as I'm concerned there are no blockers any more.)

@gasche
Copy link
Member

gasche commented Feb 10, 2023

Two comments from an outsider:

  • Nitpick: the name error is less appropriate at this abstraction level, maybe separate_error_messages => separate_messages and first_error => first_message?
  • Logic: maybe you could set first_message := true in the reset and reset_flush_history functions, and this code would magically work for the watch mode as well? (I'm assuming the watch mode is implemented by calling reset to clear the terminal). Then you could simplify your code quite a bit.

@snowleopard
Copy link
Collaborator

I quite like both of @gasche's suggestions!

@esope
Copy link
Collaborator Author

esope commented Feb 11, 2023

Very good remarks, indeed! Thanks!
Both are now implemented.

@esope
Copy link
Collaborator Author

esope commented Feb 16, 2023

I can hear no more requests for changes, does this mean everybody is happy about this implementation?
If this is a yes, this should probably be merged.

I will be happy to add an environment variable or an option in dune-project in a subsequent PR to control this setting more easily, if you think this is useful.

@esope
Copy link
Collaborator Author

esope commented Feb 19, 2023 •
edited
Loading

@snowleopard I updated the (now quite old) changes you requested, as well as @gasche 's.
I would very much like, that we make progress on this PR.

snowleopard
snowleopard reviewed Feb 21, 2023
View reviewed changes
CHANGES.md Outdated Show resolved Hide resolved
snowleopard
snowleopard approved these changes Feb 21, 2023
View reviewed changes
Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

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

Thanks! It's great that this feature is now compatible with the file-watching mode.

I wonder if --separate-error-messages is still the right name for the flag though. It now affects all messages, not just error messages, so perhaps it should be called --separate-messages? I don't have a strong opinion about this.

@rgrinberg
Copy link
Member

Thanks for taking a look. I'll rebase and merge.

It now affects all messages, not just error messages, so perhaps it should be called --separate-messages? I don't have a strong opinion about this.

I'll call it --display-separate-messages. Without the "display', it's not clear which part of dune this is affecting exactly.

@rgrinberg rgrinberg force-pushed the issue_6158 branch 2 times, most recently from 05f4103 to e4d5bdc Compare February 21, 2023 06:25
@esope @rgrinberg
feature: option to print a new line between error messages
b1612e5 
Whenever --display-separate-messages is passed, dune will separate error
messages by blank lines

Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
@esope
Copy link
Collaborator Author

esope commented Feb 21, 2023

Thanks. I will rename the flag as soon as possible.

@rgrinberg
Copy link
Member

I already did the rename

@rgrinberg rgrinberg changed the title Add a new line at the end of error reports Add a new line at the end of error reports (#6823 Feb 21, 2023
@rgrinberg rgrinberg merged commit 2f9cd73 into ocaml:main Feb 21, 2023
@gasche gasche mentioned this pull request Feb 21, 2023
Merged
@esope
Copy link
Collaborator Author

esope commented Feb 22, 2023

Great! Thanks!
I am eager to hear the feedback from users of JaneStreet and Tarides.
@gasche : should I/you try to implement the "blank line separation" on the OCaml compiler side?

@esope esope deleted the issue_6158 branch February 22, 2023 12:46
@gasche
Copy link
Member

gasche commented Feb 22, 2023

I proposed something like this yesterday in ocaml/ocaml#12024

This is a bit harder to do on the compiler side because we don't have a generic abstraction of "message", we have the "reports" (errors and warnings) that are easy to handle, and a ton of other ad-hoc printing calls that would each need proper logic to be handled as messages. The PR above is a first iteration that does a good enough job, I hope.

@gasche
Copy link
Member

gasche commented Feb 24, 2023

The compiler PR has been merged thanks to a careful review of @Octachron. Blank lines for everyone!

@snowleopard
Copy link
Collaborator

Amazing, thanks @gasche! Great to see such tight collaboration between compiler and build system devs.

@Alizter Alizter mentioned this pull request Mar 28, 2023
Closed
moyodiallo pushed a commit to moyodiallo/dune that referenced this pull request Apr 3, 2023
@esope @moyodiallo
Add a new line at the end of error reports (ocaml#6823
07d3584 
Whenever --display-separate-messages is passed, dune will separate error
messages by blank lines

Signed-off-by: Benoît Montagu <benoit.montagu@inria.fr>
@emillon emillon mentioned this pull request Apr 18, 2023
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Apr 18, 2023
@emillon
[new release] dune (15 packages) (3.8.0~alpha1)
8f04d31 
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)
@emillon emillon mentioned this pull request May 23, 2023
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request May 23, 2023
@emillon
[new release] dune (15 packages) (3.8.0)
df1ab22 
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alizter Alizter Alizter approved these changes

@snowleopard snowleopard snowleopard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.
6 participants
@esope @rgrinberg @snowleopard @Alizter @gasche @emillon