-
Notifications
You must be signed in to change notification settings - Fork 323
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
Remove redundant build systems in favor of CMake #434
Conversation
a81695f
to
fafb732
Compare
09ac627
to
b63b8a1
Compare
−87,507 lines of code 🚀 |
@Holzhaus could you review bindings/cpp/CMakeLists.txt? |
LGTM. |
c5f8d7d
to
d53fa51
Compare
Very nice improvement to simplify package management. Thank you for the work! |
a134099
to
13a4f91
Compare
4659567
to
67b917d
Compare
@Be-ing Thanks for these great contribution to the CMake build. These improvements should make CMake much easier to use for PortAudio. But, unfortunately, this PR cannot be merged because it removes build systems that other people are currently relying on. But I hate to lose the great work you have done on CMake. Would it be possible to improve CMake without In general, large PRs are hard to merge because one unacceptable change can make it impossible to accept the others. So unrelated changes should always be in separate PRs. Also please postpone changes like moving all the C++ prefixes from .cpp to .cxx. We have talked about moving the C++ bindings to another repository. We can make naming changes in that other repository. As CMake is improved we can start encouraging more people to use it. I tried CMake the other day in a different repo and was pleasantly surprised at how easy it was to generate an XCode project! We also use CMake for the native code in Android Studio and it works well. I see you updated the CMake docs, which is great. We should document how to generate the XCode and MSVC projects using CMake. That will help people more migrate to CMake. Once everybody loves CMake as much as you do then we can consider removing the other build systems one-by-one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment in Conversation.
It would be possible, but it would be a waste of effort that I have no interest in doing. Along the way I reorganized some files to make it easier to get them all building with CMake. I am unwilling to both clean up the file structure and maintain multiple build systems. |
@@ -3,7 +3,7 @@ | |||
|
|||
// --------------------------------------------------------------------------------------- | |||
|
|||
#include "portaudiocpp/System.hxx" | |||
#include "System.hxx" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I think this would break user applications. Do we need to put the headers back in bindings/cpp/include/portaudiocpp
or is there a way we can keep the headers in bindings/cpp/include
and keep the #include
s in the C++ code referring to "portaudio/headername.hxx"?
Phil wrote:
OK then we will probably have to just close this PR. It is a shame to lose all this great work. Perhaps someone else would be willing to cherry-pick some of your improvements to CMake and put them in a separate PR. |
I am not saying it should never get done. I even outlined the steps we can take to make it happen in future PRs. Are there any technical reasons why they have to be all in the same PR? |
You did? I'm not very interested in working on this without a specific roadmap for removing redundant build systems. My experience with Mixxx has shown me that maintaining multiple build systems is not very practical. Unless every supported build system and platform is run on CI with all options enabled -- which also takes work to set up and maintain, which I will not do -- some will break. It is already the case that CMake does not support the C++ bindings, most tests, any examples, nor the OSS host API. We initially planned to support both SCons and CMake for Mixxx 2.3, but maintaining both became such a hassle that we removed SCons as soon as we got CMake working on all supported OSes. My idea with making one last release with the macOS fixes using the old build systems is that nobody would be forced to switch to CMake immediately to continue using PortAudio. There would not be an urgent need to upgrade if someone doesn't want to bother learning to use CMake. Historically PortAudio releases have been years apart so I'm doubtful many people will complain about not having the newest code anyway.
No, just practical reasons. Making changes to the file organization and the CI scripts while maintaining multiple build systems is not something I'm interested in doing. I worked on those changes together with CMake, but I realized that I'd either have to maintain the other build systems or remove them. So I rebased to put the removal of the other build systems at the beginning of the Git history to keep each commit working. Also, considering that a critical bug fix (#356) has been sitting unmerged with no response for over two weeks, I didn't want to wait for weeks or months for smaller PRs to get merged first before going ahead and improving CMake. |
By the way, my initial motivation for working on this was to get PortAudio building with ASIO support with vcpkg. The vcpkg package uses CMake already. When I took a look at the code I realized there was a lot of clean up to do first. |
It can take a while to get PRs approved. And you may have noticed that bigger PRs take exponentially longer. So a very large PR that has controversial changes, like removing others peoples build systems or massive refactoring, just won't fly. I understand that moving code can require updates in all build systems. So removing unused build systems before moving code makes sense. But we don't have to do everything in one PR. There is some good stuff in this PR that we don't want to lose. I notice that the most of the changes in this PR are related to C++. We have been thinking of moving the language bindings to other repositories. Bindings for Python, Rust, C#, Lisp are already in other repositories. So support for C++ in the PortAudio CMake may not be necessary. Maybe we could break these changes into smaller more manageable pieces.
|
*.obj | ||
|
||
# annoying files | ||
**/.DS_Store |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are still annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philburk These artifacts are specific to your platform, not to the PortAudio project. Therefore, I would argue, you should try to handle them on your system, not in the PortAudio repo. I recommend setting up a global Git ignore file with core.excludesfile
in your home directory.
See https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration#_core_excludesfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be created on any Mac. If any Mac user does not configure their global config then we get their files. This is a simple and harmless solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say it is "mostly harmless".
If any Mac user does not configure their global config then we get their files.
I don't think this is a problem in practice.
It is always a possibility that contributors making a PR commit files they didn't intend to. This should be caught in code review.
I think inadventently committing .DS_Store
files may indeed be a problem when doing the initial commit, but I think it is highly unlikely to ever happen in an already established project.
I have never ever seen a PR where this happened.
And if this does actually happen (or you just want to be extra sure that it doesn't ever happen), my advice from above (#434 (comment)) should be added to the contributing guidelines. This is real good advice (if I may say so myself) that it worth spreading!
I agree with @philburk. There are way too many disparate changes here for a single PR. This needs to be broken up into many smaller PRs -- this aids with review. I recommend starting with a PR that does one thing:
I also agree with @philburk about moving the C++ binding to a separate repo -- don't bother supporting that in the CMake file. |
I agree with moving the C++ and Java bindings to new repositories. I added CMake support for the C++ bindings because the autotools support depended on the autotools files for the C library, so once I removed autotools for the C library the C++ bindings needed a new build system.
These are trivially easy. Just run How about this roadmap?
|
Can you elaborate on this? I thought they were all manual tests that require real hardware. If you want to keep some separation between them in the source tree, how about moving the |
A strong reason to remove all other build systems is because downstream CMake projects can't rely on the autogenerated CMake config files for |
And the related bug report on the FFTW bug tracker: FFTW/fftw3#130 (comment) |
The programs in "qa" may not be "pure unit tests". They do require an audio device. But they are automated tests that check results and return a pass/fail. They are intended to be run as part of the QA/release process. We need more and better QA tests. The programs in "tests" just do random things and require more user involvement and listening. |
Regarding your roadmap above. I think it is a road we can start down. Removing the other build systems will need to be considered based on community feedback at the time. Also we should keep them in CI if they are in the repo. |
As discussed on the mailing list, maintaining redundant build systems is a pointless waste of effort that almost inevitably leads to inconsistencies and frustrated users. CMake can generate MSVC project files and Unix Makefiles. There is no need to keep separate build systems for those.
This PR also makes the C++ bindings, all the tests, and all the examples buildable with CMake.