ocamltest: set TERM=dumb when calling OCaml-related programs#1532
ocamltest: set TERM=dumb when calling OCaml-related programs#1532gasche merged 1 commit intoocaml:trunkfrom
Conversation
gasche
left a comment
There was a problem hiding this comment.
Thanks, this looks very simple indeed.
Just to make sure: you decided to pass TERM=dumb to all invocations of OCaml tools (the compiler, the toplevel and expect_test) done by ocamltest. I suppose that this is a convention to follow in the future when adding new kind of actions involving OCaml tools.
|
(The Travis CI fails because there is no Changes entry, despite me adding the tag. We can ignore that, but I also think that it would be reasonable to extend the expect_test-migration Changes entry to credit @sliquister (Valentin Gatien-Baron) and @nojb (Nicolás Ojeda Bär) for their additional input into the problem -- and mention the present GPR number.) I'll let you decide what you want to do and when to merge. |
|
Gabriel Scherer (2017/12/15 01:42 -0800):
gasche approved this pull request.
Thanks, this looks very simple indeed.
Just to make sure: you decided to pass `TERM=dumb` to *all*
invocations of OCaml tools (the compiler, the toplevel and
expect_test) done by ocamltest.
Yes. I did that to be conservative. Not sure which occurrences are
really required though. And if you look at the current test harness, you
can see that TERM=duub is passed around all the time, even by make
clean. That makes me a bit suspiscious that just doing things as they
are done is the right thing to do. But I wanted to move on.
I suppose that this is a convention to follow in the future when
adding new kind of actions involving OCaml tools.
Probably, yes -- just to be conservative.
|
|
I think that conservative is good in this case, because indeed we probably need all TERM-depending behaviors to be controlled before they end up in test outputs. |
9c12582 to
7ace63f
Compare
|
Gabriel Scherer (2017/12/15 09:45 +0000):
(The Travis CI fails because there is no Changes entry, despite me adding the tag. We can ignore that, but I also think that it would be reasonable to extend the expect_test-migration Changes entry to credit @sliquister (Valentin Gatien-Baron) and @nojb (Nicolás Ojeda Bär) for their additional input into the problem -- and mention the present GPR number.)
I'll let you decide what you want to do and when to merge.
I updated the change entry.
Can you please review the update and let me know if you find it okay?
If so, would you mind doing the merge?
|
|
The new Changes entry is fine, thanks. I'll wait for CI results (mostly out of superstition) and merge. |
|
Gabriel Scherer (2017/12/15 02:36 -0800):
The new Changes entry is fine, thanks. I'll wait for CI results
(mostly out of superstition) and merge.
Thanks a lot for taking care!
|
|
Looks good, thanks (I thought at first Action_helpers.run_hook should have been modified, but I don't think so anymore: it looks like it's only involved in preparing tests, not actually running them). I confirm that it works too. The appveyor failure is a timeout, I think (during builds, not during tests, so can't blame this pull request). |
|
sliquister (2017/12/15 13:23 +0000):
Looks good, thanks (I thought at first Action_helpers.run_hook should
have been modified, but I don't think so anymore: it looks like it's
only involved in preparing tests, not actually running them).
No it is involved in running but I understand why you got confused.
Still, I cna confirm that there is no particular need to set TERM=dumb
for hooks, at the moment.
We can alway do that later, if it becomes relevant.
|
|
By the way, shouldn't this pull request have removed the TERM=dumb around ocamltest in testsuite/Makefile? |
|
sliquister (2017/12/19 20:23 -0800):
By the way, shouldn't this pull request have removed the TERM=dumb
around ocamltest in testsuite/Makefile?
It could have, but I didn't want to because at that time there was
PR #1530 affecting the Makefile to try to improve the way the
testsuite was run and I didn't want to have conflicts. But yes,
ultimately the change you propose could be done. Still it does not harm
not to do it, I think.
|
This is a follow-up to the discussion started on PR #1527