aristocratos / btop

A monitor of resources
Apache License 2.0
20.25k stars 624 forks source link

[REQUEST] Up-to-date CMake fork and CMake support #583

Closed imwints closed 1 year ago

imwints commented 1 year ago

The cmake fork by @jan-guenter seems unmaintained at the moment.

With the fork as inspiration I've created a new CMakeLists.txt to build btop with CMake. I'm committed to maintain this file as an alternative to the Makefile.

It might be time to ask again, after reading the discussion #82, if it is now worth considering to include the CMakeLists.txt file alongside the Makefile in the official tree. I would like to maintain it (in the case it gets accepted).

You've made good points against #82 and it is completely right that the Makefile is capable of building btop and does not need to be replaced.

Adding to the thread a point in favor of CMake is maintainability. The adjustments for LLVM were not trivial (86 changes). I've maintained a private CMake file for a couple of months and adding LLVM support was a one-liner, which should just point out the convenience of CMake. It was a lot easier and it will be for future tasks, like handling the fmt dependency (e.g.: using the system fmtlib and fallback to the submodule, yeah a weird usecase) or GPU support (detecting system capabilities at compile time, excluding flags and checks based on the configuration). The Makefile will grow substantially with every new feature and will become harder to maintain. Make simply wasn't designed for this.

It would be nice to have it as a secondary option and maybe later see what is easier to maintain.

Can the new branch be linked from the README? The CMake port will probably not get any attention until it gets officially supported but for now it's enough to bring CMake up-to date.

aristocratos commented 1 year ago

@nobounce Adding a CMakeLists.txt and using a build folder for the files cmake creates would work fine. I believe I even mentioned that the last time this was discussed, as long as someone is willing to maintain it I'm all for it. 👍🏼

But I noticed the compile time with the generated files from the CMakeLists.txt you linked above was really slow: 1 min 21 sec versus 24 seconds from the original Makefile. So you should probably add some parallelization by default in the CMakeLists.txt file.

Not sure why you needed to make "86 changes" for llvm support? Should only be 2 or 3 compile flags that differ no?

I'm planning on removing the "auto compiler finder" and some other "extra" functionality in the Makefile, and instead just print out a warning if the compiler isn't known to work.

The Makefile will grow substantially with every new feature and will become harder to maintain. Make simply wasn't designed for this.

Has not been my experience so far.

Regarding "GPU support", it uses dynamic loading of gpu libraries by default, so there is no detection needed. Only if compiling with static libraries, in which case it should be manually flagged by the user.

imwints commented 1 year ago

@aristocratos

We want to contribute full CMake building and installation support. Will you merge such a PR when it's ready?

From my point of view not much have changed in regards of the need of switching to a CMake based build system.

From #362, that's why I was curious if your opinion has changed :) maybe I missed a follow up discussion.

That overall sounds great.

Regarding the build time, you build it with make? I'd advise to use Ninja instead and put this in the documentation, it is using all available threads by default and should be faster with rebuilds

# Preferred with Ninja, parallel by default
cmake -B build -G Ninja && cmake --build build
# or native ninja
cmake -B build -G Ninja && ninja -C build
# or for make
cmake -B build && make -C build -j `nproc`
# or even better
cmake -B build && cmake --build build --parallel # Will test if `nproc` is needed aswell

If that's a deal breaker I'll have a look at this but there is no native support to change the make invocation as far as I'm aware. I'll properly address both things in the documentation.

About the GPU, I'm not sure if it's a runtime process only. You need to include headers which might not be present on systems and then you can't compile at all. This might need to be an optional build-time feature to enable or disable.

aristocratos commented 1 year ago

@nobounce

Regarding the build time, you build it with make? I'd advise to use Ninja instead and put this in the documentation, it is using all available threads by default and should be faster with rebuilds

As long as "good defaults" and any dependencies are documented in the readme it's all good.

About the GPU, I'm not sure if it's a runtime process only. You need to include headers which might not be present on systems and then you can't compile at all. This might need to be an optional build-time feature to enable or disable.

It uses dynamic loading of gpu libraries at runtime, all the function calls and structures are already defined in the source.

Only if manually statically compiling the libraries for AMD (disabling dynamic loading at runtime) is there any need for extra includes, you can look at the Makefile in the GPU PR for which flags are needed.

aristocratos commented 1 year ago

The fmt submodule has been reverted btw, the needed files are now in the include folder instead. So also no need to search for fmt headers anymore.

imwints commented 1 year ago

Already done that