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

Improve CMake docs #985

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

generic-pers0n
Copy link

@generic-pers0n generic-pers0n commented Nov 22, 2024

This PR intends to solve #662 by updating the documentation to better cover CMake.

Note that as far as I understand, it appears that the process of building with CMake hasn't changed much and remains fundamentally the same even after #461 and the subsequent PRs, maybe perhaps with a few changes in targets and build options. I could be extremely wrong, but either way, I try to cover the changes in target names from 19.7 to the latest development version and also add a full list of CMake build options. I also mention Ninja as an alternative to the traditional make since it also works in the same way as it too.

I'm currently marking this draft as a PR because I'm looking at refining more of the build documentation given #662 (comment). Early feedback is appreciated, but always keep in mind that things will change! 😄

EDIT: I forgot to mention that I saw #963, and I just want to say that this PR can be updated to reflect any changes should that PR get merged before this one. (It might be preferable to have that PR merged before this one).

@RossBencina RossBencina added documentation Improvements or additions to documentation build-cmake CMake build system P2 Priority: High labels Nov 25, 2024
@RossBencina RossBencina added this to the V19.8 milestone Nov 25, 2024
@RossBencina
Copy link
Collaborator

Fantastic that you are working on this, it's much appreciated. I agree that #963 should be merged first. Phil and I were hoping that we could get some feedback from CMake users before merging it.

@RossBencina
Copy link
Collaborator

We've merged #963

@generic-pers0n generic-pers0n marked this pull request as ready for review November 26, 2024 16:07
@generic-pers0n
Copy link
Author

Since #963 was merged, I'm marking this PR as ready to review.

I also have one question: should we recommend CMake over the other build systems? I can try to chip in whenever other CMake related PRs come up, although I don't consider myself a "CMake expert" but can try to help out whenever I can anyways.

(And I know it's late, but I think #963 was fine, especially considering I was operating under the assumption of its eventual merge).

@RossBencina
Copy link
Collaborator

Your offer of ongoing assistance is much appreciated.

should we recommend CMake over the other build systems?

Not for the forseeable future.

Avery King added 3 commits November 28, 2024 13:26
- Update target names
- Mention auto-download of ASIO SDK
- Mention Ninja for Linux and OS X
Copy link
Collaborator

@philburk philburk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments. We will test the current docs before merging.

@RossBencina
Copy link
Collaborator

Suggestion: in the intro section, there is an unstated assumption that the user has created an empty build directory and the CDed to it. If the user does not do that, the listed commands result in build artefacts being dumped in the current working directory. One possible fix is to add mkdir build_dir and cd build_dir to the start of each example.

@philburk
Copy link
Collaborator

I tried:
cd portaudio
cmake . -G "Unix Makefiles" -DCMAKE_INSTALL_PREFIX=/install_location

That seemed to hang.
So I did:

cd ../
cmake portaudio -G "Unix Makefiles" -DCMAKE_INSTALL_PREFIX=/install_location

But that littered my folder with a bunch of temporary cmake files. So we should warn people to make a temp directory for building.

cd portaudio
mkdir ../pabuild
cd ../pabuild
cmake ../portaudio -G "Unix Makefiles" -DCMAKE_INSTALL_PREFIX=/install_location
make

Copy link
Collaborator

@philburk philburk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then

$ make install
[100%] Built target portaudio
Install the project...
-- Install configuration: ""
CMake Error at cmake_install.cmake:41 (file):
file cannot create directory:
/install_location/share/doc/PortAudio/portaudio. Maybe need administrative
privileges.
make: *** [install] Error 1

So maybe that should be {install_location} instead of /install_location

@RossBencina
Copy link
Collaborator

Please include the section headings proposed in #951

Please put all of the ASIO related text in one place.

@RossBencina
Copy link
Collaborator

It's not clear to me when I have to blow-away and regenerate the build dir from scratch, for example @section Building Examples seems to imply that I can add a flag without reconfiguring the dir (and I tried this, and it works). Whereas @section Building Tests gives a full reconfiguration command. This is confusing.

@RossBencina
Copy link
Collaborator

On Ubuntu, running cmake did not find any host APIs. CMake reported each host API that it didn't find. But for a beginning user they are not going to be able to interpret that. Perhaps there should be a note to install some dev headers eg apt-get install libasound2-dev.

@generic-pers0n
Copy link
Author

I think I'm going to try a different approach instead: what if I rewrite the CMake documentation? Maybe from a clean slate I can better describe using PortAudio with CMake without confusing end users.

@RossBencina
Copy link
Collaborator

what if I rewrite the CMake documentation

We value your contribution and if you'd like to do that I won't stand in your way. That said, fixing these docs is a high priority and on the critical path for the next release, so a quick fix would also be fine. If you're going to try a different approach please leave this PR open and create a new one.

Whatever path is taken I think we should split the ASIO stuff onto it's own page that can be referenced by the other build instructions also. That should be a separate PR, which you are not obligated to work on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-cmake CMake build system documentation Improvements or additions to documentation P2 Priority: High
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants