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

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

Closed
imwints opened this issue Jul 23, 2023 · 5 comments · Fixed by #589
Closed

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

imwints opened this issue Jul 23, 2023 · 5 comments · Fixed by #589
Assignees
Labels
enhancement New feature or request

Comments

@imwints
Copy link
Contributor

imwints commented Jul 23, 2023

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.

@imwints imwints added the enhancement New feature or request label Jul 23, 2023
@aristocratos
Copy link
Owner

@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
Copy link
Contributor Author

imwints commented Jul 26, 2023

@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
Copy link
Owner

@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
Copy link
Owner

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
Copy link
Contributor Author

imwints commented Jul 26, 2023

Already done that

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

Successfully merging a pull request may close this issue.

2 participants