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

Switch to CMake #43

Closed
2 tasks done
aytekinar opened this issue May 15, 2018 · 22 comments
Closed
2 tasks done

Switch to CMake #43

aytekinar opened this issue May 15, 2018 · 22 comments

Comments

@aytekinar
Copy link

Would you be interested in making a switch to cmake-based installation? For the project I am working on, I have started removing Makefiles and writing CMakeLists.txt files to

  • Get better support in modern IDEs, and,
  • Have modular and relocatable libraries in my project.

To make the sample functions and tests work (enforcing C++11 features in a portable way), I guess you need a CMake version of at least 3.1.3 (dates back to Feb 12, 2015).

Once finished, the build, test, and install processes will be portable among systems that support CMake. Moreover, all the *.cpp files in test-high directory will be removed, as they are simply compile definitions used on pcg-test.cpp and pcg-test-noadvance.cpp.

If you are interested, I can make a PR as soon as I finish writing the CMake scripts.

@imneme
Copy link
Owner

imneme commented May 26, 2019

I'm interested... I'm not sure what's best as a baseline. Is there any sense in having both?

@lemire
Copy link

lemire commented May 26, 2019

Yes. You can do both.

@lemire
Copy link

lemire commented May 26, 2019

@imneme The benefit of CMake is improved portability (meaning, going to different systems and compilers). Your makefile probably won't work for Visual Studio users whereas CMake will work there as well.

However, a makefile has features of its own. You can do low-level optimizations to the build with a makefile that are hard to do with CMake.

@aytekinar
Copy link
Author

aytekinar commented May 26, 2019

You can have both, but in my opinion, it is not meaningful to have both at the same time. Makefiles are natively supported on *nix platforms. If the user is using Visual Studio on Windows, then your Makefile will not be useful. Moreover, modern IDEs such as CLion support natively cmake-based projects. This means that you can simply drag and drop a cmake-based project as a subdirectory to a super project, and let cmake handle the dependencies appropriately in a cross-platform compatible way.

#44 should be a nice place to start off. If you want, I can do a couple of more modifications to the folder structure to make it even better.

EDIT. Both @lemire and I wrote at the same time, aparently. Sorry for a similar post from my side :)

@lemire
Copy link

lemire commented May 26, 2019

it is not meaningful to have both at the same time

Certainly, the Makefile is no longer needed generally once you have a CMake, and having both introduces redundancy and thus maintenance costs... However, there are things that are easy to do with a hand-tuned Makefile that are hard to do with CMake.

CMake tends to produce rather dumb builds that are slow.

@aytekinar
Copy link
Author

However, there are things that are easy to do with a hand-tuned Makefile that are hard to do with CMake.

I tend to disagree with you on this. However, I need to mention that I am not a C/C++ expert with a lot of experience in C++, CMake and Makefiles. I have not personally experienced the dumb and slow Makefiles generated by CMake, but I have read about and experienced performance improvements obtained when using Ninja instead of Makefiles in CMake.

That being said, since this library is a header-only library, I doubt we could observe any penalty when switching to CMake. In any case, I am not the owner of the repo, and the repo can certainly have both at the same time. All I can do is to improve my first version of CMake with more of the suggested steps mentioned in Effective CMake.

@lemire
Copy link

lemire commented May 26, 2019

@aytekinar

Dropping the Makefile entirely would be fine in case of this project.

Further comments to illustrate the points I was making...

Responding to

However, there are things that are easy to do with a hand-tuned Makefile that are hard to do with CMake.

You wrote

I tend to disagree with you on this.

Let me give you an example. If you want to turn on LTO (link-time opti.), it is literally just 6 characters in a Makefile (add -flto)... In CMake, as far as I can tell, you need to do something like this...

include(CheckIPOSupported)
check_ipo_supported(RESULT ltoresult)
if(ltoresult)
set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE)
endif()

So while I can keep in my head the LTO flag (-flto) the CMake way... I just can't memorize. I have to look it up. And I submit to you that if you are reading the CMake configuration, it will be a lot harder on you to see these configuration lines.

That's unavoidable. CMake abstracts away the configuration. So it is necessarily higher level... and, as we know, all abstractions are leaky. At some point, for some corner cases, CMake is going to make you pay for the abstraction.

There is no free lunch.

I have read about and experienced performance improvements obtained when using Ninja instead of Makefiles in CMake.

That's correct, I think. A hand-tuned Makefile can match a Ninja build, but if you are using CMake to generate a Makefile, you are getting something dumb. Going with Ninja might be a workaround.

@aytekinar
Copy link
Author

Wow. Thank you, @lemire, for your time in writing such a detailed answer to explain your view. As I said, I have not thus far had any situation where I need to use any of those features of CMake.

So far, I have only used CMake as a dependency management tool --- defining targets and their dependencies (usage/consumption requirements). For most people (e.g., Effective CMake), this is how CMake should be used. The reason is that global variables such as CMAKE_INTERPROCEDURAL_OPTIMIZATION in your example above and CMAKE_CXX_STANDARD in the first version of my CMake proposal (#44) affect the whole project, and thus should be avoided. These variables should be defined in toolchain files or packaging files, where the developer or the user can finetune these compiler and linker properties. Hence, I was saying above that I still have to improve on the first version (by possibly decoupling the test and samples parts of the library from its source) by following the best practices.

Be it pcg-cpp or any other library for that matter, I think the most important thing in (sub-)projects are what the targets and their consumption requirements are. That's why I personally don't like Makefiles or CMake-based projects that define low-level compiler (e.g., -Wall) and linker properties as well as tests in the project. They should be decoupled from the library itself so that I can simply drag and drop any library to my super project and use it easily (I believe that testing is the developer's responsibility, whish is properly handled with CIs in this repo or others, and I should not be forced to have the tests and samples of each library if I am going to use them as dependencies in my CMake project).

@tdegeus
Copy link

tdegeus commented Apr 15, 2021

I'm very much interested to rely on pcg-cpp. But at the moment it is quite hard to rely on it without precisely CMake support (and a regular) release interval. In fact, it would suffice to use CMake to install the headers and create a target which then contains the relevant paths.

Without it, I guess that for me the only realistic way is to use git sub-module or so. But I must admit that I'm not strongly in favour of this.

@48656c6c6f
Copy link

Access to a package configuration file for integration into CMake projects would be helpful:
https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html?highlight=configure

@lemire
Copy link

lemire commented Nov 11, 2021

@SomethingOtherThanNull

If someone wants to produce a CMake build with the requested configuration files as part of a pull request I will review it and make a recommendation.

If nobody is willing to do the work, it may not happen.

@aytekinar
Copy link
Author

aytekinar commented Nov 12, 2021

Hi,

I don't understand the request for CMakePackageConfigHelpers and the comment on "... producing a CMake build as part of a PR ... If nobody is willing to do the work, it may not happen."

If you check the associated PR, #44, you should be able to see that CMakePackageConfigHelpers is (hopefully) used properly. I mean, the PR dates back to 3 years ago, so it may require some updates; but, I think I have done a good enough job in introducing both configure_package_config_file and write_basic_package_version_file from CMakePackageConfigHelpers in my PR #44.

Please correct me if I am wrong, or comment further on what is needed from youme.

@lemire
Copy link

lemire commented Nov 12, 2021

@aytekinar I was responding to @SomethingOtherThanNull who felt that people had not provided package configuration.

Sorry if my reply was confusing.

@lemire
Copy link

lemire commented Nov 12, 2021

@aytekinar I have edited my reply to make it more precise.

@lemire
Copy link

lemire commented Nov 12, 2021

I will email @imneme with a proposal.

@aytekinar
Copy link
Author

Thank you, @lemire, for clarification. I would appreciate your feedback, then, on my PR, which has the package configuration settings/lines in place.

@lemire
Copy link

lemire commented Nov 12, 2021

@aytekinar Given that the request came from @SomethingOtherThanNull, it would be best if they had a look.

On my end of things, I am working on seeing how we can get your PR merged.

@aytekinar
Copy link
Author

I am working on seeing how we can get your PR merged.

Thank you very much on that part. When I skimmed, today, all the comments both here and in the PR, I noticed that you've always been very supportive of this PR. I also went back in time to check our discussions/threads regarding my (unkept) promise of contributing to simdjson/simdjson#180, which I will do by the end of this year (I will go through your fantastic project, check if there is still anything I can do to contribute there, and come up with some suggestions).

Anyway, I will not clutter this space much about a different thread/project.

@48656c6c6f
Copy link

So sorry for creating confusion. My statement was meant to be in support of providing CMake as an option. With that, a major benefit is the ability to have such configuration files that make adoption of the package easier.

It was not intended to be commentary on the PR or anyones work. I’m just fullly in support of CMake as an option. Take a look at gRPC as an example of a package that provides multiple choices.

Have a great day.

@lemire
Copy link

lemire commented Nov 12, 2021

I think that there is agreement among several of us on the benefits of CMake in this instance. We just need to make it happen.

My comment above was unclear, sorry about that. I think I misunderstood what you were commenting on.

@imneme
Copy link
Owner

imneme commented Nov 12, 2021

I'm not averse to CMake, but given that PCG is an include-only header, I'm not sure what it really brings. The Makefiles in the repository are just sanity-check testing.

Anyway, a pull request would be great.

@aytekinar
Copy link
Author

I'm not averse to CMake, but given that PCG is an include-only header, I'm not sure what it really brings.

The main benefit of using CMake is to enable/facilitate the use of PCG in a portable way in C++ projects. The CMake configurations will tell the build systems where to find the header files, what language standard is used in PCG, etc. With this information, the build systems will then be able to call the compilers with the proper switches and compilation options. Even though a library is header-only, still these libraries contain certain requirements (e.g., define statements, language standards such as c++11 or c++14, etc.).

Anyway, a pull request would be great.

#44 was/is the pull request opened to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants