Skip to content

Switch to cmake 3.13 #727

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AndreiCherniaev
Copy link

stop to use make because since CMake 3.15 (Ubuntu 20.04), Generator can be set in env CMAKE_GENERATOR and sometimes it is not the Unix Makefiles; Tested in Ubuntu 24
P.S. Also you can install to non-system folder, see example https://askubuntu.com/a/1543849/1087530

stop to use make because since CMake 3.15 (Ubuntu 20.04), Generator can be set in env CMAKE_GENERATOR and sometimes it is not the Unix Makefiles;
Tested in Ubuntu 24
P.S. Also you can install to non-system folder, see example https://askubuntu.com/a/1543849/1087530
@daniestevez
Copy link
Owner

daniestevez commented Mar 17, 2025

Can you clarify why CMake >= 3.13 is required?

@AndreiCherniaev
Copy link
Author

AndreiCherniaev commented Mar 17, 2025

Can you clarify why CMake >= 3.13 is required?

In according with link, -B <path-to-build> added in version 3.13.
P.S. And CMake 3.13 is very old software. For example Qt project for now use 3.16

In fact we can change readme only to add modern cmake code... I mean because of my new build code in readme I marked minimum cmake as 3.13

@daniestevez
Copy link
Owner

There is nothing wrong with the instructions in the documentation. mkdir build && cd build && cmake .. && make && sudo make install is a perfectly fine way to install CMake projects. There are many other possible ways, and people that want to install to a specific directory or to do something else will know how to do it.

So if the whole point of requiring CMake >= 3.13 is to allow the use of cmake -B in documentation, to me that doesn't sound enough of a reason. To be honest, I don't know if there is anything in the CMakeLists.txt of gr-satellites that breaks with CMake 3.8, since this isn't tested in CI. But my point was about why to make this change now.

@AndreiCherniaev
Copy link
Author

AndreiCherniaev commented Mar 17, 2025

There is nothing wrong with the instructions in the documentation. mkdir build && cd build && cmake .. && make && sudo make install is a perfectly fine way to install CMake projects. There are many other possible ways, and people that want to install to a specific directory or to do something else will know how to do it.

So if the whole point of requiring CMake >= 3.13 is to allow the use of cmake -B in documentation, to me that doesn't sound enough of a reason. To be honest, I don't know if there is anything in the CMakeLists.txt of gr-satellites that breaks with CMake 3.8, since this isn't tested in CI. But my point was about why to make this change now.

Please don't directly call make in modern code. Since CMake 3.15 (Ubuntu 20.04), Generator can be set in env CMAKE_GENERATOR and sometimes it is not the Unix Makefiles... For example Qt can't be builded with make, you need ninja. This is main reason. But there are others:
2) don't ask user to manage multi-threading building by himself, cmake can do it automatically by --parallel;
3) use -B option to avoid cd. In this case user can more easy take your code and put it to complex script with a lot of things...

@daniestevez
Copy link
Owner

daniestevez commented Mar 17, 2025

Since CMake 3.15 (Ubuntu 20.04), Generator can be set in env CMAKE_GENERATOR and sometimes it is not the Unix Makefiles...

If someone has set CMAKE_GENERATOR to something other than make they don't need this documentation. They will know what to do. I'm yet to find a system where cmake .. && make doesn't work because cmake is generating something other than Makefiles.

don't ask user to manage multi-threading building by himself, cmake can do it automatically by --parallel

make can also do that. You can do make -j$(nproc) if that's what you want. It can be risky to recommend --parallel or -j$(nproc) by default, since depending on the amount of CPUs and RAM in the system, all RAM can be exhausted.

use -B option to avoid cd.

I fail to see the advantage here. These are two ways to accomplish the same thing. Arguably cd is more memorable because most people know about cd but fewer know about all the cmake options.


More importantly, in editing this documentation you have failed to account for its context. Before these commands, the docs say "The following can be run inside the directory containing the gr-satellites sources". There is a section above it about how to download the sources. In this section, downloading the latest stable release is recommended. But you have added commands for git clone and cd gr-satellites. After the commands, the paragraph says "after running make", but with your changes there is no make command any more.

Also, you have added cmake --install build, but that typically needs sudo, in the same way that make install.

@AndreiCherniaev
Copy link
Author

AndreiCherniaev commented Mar 17, 2025

I'm yet to find a system

Try Ubuntu 24 or any, by default there is no make application, and you no need it to build gr-satellites or other software... Just for example if you try to build Qt with make you probably will have a error (because you need ninja).
For example this is all what you need in Ubuntu 24 (one more time, I have no make in system):

sudo apt install g++ cmake ninja-build git gnuradio-dev bzip2
cmake -S . -B build
cmake --build build --parallel
sudo cmake --install build

And of cause Windows and macOS hasn't make. I suggest way which is really nice. 99% users will do as your instruction says that is why for me so important to provide modern cmake code to allow people to have good experience. When I say modern I mean from 2018 year...

you have failed to account for its context
but that typically needs sudo

Thanks, fixed.

stop to use make because since CMake 3.15 (Ubuntu 20.04), Generator can be set in env CMAKE_GENERATOR and sometimes it is not the Unix Makefiles;
Tested in Ubuntu 24
P.S. Also you can install to non-system folder, see example https://askubuntu.com/a/1543849/1087530

Actually you can still try using old cmake 3.8, but the instruction is adapted for 3.13
@daniestevez
Copy link
Owner

Try Ubuntu 24 or any, by default there is no make application

By default there is no cmake or g++ either. That's the reason you are installing them. make is a dependency of build-essential.

Just for example if you try to build Qt with make you probably will have a error (because you need ninja).

Why is this relevant? gr-satellites is not Qt.

For example this is all what you need in Ubuntu 24 (one more time, I have no make in system):

Sure you can build gr-satellites however you like on your system, and you can avoid installing make if you want to. The point of this PR is not how one particular person wants to builds gr-satellites on their system. It is whether we change the build instructions in the documentation. These build instructions have worked fine for years and still work in most cases.

And of cause Windows and macOS hasn't make.

Windows doesn't have anything of what is required to build GNU Radio out-of-tree modules (including, but not limited to, gr-satellites). This is true regardless of whether you build in the way you mention. It is quite cumbersome to get a suitable build environment in Windows, and these days probably the best way is through conda.

I don't know enough about macOS to tell if it has make by default, but I would find it surprising that installing make is any harder than installing cmake and ninja.

99% users will do as your instruction says that is why for me so important to provide modern cmake code to allow people to have good experience.

The current instructions will work fine for 99% of the users that are building from source.

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 this pull request may close these issues.

2 participants