manual, code example preprocessor : full conversion to compiler-libs#1863
manual, code example preprocessor : full conversion to compiler-libs#1863Octachron merged 11 commits intoocaml:trunkfrom
Conversation
|
At the moment, the commits in this PR have "octachron" as an author.
You may want to use your real name as in e.g. 349db3d.
In the declaration of the `output` type, the use of spaces around the
`:`that separates a filed name from is type does not look consistent to
me. More precisely, I do not understand why all the fileds except the
last one do't have any space while the last one has one space after the
colon. I would personnally add spaces befor and after the colon, for all
fields.
Same goes for `type t = { kind:kind; start:int; stop:int}` and the
`Intersection` exception.
Apart from that, I fear my uninformed eyes can't see anything deep to
say about the code so this should definitely be reviewed by a more
knowledgeable fellow developper.
One question I have, though: although I very much like the idea behind
this PR from a performance point of view, I am wondering whether we are
not going to loose something by using `compilerlibs` instead of forking
an external process. In term of simulating exactly what would happen
"In real life", I mean. I am playing the role of the devil's
advocate here, but are we convinced that we are not introducing a subtle
difference in behaviour that could become a problem later? Or, if
somethiing changes in the way the `ocaml` program works, I assume we
will have to make sure the change is reflected here (suppose there is
something more to initialize, or so). Any comment on this?
|
c5bbe06 to
0cb0883
Compare
|
I have fixed the author name and style issues. Concerning performance, I did not really have performance in mind while writing the PR since the preprocessing phase is fast compared to hevea or latex. It is true that using compiler-libs might introduce a stronger coupling with the toplevel inialization code, but it is also free us from parsing back the toplevel output to identify errors, warnings or locations. Moreover, I am not sure how problematic hypothetical minute differences between |
|
OK, many thanks for the clarification. I just wanted to make sure but I
think I am very fine with your solution.
So far I didn't approve the PR because I don't dare, but if you need me
to and nobody else can do it just ping me.
|
There was a problem hiding this comment.
The change looks reasonable to me, and I'm in favor of merging. (I think it's worth giving more leeway to less-critical parts of the compiler distribution where we have the opportunity to roll-back if something turns out to be not-perfect.)
Two comments:
-
What you are doing, namely the awkward plumbing to reliably capture stdout/stderr, warnings and errors, is exactly what @diml had to do to start utop, and in general is logic that people reinvent in many different places. Long-term, it would be nicer to have a more robust approach, where the various parts of compiler-libs involved make it easy to virtualize these things, so that consumers don't have to jump through those hoops. No need to work on that for this PR, but if you have some ideas about what interface would have been convenient to you, maybe it is something to think about for later.
-
I agree with @shindere's concern that the new code is intertwined tightly enough with the toplevel workings (and Location error formatters, etc.) that it may require to be changed from time to time -- it is a source of fragility. I think that this is fine as long as we make sure that it breaks sooner rather than later: people changing the compiler in way that break this should notice, and fix it along with the rest of the fixes, instead of having the breakage go unnoticed until the next good soul tries to build the manual. So I would suggest having some tests for this tool in the testsuite (checking that the various redirects work as intended). I don't think we have tests for anything in
manual/(pregendoesn't count); maybe it would be simpler to movecaml_tex2totoolsto test it, in which case I would suggest to move it indeed.
manual/tools/caml_tex2.ml
Outdated
| if n = size then | ||
| (Buffer.add_bytes buffer b; loop max_retry) | ||
| else | ||
| Buffer.add_bytes buffer (Bytes.sub b 0 n) |
There was a problem hiding this comment.
This could be simpler with an un-conditional Buffer.add_subbytes buffer b 0 n.
| if newline then Printf.fprintf out "\n" | ||
| Format.fprintf out "%s%s" camlbegin s; | ||
| List.iter (Format.fprintf out "{%s}") args; | ||
| if newline then Format.fprintf out "\n" |
There was a problem hiding this comment.
I wondered for a moment whether newline should be interpreted as @\n or @., but to by honest I don't really understand the differences here and I think that the current code is probably fine -- as long as it works...
There was a problem hiding this comment.
The printing is basically bypassing the Format's layout engine since we are exactly mirroring the input, thus there is no pratical difference now. ( I made the switch to format to avoid mixing Format and Printf since the compiler's printing function were using Format).
|
I agree that adding tests for caml_tex2 is a good ideat at this point, it would also help for the ellipsis annotations/extensions. I am not sure where the tests should be located between |
|
Florian Angeletti (2018/06/29 11:26 -0700):
I agree that adding tests for caml_tex2 is a good ideat at this point,
it would also help for the ellipsis annotations/extensions. I am not
sure where the tests should be located between `testsuite` or
`manual/test`. Do you have an opinion on this matter @shindere ?
Well not a strong one at least!
If the idea is to have this tests run by ocamltest then perhaps putting
them under `testsuite/tests` iis better just because the existing
infrastructure will deal with them for free but I don't know whether
such an argument is strong enough.
|
Seems good enough to me. |
|
I have moved |
gasche
left a comment
There was a problem hiding this comment.
I think that the new state is nice, and I would be happy to merge quickly -- maybe leaving a day to have minor questions answered.
| (* TEST | ||
| reference="${test_source_directory}/redirections.reference" | ||
| output="redirections.output" | ||
| script = "${ocamlrun} ${ocamlsrcdir}/tools/caml-tex -repo-root ${ocamlsrcdir} ${test_source_directory}/${test_file} -o ${output}" |
There was a problem hiding this comment.
check-typo complains about that line being too long, can you use \ to break it after ${ocamlsrcdir} for example?
| caml-tex: $(CAMLTEX) | ||
| $(CAMLC) $(LINKFLAGS) -I .. -linkall -o $@ $(CAMLTEX) | ||
|
|
||
| opt.opt:caml-tex |
There was a problem hiding this comment.
Why is this target named opt.opt? I would expect maybe all:, but not opt.opt here.
There was a problem hiding this comment.
This is due to conflicting dependencies: caml-tex depends on Str and Unix while the otherlibraries target depends on the ocamltools target.
There was a problem hiding this comment.
Could you maybe have a comment to explain this tricky subtlety?
There was a problem hiding this comment.
Good point, I added a comment.
|
I would prefer to fix the build on AppVeyor first. |
71057cf to
339273a
Compare
Yes, and also @xavierleroy who apparently wrote the first Perl implementation ( https://github.com/FBoisson/Camllight/tree/master/contrib/caml-tex ) -- in particular, this correctly attributes part of the copyright to INRIA. I have vague memories of also seeing Roberto Di Cosmo's name attached to the tools as well (maybe for caml-tex2, as a second version of caml-tex?), maybe Xavier or Jacques can correct the record, but I feel good enough about a first copyright header with just Xavier, Jacques and you to merge the PR. |
339273a to
4a21eec
Compare
|
I have added a basic copyright header and a Change entry. |
|
This looks good to me. Would you prefer to rebase to have a history, or should I just squash when the CI passes? |
|
Squashing is fine with me. |
|
This breaks Inria's CI because tools/caml-tex is not available when the |
|
Sorry, I think that the best immediate fix here is to skip the test when the native compiler is disabled. |
|
Florian Angeletti (2018/07/25 02:06 -0700):
Sorry, I think that the best immediate fix here is to skip the test
when the native compiler is disabled.
Yeah, that seems reasonable. Shall I do the commit?
|
|
If you have the time, gladly. |
|
Florian Angeletti (2018/07/25 02:10 -0700):
If you have the time, gladly.
Sure. I have a working fix. About to commit/push.
|
…s disabled This is a follow-up to GPR #1863. Without this commit, the test in tests/tool-caml-tex/redirections.ml fails when the native compiler is disabled. This commit thus makes sure the native compiler is enabled before proceeding.
|
Commit 26652b8 should fix the problem.
|
|
Thanks! And sorry again for the breakage. |
|
Florian Angeletti (2018/07/25 02:22 -0700):
Thanks! And sorry again for the breakage.
No problem, that's what we have the CI for. ;-)
|
|
I'm sorry and ashamed: I had to push
4d99eb1 because I didn't realise the
reference file would need an update, too.
I hope everything will be fine, now.
|
This is PR is essentially #1785 without the ellipses as extension nodes change: it replaces the uses of a separated toplevel process inside
manual/tools/caml_tex2.mlby a direct use ofToploop's api from the compiler-libs.The main benefits of this change is that it enables a more precise control of the data flux from the toplevel: compiler error, compiler warnings, locations to underline, and the example stdout and stderr are now collected separately.
As a direct consequence of this change, all error and warnings related locations are now underlined, not only the first one.
Similarly, code examples now always print warnings and errors message even when the toplevel output is omitted, as wished by @gasche .