do not skip unittests for Win64 #1411
Conversation
|
Hooray! |
|
Hmm, maybe not quite there yet. |
Works on my system (Win8). Does anybody know what OS the Win64 tester is According to the test history, all other targets are built, but Other strange things:
|
|
The win64 tester host rebooted itself last night and it's not setup to auto-start the tester. It's back in service now. |
|
Oh, and it's a windows server 2012 ec2 instance. |
| @@ -573,7 +573,7 @@ Throws: $(D ErrnoException) if the file is not opened or if the call to $D(fread | |||
| */ | |||
| void rawWrite(T)(in T[] buffer) | |||
| { | |||
| version(Windows) | |||
| version(Win32) | |||
There was a problem hiding this comment.
I don't know enough about windows libc's to evaluate the correctness of this change. Can you cite a resource justifying why this is win32 only? The rest of the changes are great, assuming the tests pass.
There was a problem hiding this comment.
I was just imitting what rawRead is doing, but it is wrong, I have already added setmode for MSVCRT locally, so switching the translation on/off will work, too.
Thanks for the info. |
|
I have installed Windows Server 2012 into a VM, still no problems if I add the correct libcurl.dll for x64. Is it possible that the build server picks up the 32-bit version? I have removed the corresponding unittest too try it without that dependency. |
|
$ md5sum ../win64libs/dmd2/windows/bin/libcurl.dll ../win64libs/dmd2/windows/libcurl-win64.zip |
|
Hmm, that is exactly the same dll I am using. But the recent changes never ran, because the tester seems to be merging dmd/2346 for 9 hours now... The error code reported in the log is 0xc0000022, but I didn't find a lot of information on this. Seems to be STATUS_ACCESS_DENIED, so maybe there are some restrictions on what is allowed to be executed? |
|
I just chmod u+x'ed the files, which ought to help |
Thanks. It seems one of our changes has helped. I'll add the dependency |
|
Please don't merge yet. I'll enable all unittests, most of them are not compiled in yet. Some of them revealed codegen bugs, so I'll version those out. |
|
imho, increased test coverage is important and useful. Get this or a subset of it in shape to be committed and save the rest for a second or more pull request. |
|
I have committed my changes to enable all unittests but the few that actually trigger bugs. Unfortunately I just realized that this depends on dlang/druntime#545. Anyway, I've filed the related bugs: |
| @@ -30441,6 +30444,7 @@ string tzDatabaseNameToWindowsTZName(string tzName) | |||
| case "Africa/Johannesburg": return "South Africa Standard Time"; | |||
| case "Africa/Lagos": return "W. Central Africa Standard Time"; | |||
| case "Africa/Nairobi": return "E. Africa Standard Time"; | |||
| case "Africa/Tripoli": return "Libya Standard Time"; | |||
There was a problem hiding this comment.
@jmdavis Is this the correct fix for http://d.puremagic.com/issues/show_bug.cgi?id=10161 ? I was just guessing here.
There was a problem hiding this comment.
It might be. I'll have to look some stuff up to be sure. I haven't fixed it myself yet, because I'm in the middle of splitting up std.datetime and didn't want to commit anything which would conflict with that.
There was a problem hiding this comment.
According to the page that I got all of that information from ( http://unicode.org/repos/cldr-tmp/trunk/diff/supplemental/zone_tzid.html ) "South Africa Standard Time" is "Africa/Tripoli," and "Libya Standard Time" is not yet listed at all. But Tripoli is in Libya, so I don't know. It looks like that page was last updated in March. However, given that Tripoli is the capital of Libya and that Windows now seems to have a "Libya Standard Time," it's likely correct. However, what's missing with your change is the change to windowsTZNameToTZDatabaseName. Its switch statement needs the conversion in the other direction, and it's actually that missing piece which is causing test failures for Dmitry on his Windows 8 box.
There was a problem hiding this comment.
I do clearly need to be doing a better job of checking that site for updates though, since it does have "Africa/Tripoli" (albeit with a different Windows time zone), and std.datetime doesn't currently have it at all.
There was a problem hiding this comment.
It looks like there's a bug report for "Africa/Tripoli": http://unicode.org/cldr/trac/ticket/5721
There was a problem hiding this comment.
Thanks for looking it up. Regarding the inverse in "windowsTZNameToTZDatabaseName" that's what I have added below in line 30462.
There was a problem hiding this comment.
Ah. I missed that, because I was looking at what was shown above the comment and forgot to look at the rest of the diff.
|
Argh! Out of memory! I can split object files so that it manages to build datetime.d, but it later fails on algorithm.d. Would it be possible to set the LARGEADDRESSAWARE bit on dmd.exe during the build process? That helps. |
| @@ -13941,7 +13943,7 @@ assert(tod6 == TimeOfDay(0, 0, 59)); | |||
| else | |||
| static assert(0); | |||
|
|
|||
| value %= 60; | |||
| value = value % 60; | |||
There was a problem hiding this comment.
Please mark this with the bug# so that it's clear when it should be reverted. Though it looks like Walter has a pull which fixes the %= problem already, so I don't know that we want to make commits which get rid of %=.
There was a problem hiding this comment.
I guess the fix by Walter will precede this commit, I'll revert it then.
This helps, too: |
|
This finally builds successfully, but I found another issue when disabling debug information: http://d.puremagic.com/issues/show_bug.cgi?id=10664 @braddr Regarding the failure when building debug info that I could not reproduce locally: the log says "LINK : fatal error LNK1318: Unexpected PDB error; RPC (23) '(0x000006BA)'". Are the testers checking out into a new directory or could there be some old files left from previous builds (like vc100.pdb or unittest.pdb)? |
|
always new.. the first step is mkdir and the last step is rm -r . If something is left over, it'd have to be because files are being written explicitly outside the build dir by the compiler. |
|
rebased and squashed. |
|
Rebased, squashed and tests for another fixed bug enabled. |
|
Merged and rebased. |
|
It seems that the win64.mak file is modified by the tester after checkout, but before merging. This stops any PR that tries to modify the makefile. |
|
yeah. I'll try to get that aspect of the win64 auto-tester cleaned up sometime this week. I put priority on having it back up and working at all over doing it cleanly. |
Thanks, the win64.mak issue is fine now. Unfortunately curl.lib seems to be missing for the Win64 builder. |
|
Oh right.. and I even have the previous email still. Thanks for the re-send though. I've put the file up at downloads.dlang.org/other/curl-7.28.1-devel-rainer.win64.zip for future reference. With a little luck, the next run of this pull request will go that much better. |
|
Great progress on reducing this big diff down over the last few months. Just a few issues left to split out and get addressed. All cases of disabled tests should have a critical bug created and associated with them. The real == double changes should probably be a standalone commit. Likely they should be conditional on real.sizeof == double.sizeof or something similar rather than version(Win64). One pull request that would be good to have is one that just drops the big version'ing of unittest.d. That will be a good indicator of what part of the onion of issues is the outermost, so to speak. |
I was afraid someone would say that ;-) Actually there are no longer a lot of tests disabled, these are only fibers in core.thread and tmpfile in std.stdio AFAICT. There are still some implementations missing in std.math, but these are not tested at all (or only for Posix).
real != double for Win64, but the MS-C runtime does not support reals, and we don't have a version for the used C runtime. I wish we had a D implementation of parsing and printing reals, as these differ for every C runtime. See the different versions to check output. Most of the changes here can only be split from enabling the unittests by not running any tests, some only fix the tests. I'll add a few bug resports for the actual code changes. |
|
Fixed and rebased. |
|
rebased. No interest in running phobos unittests on win64? |
|
There is still a linker error. |
|
The error seems like a variation of https://d.puremagic.com/issues/show_bug.cgi?id=9756. |
|
dlang/dmd#3074 allows this pull to pass. |
|
@rainers : Does this only require a rebase? |
|
I think the way I'd approach finishing this little bugger off is to pull the unitest.d and win64.mak changes into a separate pull request. It will, of course, fail. From there, chip off logically separate parts of this set until the unittest.d/win64.mak pull request passes. I still think this one contains 3-5 separate issues that should be addressed individually. The largest block, the real changes, I also really wish weren't conditional on win64 but rather on some less single platform and more generic real == double sort of conditional. Done with the consultation of platform porters that have similar issues. ARM, for example, falls into this set. For the other parts, is it really win64 or is it msvc vs dmc? Could the win32 side use the same code? Etc. I'm not familiar with windows enough to do a good evaluation, but this much additional conditional logic concerns me from a maintainability standpoint. |
|
fixed conflicts and rebased. |
It's slightly different for Win64, because the processor and D support 80-bit reals, but the C runtime does not. I'd rather have C runtime independent conversion functions string-to-float and back, as every C runtime is slightly different anyway.
Most of the time it is MS runtime vs. DM runtime. I use version CRuntime_Microsoft vs. CRuntime_DigitalMars in my COFF32 version. |
|
Aargh, "Out of memory" @braddr The build server uses an older version of optlink when compiling dmd. Here is the end of the dmd build log: Could you please update it to 8.00.15 to support the /LARGEADDRESS option |
|
@rainers things are changing, it no longer runs out of memory: any chance addressing this? |
|
It would be great to have Win64 on the auto tester soon :). |
|
Grrr, it's another issue of using C's printf output that's different in every implementation (nan/inf again). I'd prefer if we were not relying on it at all (aswell as the rest of the C runtime). |
…ing convention std.format, std.math: workarounds for different behaviour of sprintf std.conv: workarounds for different behaviour of strtold std.math: disable unittests for exp2f and exp2l std.math: fix lrint(real), disable tmpfile test std.process: seek to end of file before trying to append to it from another process std.process: do not try to terminate an invalid process handle win64.mak: disable COMDAT folding for release build
|
It wasn't as bad as expected, because the test call mapped to doFormat, not printf. Unittest should be fixed now. |
True, strtol is very problematic. |
|
Nice to see a working Win64 test. |
|
Looks like it's ready to merge. |
|
Auto-merge toggled on |
|
Thanks. |
With recent fixes to the compiler it is now possible to run unittests on Win64 aswell.