Conversation
Changes
Outdated
| (Hugo Heuzard, review by Mark Shinwell) | ||
|
|
||
| - GPR#__ : Add the module Libcompile, which factorize the common | ||
| part in Compile and Optcompile. This also make the pipeline more modular. |
There was a problem hiding this comment.
s/factorize/factorizes/
s/make/makes/
|
I will read this. |
|
I had a look at this using patdiff, which is excellent for checking this sort of patch, and didn't notice anything semantically wrong. However I'm only reasonably confident about that. One thing which might simplify matters further is to make the libcompile code into a functor (I think this will eliminate more duplication). It also avoids the kind of "wrap" notion which always seems slightly dubious. I would maybe think of another name rather than "libcompile" as it's not really a separate library. A couple of stylistic points: I think blank lines in functions should be avoided (if the function is unclear or too long, refactor it in another way) and spaces before record field semicolons are unusual. Also, it would be nice if all of the functions (etc.) in the libcompile interface were documented. This patch would seem to me to be a very good step along the way to making ocamlopt emit bytecode as well as native code. Maybe that's worth trying? In particular you could apply the same kind of simplifications as you have here to |
|
Thanks for the review! Indeed, the diff is quite hard to read (in fact, I had to port this from 4.03, and I ended up partially rewriting it because the diffs were impossible to work with). What about renaming I will look into turning this into a functor, but I'm not so convinced. The pipeline after typing differs significantly between byte and opt. Unless we standardize over a common API, I don't see how we can improve the situation further. I also would prefer to keep the "combinator" approach used here, as I think it's more flexible. I completely agree with your last point about merging the byte and opt driver, this is the end goal. |
|
The next step should probably be to do some large-scale testing of this on OPAM. Maybe talk to the OCaml Labs guys to see if they can assist with setting up a build? |
|
After taking another look, I'm really not sure how to improve it by using a functor. We could define a functor just for the I fixed your stylistic remarks and added some more documentation comments. I asked @avsm and @AltGr. They said they have some testing platform that should be available soon to test compilers, so I'll wait for that. |
|
@dra27 Is the appveyor failure relevant? I can't find what's wrong in the log. If not, can you relaunch the job? |
|
Thanks to @kit-ty-kate, the output of the opam run is here. Apparently, there is an issue with annot files. |
|
@shindere I would like to write a test that tries to compile a file with ocamlc and ocamlopt, and in each case verifies that |
|
Gabriel Radanne (2018/04/12 14:23 +0000):
@shindere I would like to write a test that tries to compile a file
with ocamlc and ocamlopt, and in each case verifies that `.annot`
files have been generated. I have tried to look at similar things in
the testsuite but without success, and I don't really understand the
DSL used in test files. How do I do something like that ?
Yeah I'm sorry aobut the lack of documentation of the DSL and ocamltest
tool in general.
You could do something like this (I'm replying in a mail, I hope GH will
preserve the presentation!):
```
(* TEST
flags = "..." (* flags common to both compilers, if any *)
ocamlc_flags = "..." (*ocamlc-specificflags *)
ocamlopt_flags = "..." (* ocamlopt-specific flags,if any *)
script = "sh ${test_source_directory}/check-annot"
* setup-ocamlc.byte-build-env
** ocamlc.byte
*** script
(* same for ocamlopt.byte *)
*)
```
And create the check-annot script like this:
```
if [ -f ${test_build_directory}/myfile.annot ]; then
exit ${TEST_PASS}
else
exit ${TEST_FAIL}
fi
```
Let me know if you encounter issues. I can definitely review the test.
|
|
@shindere Thanks! That's very helpful. I have something that works in the case the compiler succeed, but I would also like to allow the compiler to fail. How do I do that? |
|
Gabriel Radanne (2018/04/12 15:39 +0000):
@shindere Thanks! That's very helpful. I have something that works in
the case the compiler succeed, but I would also like to allow the
compiler to fail. How do I do that?
Oh sorry, I think I forgot to repsond to this.
ocamlc_byte_exit_status = "2"
|
|
@shindere Perfect. It works nicely now. |
|
Gabriel Radanne (2018/04/12 16:16 +0000):
@shindere Perfect. It works nicely now.
Great!!
|
|
So, the new driver code didn't emit |
I'm investigating this. |
|
It fails because of a subtle interaction between The solution (credit @shindere) is to add a line |
|
@damiendoligez and @shindere Thanks! I added the appropriate line in |
|
Tests are finally passing. @damiendoligez Are you merging as-is? |
|
@Drup: if you don't mind, I would suggest to rebase this on latest trunk
and, more importantly, to cleanup the history a bit. For instance, I
think the two most recent commits could be squashed. Also, ideally, I
think when a commit adds a file, like libcompile.ml, that very same
commit should update the dependencies so that they are as correct as
possible for every commit. This turns out to be important when
bisecting.
|
|
It's already rebased. The multiple commits are for review purposes. I'll squash when it's approved. |
|
Gabriel Radanne (2018/05/29 02:36 -0700):
It's already rebased. The multiple commits are for review purposes.
I'll squash when it's approved.
Okay thanks!
|
|
@Drup I tried to read the rest of this. I couldn't find a separate changeset which fixed the stylistic/documentation issues you said you had attended to. Do you know what's happened there? Some other remarks:
Please ensure this isn't squashed (or similar) before merging as it will be difficult to review. |
|
Mark Shinwell (2018/05/30 07:06 -0700):
Some other remarks:
- `#!/usr/bin/sh` at the top of the annot-checking script seems
unusual/wrong. Also, I recommend always having "`set -eu`" (and
probably "`set -eu -o pipefail`") at the top of every shell script, no
matter how short it may be at the moment.
pipefail is not Posix so wouldn't work with dash whichis used e.g. on
Travis, I think.
|
|
4.07 is released, the world cup is finished. @mshinwell, can I get a review? :) |
|
@mshinwell one down, one to go ? :) |
| val emit_signature : info -> Parsetree.signature -> Typedtree.signature -> unit | ||
| (** [emit_signature info parsetree typedtree] emits the [.cmi] file | ||
| containing the signature. | ||
| *) |
There was a problem hiding this comment.
"the signature" -> "the given signature"
driver/libcompile.mli
Outdated
|
|
||
| val print_if : | ||
| info -> bool ref -> (Format.formatter -> 'a -> unit) -> 'a -> 'a | ||
| (** [print_if i flag fmt x] prints [x] with [fmt] on [i] if [b] is true. *) |
There was a problem hiding this comment.
This should move elsewhere, maybe to Misc.
| @@ -0,0 +1,90 @@ | |||
| (**************************************************************************) | |||
There was a problem hiding this comment.
I still think the name of this module isn't clear. What about Compile_common or similar?
driver/libcompile.mli
Outdated
| val cmx : info -> string | ||
| val obj : info -> string | ||
| val annot : info -> string | ||
| (** Return the filename of some compiler products associated |
There was a problem hiding this comment.
"products" -> "build artifacts"
driver/libcompile.mli
Outdated
| val typecheck_impl : | ||
| info -> Parsetree.structure -> Typedtree.structure * Typedtree.module_coercion | ||
| (** [typecheck_impl info parsetree] typechecks an implementation and returns | ||
| the typedtree of the associated module, along with a coercion again |
|
Can we please run this against OPAM once again after addressing the final review comments and rebasing? |
ebed095 to
99ff164
Compare
|
@mshinwell I addressed all your comments. Rebasing after #1913 was extremely painful and I was more or less forced to squash everything. I do not want to have to rebase this patchset again. Whatever the opam repository has to say, it'll be minor and I can fix it afterwards. I insist that we merge as soon as CI passes. |
|
OK, that's fair enough. Let's get an OPAM run going at that point though, in case there is something we've missed. |
|
ah, and Changes needs the new entry formatting properly. |
Done. |
|
@Drup sorry, I've had my dose of conflicts too. Would you please rename the field info.ppf as info.ppf_dump? |
Done. |
Factorize the part from compile.ml and optcompile.ml.
|
Everything is green! |
|
🎉 |
| ~always:(fun () -> Stypes.dump (Some (outputprefix ^ ".annot"))) | ||
| )) | ||
| let implementation ~backend ~sourcefile ~outputprefix = | ||
| Compmisc.with_ppf_dump ~fileprefix:(outputprefix ^ ".cmo") @@ fun ppf_dump -> |
There was a problem hiding this comment.
This should be .cmx, not .cmo.
| let cmo i = i.outputprefix ^ ".cmo" | ||
| let annot i = i.outputprefix ^ ".annot" | ||
|
|
||
| let init ppf_dump ~init_path ~tool_name ~sourcefile ~outputprefix = |
There was a problem hiding this comment.
~init_path is a strange name. Shouldn't this be called ~native?
There was a problem hiding this comment.
Well, it's the boolean that control if paths should be initialized (which is apparently needed for native). I wouldn't object renaming it.
|
Note: this PR introduced a regression reported in MPR#7918. |
|
I propose a fix for the |
I always felt that the
CompileandOptcompilemodules were a slight violation of the DRY principle, so this patchsetremoves themfixes them.This patchset factorize the common content of
CompileandOptcompileinto a third module,Libcompile. It also make the whole pipeline significantly more modular and, I hope, easier to tinker with.I originally made this patch for the eliom fork, where each file is first typechecked, split and untyped into two parsetrees and then typechecked again and compiled. Given the current pipeline code, this was impossible to write reasonably.
With this patchset, it's just a matter of plumbing: https://github.com/ocsigen/ocaml-eliom/blob/master/driver/compile.ml#L62-L76
I believe this could be useful in a wider context, to easily build custom pipelines or tinker with the existing one. For example to test untypedast, I made a compiler which types, untypes and retypes.
Finally, it also avoids a big chunk of code duplication.
The patches are not exactly atomic. I'll squash when the time arrives.
I'm not exactly sure how to test this. At least it bootstraps.