core icon indicating copy to clipboard operation
core copied to clipboard

Please add VS 2015 support if possible

Open acmorrow opened this issue 8 years ago • 18 comments

We are using MNMLSTC core to provide polyfills for string_view and optional in the upcoming MongoDB C++11 driver: https://github.com/mongodb/mongo-cxx-driver/tree/master

It has been a big help for our GCC and clang builds, but unfortunately core doesn't appear to work on VS 2015. I just tried HEAD of master last night. If we can't use MNMLSTC core with VS 2015, then our options are to either build our own polyfills from scratch and use those everywhere, which we are not excited about, or to find a different source of the polyfills to use only with VS 2015, which we also aren't too excited about.

It does look like some CMake awareness of WIN32 was added recently. Is VS 2015 support on the roadmap for 2.0? I realize that VS 2015 is still a pretty flawed C++11 compiler, so perhaps it just isn't viable yet for something like core.

In any event, an answer either way would be helpful, and excited to see work progressing towards core 2.0.

acmorrow avatar Dec 18 '15 14:12 acmorrow

Hi.

I'm hoping to have some support for VS2015 in Core 2.0. Unfortunately development is slow at the moment while I wrestle with Travis-CI and how garbage it is at targeting multiple compilers and operating systems (take a look at the .travis.yml to see how huge the target matrix is for Core), having to write new documentation, writing new tests and all while I'm recovering from a recent surgery. I'm hoping I'll have a lot of free time during the winter holiday to take care of all this. I might unfortunately require VS2015 Update 1 as the minimum, because it's just "better".

I should note that with very few issues, string_view can be cut out of its header and placed into its own. The implementation for optional can as well (the expected and result types are non-standard and might potentially break MSVC)

bruxisma avatar Dec 19 '15 07:12 bruxisma

Thanks for the update. Totally agree about the joys of travis. I think requiring Update 1 is completely reasonable. It really feels like they need Update 2 already. We have found a lot of bugs still in Update 1.

That is encouraging about possibly being able to take string_view and optional in isolation. They are the only components we are using right now, so that might be a way forward for us if the more heavyweight components of core prove too much for VS2015.

Please let us know if there is anything we can help with in terms of testing. Our timeline for shipping the first stable C++11 driver release would be around the end of January.

acmorrow avatar Dec 20 '15 16:12 acmorrow

Hi @slurps-mad-rips -

I had a few minutes to try out VS2015 Update 1 with HEAD. I've run into two issues, one easily solvable, the other needs some investigation:

  • The string_view.hpp and meta.hpp files need to include the header <ciso646> because VC doesn't automatically make available the alternate syntax tokens and and or unless this header is included. I'm 99% sure this is a standard violation.
  • The meta.hpp file fails to compile with lots of errors relating to find_if on line 132. The error lines are offset by one in the messages below because I'd already added <ciso646> to the file. I'm not familiar enough with what is going on here to decode the error:
Z:\data\src\mnmlstic_core\include\core/meta.hpp(133): error C2143: syntax error
: missing ';' before ',' [Z:\data\src\mnmlstic_core\build\tests\test-algorithm.
vcxproj]
Z:\data\src\mnmlstic_core\include\core/meta.hpp(133): error C2514: 'F<Args...,T
>': class has no constructors [Z:\data\src\mnmlstic_core\build\tests\test-algor
ithm.vcxproj]
  Z:\data\src\mnmlstic_core\include\core/meta.hpp(133): note: see reference to
  class template instantiation 'core::v2::meta::impl::find_if<core::v2::meta::i
  mpl::list<T,Ts...>,F,Args...>' being compiled
Z:\data\src\mnmlstic_core\include\core/meta.hpp(133): error C2143: syntax error
: missing '>' before ';' [Z:\data\src\mnmlstic_core\build\tests\test-algorithm.
vcxproj]
Z:\data\src\mnmlstic_core\include\core/meta.hpp(133): error C2976: 'std::condit
ional': too few template arguments [Z:\data\src\mnmlstic_core\build\tests\test-
algorithm.vcxproj]
  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xtr1common(72)
  : note: see declaration of 'std::conditional'
Z:\data\src\mnmlstic_core\include\core/meta.hpp(133): error C2955: 'std::condit
ional': use of class template requires template argument list [Z:\data\src\mnml
stic_core\build\tests\test-algorithm.vcxproj]
  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xtr1common(72)
  : note: see declaration of 'std::conditional'

acmorrow avatar Dec 27 '15 16:12 acmorrow

So, I've been working on a local branch for VS2015 support for string_view and optional. There are some serious issues with the VS2015 compiler. The one that I find mindblowing is that the find_if is trying to construct a template class F instead of waiting for it to be instantiated (hence the error C2514: 'F<Args...,T>': class has no constructors error. I fixed this in the local branch by changing it to be F<Args..., T>::value. Other issues persist however beyond these and it will take quite a bit of work to get it all working. I was told that they permit expression based SFINAE in VS2015 Update 1, however from what I've seen it is straight up broken, which is going to make having optional be constexpr and correct (using std::addressof if an operator & overload is available).

To be quite honest, I had high hopes that bits of Core would work with VS2015U1, but it is broken in so many spectacular ways I am legitimately speechless at the sheer number of bugs in the compiler. (For example, try compiling the test-algorithm executable with MSVC and just watch what happens)

Work will continue however. Worst case scenario, there will be an optional.msvc.hpp header that gets included only if you are using VS2015 (and it may not be constexpr, as I've run into tons of bugs with it)

bruxisma avatar Dec 29 '15 17:12 bruxisma

I had the same high hopes for VS2015 that they would finally ship a quality C++11 implementation, and am similarly disappointed in its results. The MongoDB C++11 driver doesn't currently build, independently of the status of core, and we don't push the compiler even close to as hard. We are (all) going to be forced to make some hard decisions due to MSFTs continued failure to ship a decent toolchain.

I think, btw, that we can probably get by with a non-constexpr optional and string_view, special cased for MSFT, if that is the way you go.

acmorrow avatar Dec 29 '15 17:12 acmorrow

@acmorrow I made a pull request (#43) where string_view and optional are fixed for VS2015U1.

If you can try it out / report bugs or compiler errors I would be grateful.

Eyenseo avatar Jan 10 '16 15:01 Eyenseo

Hi @Eyenseo - I went in a slightly different direction for the C++11 driver, and have now added an option to use boost for the optional and string_view polyfills on Windows. That isn't ideal of course, I'd much prefer to use MNMLSTC on that platform too.

I would like to try out your fork, though it isn't the highest priority at the moment because I have a workaround.

acmorrow avatar Jan 13 '16 14:01 acmorrow

@slurps-mad-rips - The more recent VS2015 updates seem a lot better. Do you think it might be worth revisiting this?

acmorrow avatar Sep 10 '16 15:09 acmorrow

Hey! I'm definitely down to revisit it. I'm currently stuck without a windows PC until mid-october-ish. Once that rolls around I'll be able to start on this.

I'm also currently debating departing from some of Core's original mission for the 2.0 release. (Such as "no compiler hacks"). I might also make 2.0 the last C++11 release. At this point C++14 affords so much in its usefulness that it has become a pain and a chore to write C++11.

I'll be talking to a few colleagues at CppCon next week for their opinion and insight.

bruxisma avatar Sep 10 '16 18:09 bruxisma

As long as we get one more C++11 release (2.0, it sounds like) that the MongoDB C++11 driver can use, then I think we are good. We only depend on string_view and optional.

acmorrow avatar Sep 10 '16 19:09 acmorrow

@hanumantmk and @machyne - Do you have any thoughts on how much longer we would need a C++11 MNMLSTC? I think at least until C++17 ships with std::string_view and std::optional, but I also think we can basically work with whatever is in a 2.0 indefinitely.

acmorrow avatar Sep 10 '16 19:09 acmorrow

@acmorrow I've pushed a change that should make optional and string_view work in VS2015. Please let me know if you run into any issues! :)

bruxisma avatar Jan 03 '17 09:01 bruxisma

@slurps-mad-rips Awesome, thanks very much!

I think @xdg and @jrassi (the current maintainers of mongocxx) will be excited to see this. Next steps would be to do get a Windows CI loop up using MNMLSTC/core and VS2015. Assuming all goes well with that and that mongocxx now builds with MNMLSTC/core with VS2015, would this warrant a 1.1.1 release/tag?

Also, I was interested in the USE_FOLDERS change you made in aea910228b484411e5d8dde8b35f18804b05fd52. Is that something we should be looking at for our own CMake? The CMake documentation is (as is often the case), somewhat sparse.

acmorrow avatar Jan 03 '17 13:01 acmorrow

@acmorrow The release process is kind of messed up because I screwed up with the development of Core a while back (end of 2014, beginning of 2015). What was going to be Core 1.2 is now Core 2.0 because of serious underlying ABI changes across a few types that your driver is not using.

The current version that's untagged (HEAD/latest/whatever you would like to call it) is stable. I don't think I'll be making many changes other than a few "deprecated" attribute tags for some types that will be removed in Core 3.0 (which will signal the move to C++14), and at most it'll be bugfixes or last minute function additions. I should probably sit down and write a roadmap so others can know what's coming and what's going away after the 2.0 release. (2.0 will end up with a 'legacy' branch so we can keep a 2.0.x tag set going for small bugfixes and changes)

Most of the changes prior to a 2.0 release are documentation related, as sphinx-doc have upgraded their C++ domain a great deal compared to when Core's documentation was first written (and I discovered the guzzle_sphinx_theme which looks fantastic).

Regarding the ABI changes, it shouldn't be a problem because I tried to be careful with inline namespacing, and with all the people using it I've yet to see an issue filed for it.

As for the CMake thing, I used USE_FOLDERS so that I could edit the files directly within Visual Studio and get quicker feedback. That's the property's sole purpose. It's useful if you're developing in Visual Studio or any other IDE backed generator. I typically use make or ninja however. I'm going to leave it in for anyone that would like to develop in Visual Studio directly.

I'm going to leave this issue open for a bit more, in case you have any questions or run into any issues.

bruxisma avatar Jan 04 '17 09:01 bruxisma

That is all great info, much appreciated. I do think a roadmap document would be valuable. Thanks again.

acmorrow avatar Jan 04 '17 13:01 acmorrow

I confirmed today that our driver compiles with VS2015 Update 3 and passes tests, when compiled with latest Core (aea9102) after applying #55 (so, in addition to optional and string_view, it turns out that we also added a dependency on make_unique since this issue was originally filed). Our build team is currently only providing us machines with Update 2 (which hit a few "C1001: An internal error has occurred in the compiler" errors when compiling Core), so it'll take some time to get a machine with Update 3 into our CI loop, but otherwise it looks like aea9102 totally unblocks our Windows efforts.

Thanks so much @slurps-mad-rips, for your work on this. To clarify, you're not planning on tagging a release of the master branch until 2.0, correct?

jrassi avatar Jan 19 '17 01:01 jrassi

@jrassi That's correct, I won't be tagging a release of master until 2.0. I'll also be making a 2.0 branch so that we can backport changes while work begins on the 3.0 branch. Once #55 is merged in, I'll go ahead and also close this issue.

I should note unless some other issues are filed and resolved the only changes left for Core 2.0 are documentation and packaging based.

bruxisma avatar Jan 20 '17 10:01 bruxisma

Thanks again!

jrassi avatar Jan 20 '17 21:01 jrassi