-
Notifications
You must be signed in to change notification settings - Fork 6.3k
cmake/jsoncpp.cmake: update to jsoncpp v1.8.4 #3467
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
Conversation
|
The jsoncpp version we currently use also does not support move semantics, so this would also be a big benefit! |
|
Please rebase and remove the merge commit, it is not possible to review it properly. |
chriseth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove code repetitions.
libdevcore/JSON.h
Outdated
| Json::FastWriter writer; | ||
| writer.omitEndingLineFeed(); | ||
| return writer.write(_input); | ||
| std::stringstream stream; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please combine this code with the one above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
libdevcore/JSON.h
Outdated
| return writer.write(_input); | ||
| std::stringstream stream; | ||
| Json::StreamWriterBuilder builder; | ||
| builder.settings_["indentation"] = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compact layout should not even have newlines. Does it behave like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the behaviour is as expected. To be very sure, I will add a JSON Testsuite that will cover that - test/libdevcore/JSON.cpp.
libsolc/libsolc.cpp
Outdated
| Json::Value input; | ||
| if (!reader.parse(_input, input, false)) | ||
| std::stringstream inputStream(_input); | ||
| Json::CharReaderBuilder readerBuilder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write a helper function that does all this for us? It could even re-use a static variable of type Json::CharReaderBuilder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure!
a92eb55 to
bd09335
Compare
|
I wasn't able to run the ipc specific tests with
A lot of ipc related tests seem not to work. Sometimes However, because of that I couldn't run the ipc related tests. I was also not able to test my changes in Additionally to this, it looks like that void CharReaderBuilder::strictMode(Json::Value* settings)
{
//! [CharReaderBuilderStrictMode]
(*settings)["allowComments"] = false;
(*settings)["strictRoot"] = true;
(*settings)["allowDroppedNullPlaceholders"] = false;
(*settings)["allowNumericKeys"] = false;
(*settings)["allowSingleQuotes"] = false;
(*settings)["stackLimit"] = 1000;
(*settings)["failIfExtra"] = true;
(*settings)["rejectDupKeys"] = true;
(*settings)["allowSpecialFloats"] = false;
//! [CharReaderBuilderStrictMode]
}It looks like a |
|
There are compilation errors on travis. |
|
Oops.. its always good to use different compilers :) |
|
On Node.js:6 Compiler: gcc C++ In Is it possible that |
|
The C++ travis build is not finding the make target for |
|
Somehow it sometimes uses |
|
Please also re-enable the fallthrough warning in cmake/jsoncpp.cmake |
|
We can manually do We shouldn't re-enable the However, do you mean "re-enable" in the sense of explicitly enabling I think that its safe to just remove the whole implicit-fallthrough-block in |
|
|
||
| Json::Value const& sources = _input["sources"]; | ||
| if (!sources) | ||
| if (sources.empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bug fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, they just removed operator!() in version 1.8.4
bool Value::operator!() const { return isNull(); }Lol, now I was able to fully understand why I had problems using that operator. On my system where was somehow an old jsoncpp header used - will check the build system, how that was possible - and because they don't provided an implementation of operator!() I had linking problems. I just realised that they remove the operator!(), thats why I used sources.empty() here. Now I see that they also providing
Value::operator bool() const { return ! isNull(); }So everything is still fine. Wow, sorry for that! The original code still works very nice with the new version 1.8.4. I will remove that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... turned out it was a bug fix
bool Value::empty() const {
if (isNull() || isArray() || isObject())
return size() == 0u;
else
return false;
}In contrast to Value::operator bool() , Value::empty() is also checking whether an empty array, or an empty object was provided. I just added two new test-cases in test/libsolidity/StandardCompiler.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create a new PR which adds the test and removes the use of the ! operator (without upgrading jsoncpp)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! See pull-request #3502.
|
We recently (6 months?) added some code to |
6f8d731 to
70bb519
Compare
|
I tried to fix the issue with the emscripten build. I saw that somehow a very old I tried to increment the So somehow the However, I tried now my findings on travis, but it looks like that there are still other problems. Now it looks like that functions from the standard library are missing. I have no idea why I don't see that locally. I ran the stuff also within a Any ideas? One question - because I want to push my version of |
f8398dd to
b65c3e3
Compare
We added code to ignore fallthrough warnings, but later the fallthroughs were fixed and the ignore directive removed. |
|
@chriseth do you know why the build was successful for |
# Disable implicit fallthrough warning in jsoncpp for gcc >= 7 until the upstream handles it properly
if (("${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU") AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 7.0)
set(JSONCCP_EXTRA_FLAGS -Wno-implicit-fallthrough)
else()
set(JSONCCP_EXTRA_FLAGS "")
endif()I removed the whole block, because from the compiler flags point of view it should never check for falltroughs. With Probably the way how you built |
|
circle uses |
|
@chriseth interesting.. i tried 1.37.33-64 bit.. and there i had this optimizer bug with the |
|
|
eb21045 to
1be2339
Compare
| include(GNUInstallDirs) | ||
| set(prefix "${CMAKE_BINARY_DIR}/deps") | ||
| set(JSONCPP_LIBRARY "${prefix}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}jsoncpp${CMAKE_STATIC_LIBRARY_SUFFIX}") | ||
| set(JSONCPP_LIBRARY "${prefix}/${CMAKE_INSTALL_LIBDIR}/${CMAKE_STATIC_LIBRARY_PREFIX}jsoncpp${CMAKE_STATIC_LIBRARY_SUFFIX}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chfast isn't this pointing to some system wide directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${CMAKE_INSTALL_LIBDIR} should be just "lib".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean together with prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chfast if ${CMAKE_INSTALL_LIBDIR} would be just lib, we would have problems with lib64 again, or am i wrong?
75f6fb3 to
81388f7
Compare
9ca84f1 to
689ae8d
Compare
|
@aarlt this is now compiling :) |
689ae8d to
53a09d0
Compare
|
Without the |
cmake/jsoncpp.cmake
Outdated
| -DJSONCPP_WITH_PKGCONFIG_SUPPORT=OFF | ||
| -DCMAKE_CXX_FLAGS=${JSONCCP_EXTRA_FLAGS} | ||
| -DCMAKE_CXX_FLAGS=${JSONCPP_EXTRA_FLAGS} | ||
| -DCMAKE_CXX_STANDARD=11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try keeping CMAKE_CXX_STANDARD=11 only, remove "extra flags".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean remove EXTRA_FLAGS above or remove the line adding c++11 to EXTRA_FLAGS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it seems the extra flags are needed as the warnings are back now. Also there's an excessive amount of warnings from cmake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so bring it back as it was but remove -DCMAKE_CXX_STANDARD=11.
cmake/jsoncpp.cmake
Outdated
| -DJSONCPP_WITH_TESTS=OFF | ||
| -DJSONCPP_WITH_PKGCONFIG_SUPPORT=OFF | ||
| -DCMAKE_CXX_FLAGS=${JSONCCP_EXTRA_FLAGS} | ||
| -DCMAKE_CXX_FLAGS=${JSONCPP_EXTRA_FLAGS} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line.
53a09d0 to
3baffc1
Compare
| -DTESTS=0 \ | ||
| .. | ||
| make -j 4 | ||
| make VERBOSE=1 -j 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After successful build, this should be reverted.
.travis.yml
Outdated
| # Disable tests unless run on the release branch, on tags or with daily cron | ||
| #- if [ "$TRAVIS_BRANCH" != release -a -z "$TRAVIS_TAG" -a "$TRAVIS_EVENT_TYPE" != cron ]; then SOLC_TESTS=Off; fi | ||
| - SOLC_TESTS=Off | ||
| #- SOLC_TESTS=Off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After successful build, this should be probably reverted.
|
@axic yep its compiling :) i wrote that on gitter - but looks like u missed it :) |
cmake/jsoncpp.cmake
Outdated
| set(JSONCPP_LIBRARY "${prefix}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}jsoncpp${CMAKE_STATIC_LIBRARY_SUFFIX}") | ||
| set(JSONCPP_LIBRARY "${prefix}/${CMAKE_INSTALL_LIBDIR}/${CMAKE_STATIC_LIBRARY_PREFIX}jsoncpp${CMAKE_STATIC_LIBRARY_SUFFIX}") | ||
| set(JSONCPP_INCLUDE_DIR "${prefix}/include") | ||
| set(JSONCPP_EXTRA_FLAGS "-std=c++11") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end it must be only for GCC / clang. You can wrap it with if(NOT MSVC).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? Appveyor seems to be building fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So Appveyor works with this, do you think it is still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it works because of this flag: https://msdn.microsoft.com/en-us/library/mt490614.aspx.
Let's leave it if AppVeyor is ok.
|
For some reason now Travis is emotional again: |
191fdbc to
56b75c3
Compare
|
@chriseth can you have a look at this? |
|
@chriseth are you ok with this? Travis emscripten works. I'll remove the last commit if you're happy with this version before merging. |
56b75c3 to
fa2a28a
Compare
|
|
||
| BIN=$PREFIX/bin | ||
|
|
||
| PATH=$PREFIX/bin:$PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have any effect after the script has run?
Fixes #2829.
To get version 1.8.4 to work, we need to get rid of the usage of deprecated API. PR #3532 (libdevcore/JSON.h - new API) will remove the deprecated API calls.
Remember, that
scripts/travis-emscripten/build_emscripten.shis currently using the systemscmake, that is shipped withtrzeci/emscripten:sdk, it is not using the one that is installed withscripts/install_cmake.sh.The new
jsoncpp(at least version 1.8.4) needs a minimumcmakeversion of 3.1, that is currently not provided bytrzeci/emscripten:sdk, at least not with version1.37.33-64bit. So minor changes on the build scripts are needed.