Skip to content
This repository has been archived by the owner on Mar 14, 2019. It is now read-only.

Fix up includes to nlohmann json #29

Closed
wants to merge 3 commits into from
Closed

Fix up includes to nlohmann json #29

wants to merge 3 commits into from

Conversation

z3ntu
Copy link

@z3ntu z3ntu commented Sep 24, 2018

Might require more changes for the changed include path

See also mujx/nheko#448

@z3ntu
Copy link
Author

z3ntu commented Sep 24, 2018

And here the build fails because the docker image doesn't get built during process and therefore doesn't contain the necessary change. The Windows and Mac (clang-format is missing for some reason, so it actually fails) builds work though.

Copy link
Owner

@mujx mujx left a comment

Choose a reason for hiding this comment

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

Btw there already an open PR for the same issue.

@@ -53,7 +53,7 @@ set(JSON_HEADER_URL
set(JSON_HEADER_HASH
ce6b5610a051ec6795fa11c33854abebb086f0fd67c311f5921c3c07f9531b44)

file(DOWNLOAD ${JSON_HEADER_URL} ${DEPS_INSTALL_DIR}/include/json.hpp
file(DOWNLOAD ${JSON_HEADER_URL} ${DEPS_INSTALL_DIR}/include/nlohmann/json.hpp
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better to use the install target of the this project so we can also use find_package.

@z3ntu
Copy link
Author

z3ntu commented Sep 25, 2018

Oh, didn't see the other PR. #28 looks better so I'll close this one.

@z3ntu z3ntu closed this Sep 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants