-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Avoid setting global CMake directories #1451
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
Comments
if you have a fix, that would be great. I don't think anybody here is very handy with CMake. |
This problem was immediately pointed out in #1163 (comment) Personally, I just wonder why these variables keep getting reintroduced and then taken out, just to fix tests... why doesn't the test runner set up DLL dependencies for you? Meson does. :D :D Perhaps the solution is to just remove the testsuite support from cmake entirely? That seems weird too. |
Unfortunately I do not have a good solution for json-cpp as I am also not very experimented with cmake and just observed how things got messed up due to this global variables when we wanted to include a recent version of json-cpp. |
I now had a deeper look at it. In #1163 the creater opened the issue because the C-Test failed on Windows to introduce that change. Actually, I do not know much about these tests but when I build jsoncpp on a Windows machine with the current version 1.9.5 they also fail:
Maybe this is due to some problem with the creation of a shared library? I do not know, I only know that when we include jsoncpp on Windows in our CI, we always build jsoncpp static via cmake (for now at least). However, whatever the above problem is, I think that a better workaround (i.e., still not a very good solution) to my initial problem would be to only address the Windows compiler if the problem happens on Windows. Furthermore, I think it makes much sense to not set these variables globally. Here's the output of my windows built with cmake:
For the merge request see: #1453 |
Setting IMO, there are two ways how this problem can be solved
|
@atrelinski You are free to improve the workaround I proposed. It is anyhow much better then the current implementation which sets global variables for all compilers. Mine only sets local variables for MSVC only. |
So, this is PR based on @atrelinski proposal. |
Yes, now we do 😄 Thanks in advance! |
In a4fb5db, global cmake variables were set. This causes problems when jsoncpp is built together with other libraries by one cmake call. This messes up variables that should not be set or would be used differently for other libraries.
The text was updated successfully, but these errors were encountered: