Improve testing on Windows#3142
Conversation
kripken
left a comment
There was a problem hiding this comment.
Thanks for splitting this out!
lgtm aside from the comments.
|
|
||
| # Sources. | ||
|
|
||
| FILE(GLOB binaryen_HEADERS src/*.h) |
There was a problem hiding this comment.
I think we prefer file (lowercase) for these things.
Also, a newline? (but I'm not familiar enough with cmake, is there a reason not to?)
There was a problem hiding this comment.
(I meant after this line and before the next one, but no big deal.)
| return Prologue.getFileNameByIndex(FileIndex, CompDir, Kind, Result, | ||
| // FIXME: this fixes paths being incorrect when generated on | ||
| // Windows, but needs to be upstreamed how? | ||
| llvm::sys::path::Style::posix); |
There was a problem hiding this comment.
Do we need this? I thought the test was disabled earlier.
We considered doing this in the past, but were worried that it may change observable behavior for emscripten users with DWARF. If we change this we may need to make a corresponding change on the emscripten side, I'm not sure, but hopefully we can avoid both.
There was a problem hiding this comment.
This would allow us to enable those tests on Windows. I will remove it for now, since at least we now know exactly where that \ gets introduced..
This PR contains: - Changes that enable/disable tests on Windows to allow for better local testing. - Also changes many abort() into Fatal() when it is really just exiting on error. This is because abort() generates a dialog window on Windows which is not great in automated scripts. - Improvements to CMake to better work with the project in IDEs (VS).
This PR contains: