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.

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, code example preprocessor : full conversion to compiler-libs #1863

Merged
merged 11 commits into from Jul 25, 2018

Conversation

Projects
None yet
4 participants
@Octachron
Copy link
Contributor

commented Jun 26, 2018

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.ml by a direct use of Toploop '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 .

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

@Octachron Octachron force-pushed the Octachron:manual_camltex2_toploop branch from c5bbe06 to 0cb0883 Jun 27, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

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 ocaml, the manual code example preprocessor and utop would be.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

@gasche
Copy link
Member

left a 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:

  1. 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.

  2. 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/ (pregen doesn't count); maybe it would be simpler to move caml_tex2 to tools to test it, in which case I would suggest to move it indeed.

if n = size then
(Buffer.add_bytes buffer b; loop max_retry)
else
Buffer.add_bytes buffer (Bytes.sub b 0 n)

This comment has been minimized.

Copy link
@gasche

gasche Jun 29, 2018

Member

This could be simpler with an un-conditional Buffer.add_subbytes buffer b 0 n.

This comment has been minimized.

Copy link
@Octachron

Octachron Jul 19, 2018

Author Contributor

Fixed.

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"

This comment has been minimized.

Copy link
@gasche

gasche Jun 29, 2018

Member

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...

This comment has been minimized.

Copy link
@Octachron

Octachron Jun 29, 2018

Author Contributor

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).

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

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 ?

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

but I don't know whether such an argument is strong enough.

Seems good enough to me.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

I have moved caml_tex2 from manual/tools/caml_tex2 to tools/caml_tex and added a new test in testsuite/tests/tool-caml-tex .

@gasche

gasche approved these changes Jul 20, 2018

Copy link
Member

left a 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}"

This comment has been minimized.

Copy link
@gasche

gasche Jul 20, 2018

Member

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

This comment has been minimized.

Copy link
@gasche

gasche Jul 20, 2018

Member

Why is this target named opt.opt? I would expect maybe all:, but not opt.opt here.

This comment has been minimized.

Copy link
@Octachron

Octachron Jul 20, 2018

Author Contributor

This is due to conflicting dependencies: caml-tex depends on Str and Unix while the otherlibraries target depends on the ocamltools target.

This comment has been minimized.

Copy link
@gasche

gasche Jul 20, 2018

Member

Could you maybe have a comment to explain this tricky subtlety?

This comment has been minimized.

Copy link
@Octachron

Octachron Jul 21, 2018

Author Contributor

Good point, I added a comment.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2018

I would prefer to fix the build on AppVeyor first.

@Octachron Octachron force-pushed the Octachron:manual_camltex2_toploop branch from 71057cf to 339273a Jul 21, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2018

I have fixed the windows build by slightly reworking the redirection. Finally, check-typo is asking for a copyright header, should I credit @garrigue (5d6080a) and me?

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 21, 2018

Finally, check-typo is asking for a copyright header, should I credit @garrigue (5d6080a) and me?

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.

@Octachron Octachron force-pushed the Octachron:manual_camltex2_toploop branch from 339273a to 4a21eec Jul 24, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

I have added a basic copyright header and a Change entry.

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 24, 2018

This looks good to me. Would you prefer to rebase to have a history, or should I just squash when the CI passes?

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

Squashing is fine with me.

@Octachron Octachron merged commit 4be6caf into ocaml:trunk Jul 25, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

This breaks Inria's CI because tools/caml-tex is not available when the
native compiler is disabled, leading to the failure of the
`tests/tool-caml-tex/redirections.ml' test.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

Sorry, I think that the best immediate fix here is to skip the test when the native compiler is disabled.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

If you have the time, gladly.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

shindere added a commit that referenced this pull request Jul 25, 2018

Skip test in tests/tool-caml-tex/redirections.ml if native compiler i…
…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.
@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

Thanks! And sorry again for the breakage.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.