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.

Sign up for GitHub

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

Jump to bottom

Update embedded jsoncpp from unk version to 0.10.6 + move libs to lib/ instead of src/ #5473

Merged
merged 9 commits into from
Apr 2, 2017
Merged

Update embedded jsoncpp from unk version to 0.10.6 + move libs to lib/ instead of src/ #5473

merged 9 commits into from
Apr 2, 2017

Conversation

nerzhul
Copy link
Member

@nerzhul nerzhul commented Mar 28, 2017 •
edited

0.10.6 is last release without c++11

It seems we are using a 2010 update to a 2016 version.
Also properly integrate jsoncpp static library using same architecture as amalgamate.py generate in jsoncpp repo

Also reorganize our repository to a more standard way by moving external libraries to lib/ instead of our source tree src/

@nerzhul
Update embedded jsoncpp from unk version to 0.10.6
66175d2 
0.10.6 is last release without c++11
@nerzhul nerzhul added the Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements label Mar 28, 2017
@nerzhul
Copy link
Member Author

nerzhul commented Mar 28, 2017

@SmallJoker can you test it on VS ?

@nerzhul
Copy link
Member Author

nerzhul commented Mar 28, 2017 •
edited

The amalgamated version was generated cloning https://github.com/open-source-parsers/jsoncpp/tree/0.10.6, running amalgamate.py , and i fixed some integration problems we have (one macro to define, include fix, and removal usage of a gcc 6 feature which is unk to gcc 6.1...)

@nerzhul
Make jsoncpp more compliant with its amalgamate
d939f03 
Jsoncpp cpp file should be upper, make the library like it does in amalgamate
@SmallJoker
Copy link
Member

Compiled and tested on MSVC 2010. Works as before, no noticeable changes.

@nerzhul
Copy link
Member Author

nerzhul commented Mar 29, 2017

@SmallJoker is this an approval ?
Thanks in advance

@SmallJoker
Copy link
Member

SmallJoker commented Mar 30, 2017 •
edited

@nerzhul Not yet an approval.
My suggestion for a new directory structure: (does not break any includes but the one you fixed in jsoncpp itself)

minetest/src/
   json/    (renamed from jsoncpp)
      CMakeLists.txt
      json.h
      jsoncpp.cpp
      UPDATING

There are also checks needed if issue #1793 is still a thing. (as linked in jsoncpp/CMakeLists.txt)

@nerzhul
Copy link
Member Author

nerzhul commented Mar 30, 2017

for the folder structure i will propose a second PR to move our libs in a dedicated lib/ directory instead of being in the source folder tree

@SmallJoker
Copy link
Member

You already moved CMakeLists.txt and jsoncpp.cpp in this PR. IMO it should be only updated with the same paths or updates + moved outside. (I think 1st is what you prefer right now)

@nerzhul nerzhul changed the title Update embedded jsoncpp from unk version to 0.10.6 Update embedded jsoncpp from unk version to 0.10.6 + move libs to lib/ instead of src/ Mar 31, 2017
@nerzhul
Copy link
Member Author

nerzhul commented Mar 31, 2017

@SmallJoker i moved the libs to lib/ folder as discussed on IRC yesterday, can you test it on VS (don't forget to use flags to use embedded libs for jsoncpp, lua and gmp)

@SmallJoker
Copy link
Member

Successfully compiled in debug mode, using bundled Lua, gmp and Json.

sfan5
sfan5 previously requested changes Mar 31, 2017
View reviewed changes
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also forgot to update util/travis/script.sh and build/android/jni/Android.mk

cmake/Modules/FindGMP.cmake Outdated
set(GMP_LIBRARY gmp)
add_subdirectory(gmp)
add_subdirectory(${PROJECT_SOURCE_DIR}/../lib/gmp ${PROJECT_BINARY_DIR}/../lib/gmp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you adding two paths here?

If the reason is that it doesn't work otherwise, then: no, sorry this is way too hacky.

cmake/Modules/FindJson.cmake Outdated
set(JSON_LIBRARY jsoncpp)
add_subdirectory(jsoncpp/json)
add_subdirectory(${PROJECT_SOURCE_DIR}/../lib/jsoncpp ${CMAKE_BINARY_DIR}/lib/jsoncpp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

src/CMakeLists.txt Outdated
set(LUA_INCLUDE_DIR "${PROJECT_SOURCE_DIR}/lua/src")
add_subdirectory(lua)
set(LUA_INCLUDE_DIR "${PROJECT_SOURCE_DIR}/../lib/lua/src")
add_subdirectory(../lib/lua ${CMAKE_BINARY_DIR}/lib/lua)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@nerzhul
Copy link
Member Author

nerzhul commented Mar 31, 2017

I'm working on a better implementation using lib/CMakeLists.txt

@nerzhul nerzhul dismissed sfan5’s stale review April 1, 2017 08:18

Fixed points you mention, please re-review

@nerzhul nerzhul requested a review from sfan5 April 1, 2017 11:41
@nerzhul nerzhul merged commit 86b1542 into minetest:master Apr 2, 2017
@nerzhul nerzhul deleted the jsoncpp_update branch April 2, 2017 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SmallJoker SmallJoker SmallJoker approved these changes

@sofar sofar Awaiting requested review from sofar

@sfan5 sfan5 Awaiting requested review from sfan5

Assignees
No one assigned
Labels
Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements One approval ✅ ◻️
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nerzhul @SmallJoker @sfan5