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

ocamltest: set TERM=dumb when calling OCaml-related programs #1532

Merged
merged 1 commit into from Dec 15, 2017

Conversation

Projects
None yet
3 participants
@shindere
Copy link
Contributor

shindere commented Dec 15, 2017

This is a follow-up to the discussion started on PR #1527

@gasche

gasche approved these changes Dec 15, 2017

Copy link
Member

gasche left a 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.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 15, 2017

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

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 15, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 15, 2017

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.

@shindere shindere force-pushed the shindere:ocamltest-set-TERM branch from 9c12582 to 7ace63f Dec 15, 2017

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 15, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 15, 2017

The new Changes entry is fine, thanks. I'll wait for CI results (mostly out of superstition) and merge.

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 15, 2017

@sliquister

This comment has been minimized.

Copy link
Contributor

sliquister commented Dec 15, 2017

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

@gasche gasche merged commit e2a3585 into ocaml:trunk Dec 15, 2017

1 of 2 checks passed

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

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 19, 2017

@sliquister

This comment has been minimized.

Copy link
Contributor

sliquister commented Dec 20, 2017

By the way, shouldn't this pull request have removed the TERM=dumb around ocamltest in testsuite/Makefile?

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 20, 2017

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.