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

Manual tools: fail on unexpected errors or warnings within caml_example #693

Merged
merged 10 commits into from Sep 2, 2016

Conversation

Projects
None yet
2 participants
@Octachron
Contributor

Octachron commented Jul 16, 2016

This pull request proposes to modify the manual tools caml_tex2 to make it fails on unexpected errors or warnings within a caml_example environment. This change would make easier to notice new warnings or errors triggering on the code examples within the manual (like the few recent cases of deprecated warning or the quite hidden warning
in advexamples.etex corrected within this PR). Combined with the new use of in-tree,
this change would also make safer the potential use of the caml_example in the Langage extensions chapter.

In more detail, the second commit of this pull request makes the following code

\begin{caml_example}
1.0 * 2 ;;
\end{caml_example}

fail with

Error when evaluating a caml_example environment in filename.etex:
unexpected error.
If an error status was expected, add an [@@expect error] annotation.

As explained by the error message above, if the error or warning was intended, it
is necessary to annotate the example either with an attribute:

\begin{caml_example}
1 + 2. [@@expect error] ;;
let f None = None [@@expect warning 8];;
3 + 4 (*optional ok attribute) [@@expect ok];; 
\end{caml_example}

or with a global latex environment option, e.g.

\begin{caml_example}[error]
1 + 2.;;
3. + 4;;
\end{caml_example}

or for an intended warning

\begin{caml_example}[warning=8]
let f None = ();;
\end{caml_example}

Both annotations can be combined if needed

\begin{caml_example}[error]
1 + 2.;;
3. + 4;;
5 +6 [@@expect ok];;
\end{caml_example}

(The existing source files for the manual has been added the needed annotations.)

Moreover, to make this change useful, the first commit in this PR alters the build process of the manual to make the compilation of the manual fails in case of problems when generating the latex files from the ".etex" source files. Without this modifications, the manual build process had a tendency to "silently" generate empty tex files and then go on with the build process. This should no longer be the case.

Show outdated Hide outdated manual/tools/caml_tex2.ml
let status s = match catch_warning s, catch_error s with
| Some w, _ -> w
| _, Some e -> e

This comment has been minimized.

@gasche

gasche Aug 3, 2016

Member

if you excuse my purely stylistic nitpicking comment, I would prefer the use of None, Some e here to express the ordering using precise patterns instead of just clause ordering. In my opinion this makes the code easier to read, and it also makes it robust to eventual (possibly mistaken) clause reordering.

@gasche

gasche Aug 3, 2016

Member

if you excuse my purely stylistic nitpicking comment, I would prefer the use of None, Some e here to express the ordering using precise patterns instead of just clause ordering. In my opinion this makes the code easier to read, and it also makes it robust to eventual (possibly mistaken) clause reordering.

This comment has been minimized.

@Octachron

Octachron Aug 5, 2016

Contributor

In the current implementation, the two clauses should be commutative since simultaneous multiple warnings and error are not handled.

Should they be handled? it would increase the complexity of the parsing for options and I am not sure how useful it would be. Maybe detecting the situation and raising an error would be worthwile?

Anyway I am perfectly fine with adding a None to choose an arbitrary ordering of these clauses.

@Octachron

Octachron Aug 5, 2016

Contributor

In the current implementation, the two clauses should be commutative since simultaneous multiple warnings and error are not handled.

Should they be handled? it would increase the complexity of the parsing for options and I am not sure how useful it would be. Maybe detecting the situation and raising an error would be worthwile?

Anyway I am perfectly fine with adding a None to choose an arbitrary ordering of these clauses.

This comment has been minimized.

@gasche

gasche Aug 6, 2016

Member

Ah, I read another instance of the same pattern below as implementing a priority order. Don't bother, then.

@gasche

gasche Aug 6, 2016

Member

Ah, I read another instance of the same pattern below as implementing a priority order. Don't bother, then.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 3, 2016

Member

This is excellent work and a much-needed improvement, in particular if we want to get serious about documenting warnings and errors in the manual.

However, I'm worried that the usefulness of this work could be limited by the fact that building the manual is not at all part of the OCaml development workflow, currently. This means that the statu quo of risking the release of inconsistent document is avoided, but what we get instead is Damien fighting for manual sanity around release time, with your occasional much-appreciated contributions catching some of the errors.

I think that making a full manual build part of the common build (or test target) would be too time-consuming, but maybe we could include the production of the .tex files from the .etex files? Or at least have a target for it in the central Makefile, not part of the default developer workflow, but used by our continuous integration builds?

Here would be my proposal:

  • add a target in the central Makefile that performs the minimal amount of manual work to catch these errors
  • measure the time it takes on your machine and report it here
  • include it in .travis-ci.sh before mkdir external-packages
Member

gasche commented Aug 3, 2016

This is excellent work and a much-needed improvement, in particular if we want to get serious about documenting warnings and errors in the manual.

However, I'm worried that the usefulness of this work could be limited by the fact that building the manual is not at all part of the OCaml development workflow, currently. This means that the statu quo of risking the release of inconsistent document is avoided, but what we get instead is Damien fighting for manual sanity around release time, with your occasional much-appreciated contributions catching some of the errors.

I think that making a full manual build part of the common build (or test target) would be too time-consuming, but maybe we could include the production of the .tex files from the .etex files? Or at least have a target for it in the central Makefile, not part of the default developer workflow, but used by our continuous integration builds?

Here would be my proposal:

  • add a target in the central Makefile that performs the minimal amount of manual work to catch these errors
  • measure the time it takes on your machine and report it here
  • include it in .travis-ci.sh before mkdir external-packages
@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Aug 5, 2016

Contributor

Rather than time, I fear that a full manual build would be more constraining in term of external dependencies: both latex and hevea are not exactly essential tools for building compilers.

However, building both the ".tex" files from the ".etex" source files and the ocamldoc documentation for the standard library seems perfectly doable. I have added a makefile target manual-pregen in the central Makefile to build these files. Some quick tests on my machine yield

  • make manual-pregen: 5s ± 0.03s
  • make test: 150s ± 20 s

So the relative increase in time seems limited.

Contributor

Octachron commented Aug 5, 2016

Rather than time, I fear that a full manual build would be more constraining in term of external dependencies: both latex and hevea are not exactly essential tools for building compilers.

However, building both the ".tex" files from the ".etex" source files and the ocamldoc documentation for the standard library seems perfectly doable. I have added a makefile target manual-pregen in the central Makefile to build these files. Some quick tests on my machine yield

  • make manual-pregen: 5s ± 0.03s
  • make test: 150s ± 20 s

So the relative increase in time seems limited.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 8, 2016

Member

I sent Octachron/ocaml#1 as a proposed change on top of the current PR which builds only the .etex file and not the standard library, and reduces build time from 5s to 1.6s. It's not clear to me whether the standard library's documentation build can fail in useful ways that would justify the three extra seconds.

Member

gasche commented Aug 8, 2016

I sent Octachron/ocaml#1 as a proposed change on top of the current PR which builds only the .etex file and not the standard library, and reduces build time from 5s to 1.6s. It's not clear to me whether the standard library's documentation build can fail in useful ways that would justify the three extra seconds.

@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Aug 10, 2016

Contributor

It's not clear to me whether the standard library's documentation build can fail in useful ways that would justify the three extra seconds.

I can think of at least two examples of manual build failure that would have been caught by building the standard library documentation:

  • the problem with infix record in #690
  • the missing escape characters in brace in #419

So it can fail in useful way, but I agree that useful failures are probably very unfrequent and it might make sense to skip these files.

Another point, going further in this direction, we could also only test the generation of the "caml_environment" latex code which would be even lighter. An argument in this direction is that it is the only part of the manual build process that can be broken by unrelated changes in the compiler whereas other part of the build process can only be broken by altering the manual itself or the standard
library documentation.

Contributor

Octachron commented Aug 10, 2016

It's not clear to me whether the standard library's documentation build can fail in useful ways that would justify the three extra seconds.

I can think of at least two examples of manual build failure that would have been caught by building the standard library documentation:

  • the problem with infix record in #690
  • the missing escape characters in brace in #419

So it can fail in useful way, but I agree that useful failures are probably very unfrequent and it might make sense to skip these files.

Another point, going further in this direction, we could also only test the generation of the "caml_environment" latex code which would be even lighter. An argument in this direction is that it is the only part of the manual build process that can be broken by unrelated changes in the compiler whereas other part of the build process can only be broken by altering the manual itself or the standard
library documentation.

Octachron and others added some commits Jul 15, 2016

Manual: fail earlier
This commit modifies the build process of the manual to make it fails in
case of problems during the generation of the tex files from the etex
sources files.
Manual tools: guarded caml_example environment
This commit modifies the manual tool caml_tex2 to catch the status of
the output and raises an error in case of unexpected error of warning
message. Expected errors or warnings must be now marked explicitly.
There are two options to mark these expected errors or warnings:
The first option is to use the new optional parameter of the
`caml_example` environment, e.g.

\begin{caml_example}[warning=3]
  String.capitalize "a word";;
\end{caml_example}

or for an error:

\begin{caml_example}[error]
  1 +. 3.;;
\end{caml_example}

The second option is to use `[@@expect ..]` attribute before `;;` to
override locally the global expected status output:

\begin{caml_example}
  1 + 2 ;;
  1 +. 3. [@@expect error];;
  String.capitalize [@@expect warning 3];;
  3 + 4 ;;
\end{caml_example}

Note that the two options can be combined together, e.g

\begin{caml_example}[error]
  1 +. 3.;; (* an error is expected here *)
  1. +. 3. [@@expect ok] ;;
  1 + 3. ;; (* an error is still expected here *)
\end{caml_example}
manual pre-generation: separate .etex files from stdlib build
We introduce a `pregen-etex` Makefile rule that only build the .etex
files of the manual (whose OCaml examples may be tested and expected
to produce certain outputs), without also building the
standard-library documentation. This is faster than the previous
`pregen` rule as ocamldoc on the standard library was the
bottleneck. On my machine, `pregen-etex` completes in 1.5s-2s, while
`pregen` completes in 6s-7s.
@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Sep 1, 2016

Contributor

@gasche, I have merged your proposed changes, updated the root makefile and travis script.

Moreover, I was quite unhappy with the error reporting; errors were lost in the middle of noise.
So I went ahead and reduced the noisiness of caml_tex2 by adding a verbosity flag and improved the error reporting itself by including the line location, the problematic phrase and output in the error message.

Contributor

Octachron commented Sep 1, 2016

@gasche, I have merged your proposed changes, updated the root makefile and travis script.

Moreover, I was quite unhappy with the error reporting; errors were lost in the middle of noise.
So I went ahead and reduced the noisiness of caml_tex2 by adding a verbosity flag and improved the error reporting itself by including the line location, the problematic phrase and output in the error message.

@gasche gasche merged commit 1b1f5e2 into ocaml:trunk Sep 2, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 2, 2016

Member

Thanks for the additional work! I merged in trunk.

Member

gasche commented Sep 2, 2016

Thanks for the additional work! I merged in trunk.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #693 from Octachron/manual_guarded_caml_example
Manual tools: fail on unexpected errors or warnings within caml_example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment