Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake support for shared library and Visual Studio 2019 #2133

Closed
wants to merge 5 commits into from

Conversation

paulhoux
Copy link
Collaborator

@paulhoux paulhoux commented Feb 9, 2020

This PR adds support for building Cinder as a shared library using the CMake system. It also adds support for Visual Studio 2019.

See also the discussion around PR #2117 . My PR also removes the forced /MT setting and can therefore be used instead of #2117.

The CMake files for Windows were missing the following files in the DLL build:

  • videoInput/videoInput.cpp

The following files were specifically excluded from the DLL build:

  • All of AntTweakBar's files
  • cinder/ImageSourcePng.cpp
  • cinder/app/AppScreenSaver.cpp
  • cinder/app/msw/AppInplMswScreenSaver.cpp

The following libraries were added as dependencies in the DLL build:
kernel32.lib,user32.lib,gdi32.lib,winspool.lib,comdlg32.lib,advapi32.lib,shell32.lib,ole32.lib,oleaut32.lib,uuid.lib,odbc32.lib,odbccp32.lib

A build option was added: CINDER_ENABLE_CXX17 (defaults to OFF). Setting this to ON will attempt to build Cinder using the C++17 language. For Visual Studio 2019, this means it will use the proper <filesystem> header, instead of <experimental/filesystem>.

Important! This PR has only been tested on Windows. Before accepting it, please make sure it does not break building on the other platforms!

@MikeGitb
Copy link
Contributor

MikeGitb commented Feb 9, 2020

Why is CINDER_ENABLE_CXX17 (and the whole associated machinery) necessary, when cmake already has CMAKE_CXX_STANDARD?

@paulhoux
Copy link
Collaborator Author

Visual Studio 2019 defaults to C++14, so I wanted to allow people to opt for C++17 instead. Most of the "machinery" was already in the make files, but I guess using CMAKE_CXX_STANDARD for that is indeed cleaner.

@MikeGitb
Copy link
Contributor

MikeGitb commented Feb 10, 2020

Personally I think you could plain remove everything from line 89 to 134. It is essentially just duplicating existing cmake functionality.

But I haven't checked myself if those heads any bad sideeffect.

@paulhoux
Copy link
Collaborator Author

Doesn't that break compatibility with Visual Studio 2015? I've looked at the current version of the cmake file and those lines were added in #1594 . According to CMake's docs, prior to Visual Studio 2015 update 3 the CXX_STANDARD flag doesn't do anything.

@MikeGitb
Copy link
Contributor

Yes, because VS2015 doesn't have different language modes in the first place.

@MikeGitb
Copy link
Contributor

Btw. I believe that _HAS_CXX17 is a STL-internal macro that should not be used in user code.

https://stackoverflow.com/questions/52379233/is-has-cxx17-marco-usable-in-custom-project-headers-to-enable-c17-language

I haven't checked, when this was introduced, but for conditionally enabling std::filesystem, the standard featur test macro __cpp_lib_filesystem might be a better choice. Alternatively there exists _MSVC_LANG mentioned in the linked SO thread.

@paulhoux
Copy link
Collaborator Author

Thanks, Mike. Seems I still have a lot to learn.

@MikeGitb
Copy link
Contributor

MikeGitb commented Feb 13, 2020

Mostly minor stuff I'd say. I also just realized that the scrip claims compatibility with cmake 2.9 (I wonder if anyone is actually verifying this - 2.9 was realeased around 2010 ) which I believe didn't have CMAKE_CXX_STANDARD.

Anyway, I think a cmake overhaul has to wait until after 9.3 got released, but I'd very much appreciate if this PR would find its way into 9.3.

[EDIT: that was supposed to mean 9.2]

@MikeGitb MikeGitb mentioned this pull request Aug 21, 2020
26 tasks
@godefv
Copy link

godefv commented Nov 6, 2020

I have used cinder on linux without any issue.
Then, on Windows, it is broken because it is incompatible with C++17 and C++20.

Thanks to this PR, I can compile cinder with C++17, but still not C++20, because std::result_of has been removed in C++20. We now need to use std::invoke_result, which is not there before C++17.

It is a pain to keep supporting every C++ standard. So, I think that, since C++14 is quite old now, there could be a last release supporting it, before dropping C++14 support for future releases, in favor of C++17 and C++20. It would make cinder only fresher and more attractive.

@MikeGitb
Copy link
Contributor

MikeGitb commented Nov 6, 2020

Could you open an issue about the c++20 breakage - ideally with the error message and or line, where result_of is used?

@godefv
Copy link

godefv commented Nov 7, 2020

Let me provide more detail.

With GCC 10.2 on linux, I can compile with C++20. It seems to have kept backward compatibility, despite the C++20 standard not requiring it.
With Clang or VS2019 on Windows 10, compilation fails. There a couple of other issues in addition to result_of.

You can check here (godefv@f33b943) what I have done to successfully compile with C++20. This commit is on top of this PR.

EDIT: I have put this comment in a new issue as you have asked.

@godefv godefv mentioned this pull request Nov 7, 2020
@paulhoux paulhoux closed this Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants