Conversation
TurkeyMan
requested review from
WalterBright,
andralex and
wilzbach
as code owners
|
Thanks for your pull request and interest in making D better, @TurkeyMan! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + druntime#2707" |
|
I shunted the if's out of the makefile... what a hack! |
|
Lol, nice! |
| TESTS=array allocator new string | ||
|
|
||
| TESTS= array allocator new string | ||
| _MSC_VER=$(file < ..\..\ver.txt) |
There was a problem hiding this comment.
I would be very surprised if this works with Digital Mars make.
AFAICT including a file only works with a line like this (from druntime/win64.mak):
$(mak\COPY)
This reads a single assignment from the file.
There was a problem hiding this comment.
What do you expect doesn't work?
This does work... the tests all pass.
There was a problem hiding this comment.
Look for -version=_MSC_VER_xxxx in the CI logs. That value is read from this line at your comment.
There was a problem hiding this comment.
Surprising. $(file < MAK\COPY) is empty for me. _MSC_VER=$(..\..\ver.txt) would work.
I suspect the auto-tester is not using Digital Mars make here?
There was a problem hiding this comment.
I ran the same script locally... It works, and the tests invoke with all proper args... How are you running it?
auto-tester and appveyor both seem to work... My local machine too. I'm surprised...
There was a problem hiding this comment.
Like, check the CI logs. The msc ver and the cflags and dflags are supplied correctly. It also did build string_view, which is pulled the same way...
There was a problem hiding this comment.
Your PR passes for me locally, too. I'm confused ;-)
|
Why did that fail?! |
| $(TESTS): | ||
| "$(CC)" -c /Fo$@_cpp.obj test\stdcpp\src\$@.cpp /EHsc /MT | ||
| "$(DMD)" -of=$@.exe -m$(MODEL) -conf= -Isrc -defaultlib=$(DRUNTIMELIB) -main -unittest -version=_MSC_VER_$(_MSC_VER) -mscrtlib=libcmt test\stdcpp\src\$@_test.d $@_cpp.obj | ||
| "$(CC)" -c /Fo$@_cpp.obj test\stdcpp\src\$@.cpp /EHsc /MT $(ADD_CFLAGS) |
There was a problem hiding this comment.
Maybe this also works directly if there is a file with the name ADD_CFLAGS that has the settings, see https://github.com/DigitalMars/Compiler/blob/1dbd3e3381c8f7f7ab2e35214dcd63455ae38c29/dm/src/make/dmake.d#L1130.
There was a problem hiding this comment.
I didn't see any documentation to that effect in the scant docs.
That would be highly unusual though I think... I kinda like that way I've written it makes it terribly clear what mischief is at play.
Looks like one of the spurious failures (systime timed out). |
|
What's going on with buildkite? |
|
see e.g. dlang/dmd#10244 (review) |
|
Okay, so this PR is good then? |
|
Wait.. it just switched from green to red... what's happening here? O_o |
|
the concurrency tests have always been flaky |
|
So... am I supposed to do something now? |
|
Ive restarted the tests, they might take a bit to come online. |
5 participants
Enable C++17 unit tests (including
string_viewwhich hasn't had unit tests run before) for windows where the compiler supports it.Fixed a bug in
newwhen C++17 is enabled.