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.

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

One build system to rule them all #698

Closed
theodelrieu opened this issue Aug 13, 2017 · 16 comments
Closed

One build system to rule them all #698

theodelrieu opened this issue Aug 13, 2017 · 16 comments

Comments

@theodelrieu
Copy link
Contributor

Right now, we have mainly two ways of building the library (i.e. tests):

  • Makefile
  • CMake

There is also a meson.build file, which only provides support for meson users, so they don't have to rely on a wrap.

There are some issues with the CMakeLists.txt (as #695 shows. I also personally have to clean my build folder each time to regenerate the precompiled header on Linux)

There are also targets that are only available in the Makefile (e.g. make pretty)

I think we should adopt a single build system, I will soon need to add a target to amalgamate the library after we split the monolithic json.hpp, and I don't know if I should add it only in the Makefile, the CMakeLists.txt etc...

Rewriting everything would not take very long, since this is not a huge project.

What do people think?

My personal preference would go towards Meson, but we could also simply rewrite the CMake to be more in phase with what people call "modern CMake".

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Aug 13, 2017
@nlohmann
Copy link
Owner

An explanation of the status quo:

  • I added the Makefiles to ease my workflows for the development and release process. They do not necessarily belong in this repository, but they make things easier if people open PRs.
  • The Cmake files were originally added to use MSVC on AppVeyor. Meanwhile, I also use them for the Linux and macos builds on Travis. They were not intended to replace the Makefiles, because I only added targets to compile and run the test suite.
  • I personally do not care about package managers. For those who use them, I started to add files and a small section to the README file. But frankly I think this is overhead for a single-header library.

I personally would not really favor merging the pure maintainer scripts and the Cmake files. If the Makefile in the root is confusing, moving it into a subfolder could help.

@nlohmann
Copy link
Owner

I think it would be a good idea to move all Makefile targets that are used in Travis (i.e., running cppcheck, sanitiziers, Valgrind, etc.) into the CMake file.

@theodelrieu
Copy link
Contributor Author

That would be great!

@nlohmann
Copy link
Owner

FYI: Started with https://github.com/nlohmann/json/tree/feature/issue698.

Strange issue: Clang sanitizers finds issues that were not found before: https://travis-ci.org/nlohmann/json/jobs/268514575 I tried to move the check from the Makefile to the Cmakelists, but it seems the CXXFLAGS differ. Or there really is an issue in the std::valarray code that was not detected before and that I cannot find otherwise.

@theodelrieu
Copy link
Contributor Author

Awesome!

This could be related to valarray implementation, according to cppreference some implementations use expression templates, in which case using auto can be troublesome.

But that's just pure speculation at this point.

@dan-42
Copy link
Contributor

dan-42 commented Aug 26, 2017

to tackle the package-manager concerns, one way would be that you throw it all out, and keep this project clean from any package-manager stuff. And provide only developer scripts for compiling and running tests, but no installing.
And every maintainer for the specific package-manager shall fork/submodule/orWhatEver this project and provide all what is needed for the specific package manager. And only a short documentation in the README about which package-manager is supported by which project as an entry point for the users.

Pro:

  • no overhead in a single-header lib from all that stuff
  • clear position on how and which package-managers are supported
  • freedom for the maintainer for the package-manager

Con

  • usage may not be clear for the user who
  • maybe many forks/submodules for the same package manager
  • no " control " by the lib-maintainer

@nlohmann
Copy link
Owner

I totally agree. And so far, we only have the meson.build file in the repo.

@theodelrieu
Copy link
Contributor Author

Agreed, as for Meson, there's no need for a meson.build file in the repo, the wrap-db system works fine.

For Conan, it is trivial to implement a recipe for such a project.

@theodelrieu
Copy link
Contributor Author

Hi, I took a look at branch feature/issue698 and I was wondering if there was anything left to do with it?

I'd like to integrate the changes in my PR, it might solve the Coverall deadlock.

@nlohmann
Copy link
Owner

nlohmann commented Sep 4, 2017

I'll check tonight.

@nlohmann
Copy link
Owner

nlohmann commented Oct 4, 2017

@theodelrieu I cleaned up the Makefiles and I currently can't get rid of the json_unit file, because I found no way to convince Coveralls to collect data from several binaries. I saw that you seemed to have fixed this in #700, but I have not understood that. Once I have Coveralls working without json_unit, I can further clean up Makfile and test/Makefile.

@theodelrieu
Copy link
Contributor Author

I propose that you cherry-pick the latest #700 commit to see if that fixes the issue.

Basically, the arguments given to coveralls were not in the correct path, and I found out that lcov was not needed anymore.

Coveralls can be given a single directory, whether including or excluding it.

@nlohmann
Copy link
Owner

nlohmann commented Oct 5, 2017

Thanks, got it! :)

@nlohmann
Copy link
Owner

nlohmann commented Oct 5, 2017

Coverage is now also handled within CMake.

@nlohmann nlohmann self-assigned this Oct 5, 2017
@nlohmann nlohmann removed the state: please discuss please discuss the issue or vote for your favorite option label Oct 5, 2017
@nlohmann nlohmann added this to the Release 3.0.0 milestone Oct 5, 2017
@theodelrieu
Copy link
Contributor Author

Great! I'll try to convert the single_include target to CMake again on #700

@nlohmann
Copy link
Owner

nlohmann commented Oct 5, 2017

OK. Most of the building is now done by Cmake. There are some maintainer targets left I may clean up some time later. I close this ticket for now.

@nlohmann nlohmann closed this as completed Oct 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants