Skip to content

Various test-suite fixes for Windows#366

Merged
mshinwell merged 10 commits intoocaml:trunkfrom
dra27:testsuite-windows
Jan 26, 2016
Merged

Various test-suite fixes for Windows#366
mshinwell merged 10 commits intoocaml:trunkfrom
dra27:testsuite-windows

Conversation

@dra27
Copy link
Member

@dra27 dra27 commented Dec 22, 2015

These commits deal with various issues introduced in d8b7a4f causing the line endings of some files in the test suite to be altered.

These patches restore full operation to the test suite tested, so far, using Visual Studio 2005 Professional SP1 x86 & x64.

@dra27
Copy link
Member Author

dra27 commented Dec 22, 2015

There is one issue which I don't know the implications of:

Running tests from 'tests/runtime-errors' ...
make compile
make run
/bin/sh: line 0: ulimit: stack size: cannot modify limit: Invalid argument
 ... testing 'stackoverflow.bytecode': => passed
 ... testing 'syserror.bytecode': => passed
 ... testing 'syserror.native': => passed

As far as I can tell, you cannot alter the stack size in Cygwin (at least not with ulimit). Does that have implications for this test? If not, shall I put a test using uname for Cygwin to short-circuit the command just to remove the erroneous looking output?

@gasche
Copy link
Member

gasche commented Dec 22, 2015

The commits seem fine so far -- I would have merged them without your warning.

The ulimit call here is designed to make sure that there is a limit to the stack. The implemented test should succeed no matter what this limit is (so having the default limit is fine if ulimit does not exist), but it would fail if there was no limit with a dynamically-growing stack (the program would loop for a very long time, and possibly OOM instead of having the expected behavior). If you know for sure that Cygwin never uses a dynamically-growing call stack, then just disabling ulimit is fine, otherwise it should be replaced by something of similar effect.

@dra27
Copy link
Member Author

dra27 commented Dec 22, 2015

Hmm - Cygwin certainly has a default limit, but I don't know how it's arrived at. For example, the machine I'm on (fairly old Cygwin) has ulimit -s = 2025 but in a VM on the same machine with a newer Cygwin it has ulimit -s = 2033. Does ulimit give a known value if the stack is unlimited which could be tested for? Equally, if a Cygwin comes along with an unlimited stack, we're going to notice that suddenly the testsuite hangs, so maybe disabling the ulimit for Cygwin with just a comment in the Makefile as to why is enough?

@gasche
Copy link
Member

gasche commented Dec 22, 2015

On Linux, you can set ulimit -s unlimited, and if the stack size is unlimited then ulimit -s will print unlimited. I would assume that Cygwin would also return unlimited if the stack size was unlimited. This mean you could change the Makefile to only skip the test if ulimit -s prints unlimited, either just on Cygwin or on all systems.

@dra27
Copy link
Member Author

dra27 commented Dec 22, 2015

75a000d added to eliminate the ulimit call on Cygwin. Also a minor bit of tidying in make -f Makefile.nt [dist]clean

@dra27 dra27 force-pushed the testsuite-windows branch from 607c9ce to 6108518 Compare December 23, 2015 11:24
dra27 referenced this pull request in dra27/ocaml Dec 23, 2015
Stray `\r` characters included in the attribute when parsed on Windows.
Fixed by specifying that deprecated_module.mli must be checked out with LF
endings.
@dra27 dra27 force-pushed the testsuite-windows branch from 6108518 to 9cf9c88 Compare December 23, 2015 12:15
@dra27
Copy link
Member Author

dra27 commented Dec 23, 2015

Right - this is all ready-to-go as far as I'm concerned. The deprecated_module_use test fails at the moment but will start passing when #370 is merged (I've checked).
Would you like me to rebase before merging?

@gasche
Copy link
Member

gasche commented Dec 23, 2015

The commits look reasonably atomic. I'll wait for #370 to be merged (it's still waiting on some further changes if I understand correctly) and merge this one then.

@dra27 dra27 force-pushed the testsuite-windows branch from cec3811 to 1d60930 Compare December 30, 2015 22:24
dra27 referenced this pull request Dec 30, 2015
Warning for missing .cmx files
@dra27 dra27 force-pushed the testsuite-windows branch 2 times, most recently from da08390 to 2e32cca Compare December 31, 2015 09:18
@dra27
Copy link
Member Author

dra27 commented Dec 31, 2015

Rebased in order to include a small fix for #319

@dra27 dra27 force-pushed the testsuite-windows branch from 2e32cca to e243bf8 Compare January 2, 2016 08:23
@dra27 dra27 force-pushed the testsuite-windows branch 2 times, most recently from 5a3c2af to 34726d4 Compare January 16, 2016 16:07
@dra27
Copy link
Member Author

dra27 commented Jan 16, 2016

OK - hopefully that really is the last change appropriate to this PR! A small revision to the third commit (1cc554f) to include .runner scripts. The last commit re-enables a disabled test (lib-threads/sockets.ml) and adds support for a lib-threads/signal.ml - Windows may not have signals, but it does have a reliable way to send CTRL+C to a console program.
CANKILL seems very confusing - especially as Cygwin does have the ability to kill native Windows processes (though not in a Posix way) - given that the only remaining use of this is lib-threads/signal2.ml, I changed the way .precheck works to pass on the TOOLCHAIN variable and explicitly disable the test when running on msvc or mingw (TOOLCHAIN is the same value between 32 and 64-bit)

@damiendoligez
Copy link
Member

About copyright headers: we recently decided that they are not needed in testsuite/test so please do not add any.

@damiendoligez
Copy link
Member

Could you pull all the .gitignore and .gitattributes files up to the top directory? Our current doctrine is that it's better to have all the information centralized in one file.

@damiendoligez damiendoligez added this to the 4.03.0 milestone Jan 22, 2016
@damiendoligez
Copy link
Member

OK for merge after the .gitignore and .gitattributes contents are moved.

dra27 added 3 commits January 25, 2016 15:07
XML sample document behaves differently when checked out on Windows where
the source line endings in the test document become `\r\n`.

Alternative would have been to specify that t01.ml needs LF endings in
.gitattributes, but explicitly including the \n and \t in the OCaml string
seems less brittle.
Testing the parsetree fails on Windows because the .ml files have `\r\n`
endings. For these tests, simplest simply to ensure that they are checked
out using LF endings, even on Windows.
.precheck, .runner and .checker files are part of scripts and need to
have LF line-endings.
dra27 added 7 commits January 25, 2016 15:08
Cygwin doesn't allow the stack limit (uname -s) to be changed, though it
can be queried. Alter the test so that the stack limit is only changed if
it is either unlimited or very large (and skip the tests if ulimit returns
an error)
Warnings tests didn't check `$(BYTECODE_ONLY)`
Various testsuite Makefiles displaying commands, which fog up the log
files.
The `CANKILL` testsuite variable is eliminated in favour of testing for
`TOOLCHAIN`.

tests/lib-threads/signal.ml can be executed under native Windows by
means of a wrapper program to send CTRL+C.

tests/lib-threads/signal2.ml is not possible under native Windows
because Thread.sigmask is not implemented, so the precheck is updated to
reflect this, rather than the lack of kill -INT.

tests/lib-threads/sockets.ml is re-enabled, since the two MPRs affecting
it have been fixed.
@dra27 dra27 force-pushed the testsuite-windows branch from 34726d4 to b505fc6 Compare January 25, 2016 16:20
@dra27
Copy link
Member Author

dra27 commented Jan 25, 2016

.gitattributes & .gitignore updated as requested, and rebased onto trunk.

mshinwell added a commit that referenced this pull request Jan 26, 2016
GPR#366: Various test-suite fixes for Windows
@mshinwell mshinwell merged commit 026572c into ocaml:trunk Jan 26, 2016
@dra27 dra27 deleted the testsuite-windows branch February 8, 2016 09:23
anmolsahoo25 pushed a commit to anmolsahoo25/ocaml that referenced this pull request Aug 25, 2020
mshinwell added a commit to mshinwell/ocaml that referenced this pull request Mar 26, 2021
chambart pushed a commit to chambart/ocaml-1 that referenced this pull request Jan 4, 2022
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants