Pass --warn-error to Odoc with (env ..) #3029

Julow · 2020-01-13T18:25:36Z

Alternative PR to #3018

This PR adds a way to pass --warn-error to Odoc by adding a field to the env stanza:

(env
 (<profile>
  (odoc
   (warnings fatal))))

Warnings can be set to fatal or nonfatal. They are nonfatal by default (for now).

Enabling this option and having an Odoc version that doesn't support it is an error. (no attempt is made to catch this)

ghost · 2020-01-15T13:11:26Z

Enabling this option and having an Odoc version that doesn't support it is an error. (no attempt is made to catch this)

Just update the odoc conflict in the dune-project file. No need to make life complicated :)

src/dune/dune_env.ml

src/dune/env_node.ml

ghost · 2020-01-15T13:37:48Z

test/blackbox-tests/test-cases/odoc/warn-error/run.t

+  (cd _build/default/_doc/_html && %OPAM%/bin/odoc html --warn-error -I ../_odoc/pkg/bar -I ../../sub_env/lib/.bar.objs/byte -o . ../_odoc/pkg/bar/page-index.odoc)
+  (cd _build/default/_doc/_html && %OPAM%/bin/odoc html -I ../_odoc/pkg/bar -I ../../sub_env/lib/.bar.objs/byte -o . ../../sub_env/lib/.bar.objs/byte/bar.odoc)
+  (cd _build/default/_doc/_html && %OPAM%/bin/odoc html --warn-error -I ../_odoc/pkg/foo -I ../../lib/.foo.objs/byte -o . ../_odoc/pkg/foo/page-index.odoc)
+  (cd _build/default/_doc/_html && %OPAM%/bin/odoc html --warn-error -I ../_odoc/pkg/foo -I ../../lib/.foo.objs/byte -o . ../../lib/.foo.objs/byte/foo.odoc)


This output feels a bit too verbose. In order to convince ourselves that the test is passing, we have to go through it to check that --warn-error is correctly passed where it should. What about actually leaving an odoc error and checking that it is reported as an error?

Generating a warning from the odoc html is not easy and Odoc is called from multiple independent paths in the source code and I wasn't convinced each paths had the correct environment.
Would it be better if I do both ?

Well, by testing with an error in the source file, we are really testing the user-facing aspect of this PR, which is the most important.

If it's difficult to do that, we can keep the current way, but the output should be cleaned up more. Basically, a new reader should be able to look at the test and quickly understand what it is doing and whether it is passing or not. Otherwise, if some day this test breaks we might just accept the new output without realising the test is no longer passing.

I changed the tests to show the actual output. I agree it looks a bit better.

ghost · 2020-01-16T11:03:56Z

To fix the CI failure, look for odoc "odoc-simple" in test/blackbox-tests/gen_tests.ml and add a line odoc "odoc/warn-error".

I also prepared a PR to stop dune from printing command lines in tests (#3042). Once it's merged, you can rebase this PR and remove the | sed ... from the tests.

ghost · 2020-01-16T11:06:21Z

To fix the CI failure, look for odoc "odoc-simple" in test/blackbox-tests/gen_tests.ml and add a line odoc "odoc/warn-error".

And then run make test again. Basically this will setup things so that the new test is considered as a test that has external dependencies (odoc in this case).

ghost · 2020-01-16T12:07:46Z

Alright, #3042 has just been merged

Julow · 2020-01-16T13:46:42Z

Just rebased and fixed the tests !

ghost

Looks good!

ghost · 2020-01-16T13:53:02Z

Is the version of odoc that supports --warn-error released yet?

Julow · 2020-01-16T13:53:38Z

It's not yet

ghost · 2020-01-20T13:14:28Z

Ok. Let's way for odoc to be released then, and then we can update the odoc version constraint in Dune and merge this PR.

rgrinberg · 2020-01-23T09:21:39Z

src/dune/env_node.ml

+  | None ->
+    let open Odoc in
+    let default =
+      (* DUNE3: Enable for dev profile in the future *)


This comment should apply only to the dev profile perhaps

rgrinberg · 2020-01-23T09:22:59Z

It's a bit inconsistent that we have semantic fields for odoc but general <tool>_flags fields for menhir for example. Do you think it makes sense to add odoc_flags instead/in addition to this?

rgrinberg · 2020-01-23T09:23:26Z

This also needs an appropriate change list entry and a mention in the documentation.

Julow · 2020-01-23T11:05:12Z

I added a bit of doc.

Adding odoc_flags is not easy because Dune calls several Odoc sub commands (html, compile and support-files). These commands have a few options in common but would all interfere with the options provided by Dune (eg. -I, -o) or need to be passed only on a specific unit (--hidden).
Also, I think other options may be useful to configure later but will require logic in Dune (eg. --syntax could be passed automatically, --theme-uri should add a dependency on a file).

rgrinberg · 2020-01-23T12:12:25Z

I see. Thanks for the explanation.

doc/documentation.rst

emillon · 2020-01-23T14:37:35Z

doc/documentation.rst

+``<optional-fields>`` are:
+
+- ``(warn-error <bool>)`` turns warnings into errors, if set to ``true``.
+  This field is available since Dune 2.2 and requires Odoc 1.4.3.


Also, I'm not too sure about the warn-error key. It mimics the option in odoc and the ocaml compilers, but maybe that part does not have to be exposed as is.

What do you think about (warnings fatal) and (warnings nonfatal) (or maybe (warnings display)), which can be extended more easily to things like (warnings silenced) if we decide to extend the feature later?

Just did that. I agree it looks better.

src/dune/super_context.mli

src/dune_cache/dune_cache.ml

src/stdune/list.ml

emillon

I mostly had a look at the doc, thanks!

Julow · 2020-02-20T16:52:11Z

Sorry for the delay, Odoc 1.5.0 has been release 12 days ago.
I made some changes after the last review.

ghost · 2020-02-20T17:44:03Z

Could you update the odoc constraint in the dune-project file and .travis-ci.sh script, regenerate the opam file and rebase?

Then this should be good to merge.

Julow · 2020-02-21T10:14:37Z

I've update the PR, assuming the next release is 2.4.0

rgrinberg · 2020-02-21T14:18:50Z

Could you rebase again? That should fix the CI failure.

src/stdune/list.ml

rgrinberg · 2020-02-24T09:58:20Z

I've rebased the PR

rgrinberg · 2020-02-24T10:37:55Z

src/dune/odoc.ml

  let lib =
-    let scope = Compilation_context.scope cctx in


I've reverted this change

Allow to set the warnings mode for odoc compilation. Signed-off-by: Rudi Grinberg <me@rgrinberg.com> Signed-off-by: Jules Aguillon <juloo.dsi@gmail.com>

@NathanReb

…lugin, dune-private-libs and dune-glob (2.4.0) CHANGES: - Add `mdx` extension and stanza version 0.1 (ocaml/dune#3094, @NathanReb) - Allow to make Odoc warnings fatal. This is configured from the `(env ...)` stanza. (ocaml/dune#3029, @Julow) - Fix separate compilation of JS when findlib is not installed. (ocaml/dune#3177, @nojb) - Add a `dune describe` command to obtain the topology of a dune workspace, for projects such as ROTOR. (ocaml/dune#3128, @diml) - Add `plugin` linking mode for executables and the `(embed_in_plugin_libraries ...)` field. (ocaml/dune#3141, @nojb) - Add an `%{ext_plugin}` variable (ocaml/dune#3141, @nojb) - Dune will no longer build shared objects for stubs if `supports_shared_libraries` is false (ocaml/dune#3225, fixes ocaml/dune#3222, @rgrinberg) - Fix a memory leak in the file-watching mode (`dune build -w`) (ocaml/dune#3220, @snowleopard and @aalekseyev)

Julow force-pushed the odoc_warn_error_env branch from 674c35b to 8e89a0c Compare January 13, 2020 18:26

Julow mentioned this pull request Jan 13, 2020

Pass --warn-error to Odoc #3018

Closed

ghost suggested changes Jan 15, 2020

View reviewed changes

Julow force-pushed the odoc_warn_error_env branch 2 times, most recently from 869e469 to 5e70dd7 Compare January 15, 2020 16:15

Julow force-pushed the odoc_warn_error_env branch from 5ff7e32 to e74a23c Compare January 16, 2020 13:40

ghost approved these changes Jan 16, 2020

View reviewed changes

rgrinberg reviewed Jan 23, 2020

View reviewed changes

Julow requested review from emillon and nojb as code owners January 23, 2020 11:08

Julow force-pushed the odoc_warn_error_env branch from f6637a4 to 1258af0 Compare January 23, 2020 12:39

emillon reviewed Jan 23, 2020

View reviewed changes

Julow force-pushed the odoc_warn_error_env branch from 65889f5 to 70d7d45 Compare January 23, 2020 16:15

Julow force-pushed the odoc_warn_error_env branch 2 times, most recently from 197cffa to bf43f8c Compare February 21, 2020 09:56

Julow force-pushed the odoc_warn_error_env branch from bf43f8c to 58dd4d8 Compare February 21, 2020 10:07

rgrinberg reviewed Feb 21, 2020

View reviewed changes

src/stdune/list.ml Outdated Show resolved Hide resolved

rgrinberg mentioned this pull request Feb 21, 2020

Remove ad-hoc memoization in [Env_node] #3168

Merged

rgrinberg force-pushed the odoc_warn_error_env branch from 58dd4d8 to 6122d5f Compare February 24, 2020 09:58

rgrinberg reviewed Feb 24, 2020

View reviewed changes

src/dune/odoc.ml Outdated

let lib =

let scope = Compilation_context.scope cctx in

Copy link

Member

rgrinberg Feb 24, 2020

Choose a reason for hiding this comment

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

I've reverted this change

rgrinberg force-pushed the odoc_warn_error_env branch 2 times, most recently from b6a3df7 to 8597ad3 Compare February 24, 2020 14:24

Add Odoc config to the (env ..) stanza

e6b360b

Allow to set the warnings mode for odoc compilation. Signed-off-by: Rudi Grinberg <me@rgrinberg.com> Signed-off-by: Jules Aguillon <juloo.dsi@gmail.com>

rgrinberg force-pushed the odoc_warn_error_env branch from 881c6eb to e6b360b Compare February 24, 2020 14:55

Merge branch 'master' into odoc_warn_error_env

678f800

snowleopard merged commit 0e40917 into ocaml:master Feb 24, 2020

rgrinberg mentioned this pull request Mar 6, 2020

[new release] dune-build-info, dune, dune-configurator, dune-action-plugin, dune-private-libs and dune-glob (2.4.0) ocaml/opam-repository#15956

Merged

Julow mentioned this pull request Apr 7, 2023

How to pass options to odoc ? #2145

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass --warn-error to Odoc with (env ..) #3029

Pass --warn-error to Odoc with (env ..) #3029

Julow commented Jan 13, 2020 •

edited

Loading

ghost commented Jan 15, 2020

ghost Jan 15, 2020

Julow Jan 15, 2020

ghost Jan 15, 2020

Julow Jan 15, 2020

ghost commented Jan 16, 2020

ghost commented Jan 16, 2020

ghost commented Jan 16, 2020

Julow commented Jan 16, 2020

ghost left a comment

ghost commented Jan 16, 2020

Julow commented Jan 16, 2020

ghost commented Jan 20, 2020

rgrinberg Jan 23, 2020

rgrinberg commented Jan 23, 2020

rgrinberg commented Jan 23, 2020

Julow commented Jan 23, 2020

rgrinberg commented Jan 23, 2020

emillon Jan 23, 2020 •

edited

Loading

Julow Jan 23, 2020

emillon left a comment

Julow commented Feb 20, 2020

ghost commented Feb 20, 2020

Julow commented Feb 21, 2020

rgrinberg commented Feb 21, 2020

rgrinberg commented Feb 24, 2020

rgrinberg Feb 24, 2020

Pass --warn-error to Odoc with (env ..) #3029

Pass --warn-error to Odoc with (env ..) #3029

Conversation

Julow commented Jan 13, 2020 • edited Loading

ghost commented Jan 15, 2020

ghost Jan 15, 2020

Choose a reason for hiding this comment

Julow Jan 15, 2020

Choose a reason for hiding this comment

ghost Jan 15, 2020

Choose a reason for hiding this comment

Julow Jan 15, 2020

Choose a reason for hiding this comment

ghost commented Jan 16, 2020

ghost commented Jan 16, 2020

ghost commented Jan 16, 2020

Julow commented Jan 16, 2020

ghost left a comment

Choose a reason for hiding this comment

ghost commented Jan 16, 2020

Julow commented Jan 16, 2020

ghost commented Jan 20, 2020

rgrinberg Jan 23, 2020

Choose a reason for hiding this comment

rgrinberg commented Jan 23, 2020

rgrinberg commented Jan 23, 2020

Julow commented Jan 23, 2020

rgrinberg commented Jan 23, 2020

emillon Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Julow Jan 23, 2020

Choose a reason for hiding this comment

emillon left a comment

Choose a reason for hiding this comment

Julow commented Feb 20, 2020

ghost commented Feb 20, 2020

Julow commented Feb 21, 2020

rgrinberg commented Feb 21, 2020

rgrinberg commented Feb 24, 2020

rgrinberg Feb 24, 2020

Choose a reason for hiding this comment

Julow commented Jan 13, 2020 •

edited

Loading

emillon Jan 23, 2020 •

edited

Loading