mamedev / mame

MAME
https://www.mamedev.org/
Other
7.97k stars 1.98k forks source link

should VS2019 build failures be demoted to non-critical? #6044

Closed belegdol closed 1 year ago

belegdol commented 4 years ago

Hello, VS2019 build is currently broken, most likely by https://github.com/mamedev/mame/commit/d27918db8c5a503c7f272e1d4539a410d4807df3:

C:\projects\mame\src\emu\diexec.cpp(414,60): error C2664: 'void device_t::save_item<device_execute_interface::device_input(&)[67]>(ItemType,const char *,int)': cannot convert argument 2 from 'osd::s32 device_execute_interface::device_input::* ' to 'const char *' [C:\projects\mame\build\projects\windows\mametiny\vs2019\emu.vcxproj]
C:\projects\mame\src\emu\diexec.cpp(414,60): error C2664:         with [C:\projects\mame\build\projects\windows\mametiny\vs2019\emu.vcxproj]
C:\projects\mame\src\emu\diexec.cpp(414,60): error C2664:         [ [C:\projects\mame\build\projects\windows\mametiny\vs2019\emu.vcxproj]
C:\projects\mame\src\emu\diexec.cpp(414,60): error C2664:             ItemType=device_execute_interface::device_input (&)[67] [C:\projects\mame\build\projects\windows\mametiny\vs2019\emu.vcxproj]
C:\projects\mame\src\emu\diexec.cpp(414,60): error C2664:         ] [C:\projects\mame\build\projects\windows\mametiny\vs2019\emu.vcxproj]
C:\projects\mame\src\emu\diexec.cpp(414,60): error C2664:   device().save_item(STRUCT_MEMBER(m_input, m_stored_vector)); [C:\projects\mame\build\projects\windows\mametiny\vs2019\emu.vcxproj]
C:\projects\mame\src\emu\diexec.cpp(414,60): error C2664:                                                             ^ [C:\projects\mame\build\projects\windows\mametiny\vs2019\emu.vcxproj]
       C:\projects\mame\src\emu\diexec.cpp(414): message : There is no context in which this conversion is possible [C:\projects\mame\build\projects\windows\mametiny\vs2019\emu.vcxproj]
       C:\projects\mame\src\emu\diexec.cpp(414): message :  device().save_item(STRUCT_MEMBER(m_input, m_stored_vector)); [C:\projects\mame\build\projects\windows\mametiny\vs2019\emu.vcxproj]
       C:\projects\mame\src\emu\device.h(613,17): message : see declaration of 'device_t::save_item' [C:\projects\mame\build\projects\windows\mametiny\vs2019\emu.vcxproj]
       C:\projects\mame\src\emu\device.h(613,17): message :     void ATTR_COLD save_item(ItemType &&value, const char *valname, int index = 0) [C:\projects\mame\build\projects\windows\mametiny\vs2019\emu.vcxproj]
       C:\projects\mame\src\emu\device.h(613,17): message :                    ^ [C:\projects\mame\build\projects\windows\mametiny\vs2019\emu.vcxproj]

This has an unfortunate side effect of causing the appveyor CI check to fail even though MinGW builds are working absolutely fine. Xes instead of ticks are then shown next to commits and PRs, reducing signal-to-noise ratio considerably. Appveyor does support allowing build failures: https://www.appveyor.com/docs/build-configuration/#allow-failing-jobs If ability to build with MSVC is only a nice to have, I could sumbit a PR allowing the MSVC build to fail and only failing the CI check if the MinGW build fails.

cuavas commented 4 years ago

It seems that MSVC has selected the wrong device_t::save_item overload. It should take the other one. Can you make a minimal test case to submit to Microsoft? I was pretty careful about doing the right thing with the wrappers here – it’s really frustrating having seemingly simple things like this fail. Also, is it really that commit, or is it the previous one that only worked with arrays where the compiler can work out what the length of the array is? That isn't actually an indeterminate length array.

Tangentially, what’s support for clang in Visual Studio like now? @smf- says it isn’t supported, but people still seem to be having success using it. Not having to deal with the idiosyncrasies of MSVC would avoid a lot of issues.

belegdol commented 4 years ago

I am afraid it is a negative regarding a test case. I am a packaging/patch/upstream sync/test monkey, I do not know C++. @yz70s, would you be able to help here? Having said that, I have verified that the build failures started with d27918db locally: 017b53b2 does build, d27918db does not. Finally, regarding visual studio: there is an official clang plugin [1] [2], but at the moment it is 32-bit only [3] so it runs out of memory when trying to build mame. The old plugin only works with VS2017 installed in parallel [4] according to what google says.

[1] https://devblogs.microsoft.com/cppblog/clang-llvm-support-in-visual-studio/ [2] https://devblogs.microsoft.com/cppblog/clang-llvm-support-for-msbuild-projects/ [3] https://developercommunity.visualstudio.com/idea/806348/distribute-x64-builds-of-clang.html [4] https://marketplace.visualstudio.com/items?itemName=LLVMExtensions.llvm-toolchain

cuavas commented 4 years ago

Does 017b53b2f58fa3c1b3e0b74d23f248892256dad5 only build successfully in CI because it’s building a subtarget that doesn’t try to save a member of an array in a struct? If you try building cps2.cpp or zn.cpp will MSVC choke when it gets to qsoundhle.cpp?

belegdol commented 4 years ago

You are correct, a two-driver build of 017b53b including cps2.cpp and zn.cpp fails with C2664 on qsoundhle.cpp.

cuavas commented 4 years ago

Well, a workaround for that compiler issue has been committed. But to (not) answer the original question, I don’t really know. MSVC support is marginal at best as not all developers have access to it, and only very specific versions will work because of the rate that MS introduces bugs.

yz70s commented 4 years ago

I have submitted a bug report that can be seen here https://developercommunity.visualstudio.com/content/problem/855010/vs2019-v1641-error-with-templates-and-default-para.html

rb6502 commented 4 years ago

I'm not answering the question either, but generally we'd like MAME to build on MSVC, and Microsoft wants MAME to build on MSVC (they've added MAME to their regression test suite, but it's unclear how up to date they keep it). MSVC itself isn't great, but there's a third-party ecosystem around it with genuinely cool things like Live++ ( https://molecular-matters.com/products_livepp.html ) that would be interesting to have access to.

Along the same lines, I'd like MAME to be buildable in Xcode to use their debugger and profiling tools but that may require switching the build system which is a big task.

cuavas commented 1 year ago

We no longer have CI builds for Visual Studio – they’re too resource-intensive and tended to just die for no good reason.