-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
Maybe you could create a new branch that I can retarget? I could not find a way to do that in the PR interface. |
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 the "single-header" mode is not useful if you are using conan. I think it might not be worth modeling it.
conanfile.py
Outdated
tmp = tools.load("json.hpp") | ||
license_contents = tmp[2:tmp.find("*/", 1)] | ||
tools.save("LICENSE", license_contents) | ||
if self.options.single_header: |
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.
Using options in source()
doesn't work that way. If you try to use the other option, it will fail.
source()
must be common for all settings and options. If anything I would move that to build()
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.
Right, my bad.
Hi! |
The previous version installed the With 3.1.0, it should be As for the single header version, I'm not sure what to do. Having the multiple headers only seems good to me, and if people really want the amalgamated version, adding the option is not a problem. |
0e53278
to
983f85c
Compare
I've pushed-force my changes, seems that Travis cannot run |
add_compile_options(-std=c++11) | ||
|
||
set(CMAKE_CXX_STANDARD 11) | ||
set(CMAKE_CXX_EXTENSIONS OFF) | ||
|
||
include(${CMAKE_BINARY_DIR}/conanbuildinfo.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.
This requires CMake 3.1.2 according to Conan's documentation
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.
Nice catch!
@theodelrieu Travis fails because of the deletion of build.py, see the build |
Oh right, sorry I've seen it fail so many times because of those |
ebd5528
to
9101455
Compare
I think that the include folder shouldn't probably be changed, so users can upgrade without changing their code. Regarding the amalgamated vs multiple headers, if the multiple headers really have a use case that they can be separately included, then maybe that should be the one and only package? Does it have a simple If that is the case, I'd say the amalgamated version might not provide any advantage for conan users. cc / @nlohmann |
We haven't made that I fear that allowing people to still include |
I understand both approaches, this is why I cc'd the library main author, to gather feedback about their personal preferences about this. |
I'm the one who split the library, in the end we chose to unify the installation methods and to install everything into an I think we should make reflect that intention in the package, and make the consumers change their code to Moreover, if we don't support the amalgamation, users will have to change their includes. |
@memsharded I trust in @theodelrieu in this regard. We have updated our documentation to use |
Ok, thanks! I'd say that such change would have deserved a 4.0 version, as it is breaking for consumers, but I am fine with it, not a big deal. Go @theodelrieu with the proposed changes to the conan package, thanks! |
Well technically, the library did not break anything. Rather, we were poor on installation instructions, but it did not matter that much with a single header library. However the cmake install target already installed the single header within an To sum up, users can still use the amalgamated version and install it wherever they want. Though, I understand the current PR breaks the conan package. That way users of It's annoying that people got confused whereas where to the library, I hope 3.1.1 will solve that. |
I'd say to keep it simple. If upstream github json 3.1.0 changed that, lets adjust to that. If I understood it correctly, the cmake install of the original repo already defined that in 3.1.0 (and broke to users), so conan package might be fine following the same behavior. |
No, it already defined that in 2016 (I forgot which release it was), and it did not break because the cmake install target did not exist prior to that date. To rephrase, the cmake install has always installed in What really 3.1.0 added is the multiple headers version. |
Ok, got it. Just as an added motivation: we just found a use case of a library doing |
Good! I'll merge the PR as is then |
.travis/run.sh
Outdated
conan create . travis/testing -s compiler=clang -s compiler.version=5.0 -s compiler.libcxx=libstdc++ -e CXX=clang++ |
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.
Removing the conan-package-tools will remove the automatic upload to bintray?
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.
Oops, I didn't know there was such a feature :/
Should I revert the removal of build.py
?
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.
Yep :)
I think so, it is very convenient that the upload is automatic, in that way @vthiery just have to merge contributions and packages will be automatically uploaded, no need to do it manually.
It can also be done with a conan upload
in the scripts but you need first to set the remote, then the user credentials for that remote... maybe better revert build.py, yes
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, my bad. Will do
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.
Ouch missed that! Yes indeed, the automatic upload is quite convenient, would be nice to keep it around :)
* Clean up test_package/ files.
* multi-header installation is now the default * Install inside include/nlohmann * Add sha256 verification
9101455
to
26f0740
Compare
Should be good now :) |
Hi,
I've updated the recipe for Json v3.1.0.
This release now includes a different way to install the project (multiple headers).
Users also have to
#include <nlohmann/json.hpp>
.I've cleaned/updated a few parts of the recipe as well.