-
Notifications
You must be signed in to change notification settings - Fork 167
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
CMake improvements #69
Conversation
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.
This is a whole bunch of unrelated changes. You should probably PR some of these separately. You should also understand why things are the way they are before you decide to change them.
@@ -19,7 +19,7 @@ test: | |||
######## | |||
|
|||
lib_LTLIBRARIES = src/libsamplerate.la | |||
include_HEADERS = src/samplerate.h | |||
include_HEADERS = include/samplerate.h |
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.
What???? Its fine (desirable even) to keep the header file in src/
and during installation put it in /usr/include
.
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 disagree here. Could you tell me your reasoning why it would be "even desirable" to keep the public header in src/
?
What proved advantageous was to separate out public and private headers already in the source tree. Public headers (ones installed) go into include
private ones into src
. This has 2 advantages:
- Tests and examples can be build against the source tree. See https://github.com/erikd/libsamplerate/pull/69/files#diff-af3b638bc2a3e6c650974192a53c7291R70 The binaries linked against libsamplerate don't get to include the private headers. This avoids accidentally using a private there which would break on "real installs" and hence is an additional safe guard.
- Consumers can add this git repo as a submodule and simply use it with
add_subdirectory
(motivation for this PR) They get the same includes (only public ones) as if using the installed library. This avoids having to install this in a separate step (and with a possibly mismatching compiler setting) which is a big bonus especially for Windows where dependencies are just a nightmare.
Sure. Which ones would you have separately? Most of the things is very related so I just put them in separate commits. What I can think of is the math/clip fixes and the src-evaluate fix although the cmake part depends on this one and I don't really like dependent MRs. So just tell me what you'd like in a separate MR.
I do. I'm proficient in C++ & CMake but might miss some parts on C and autotools. So I got good reasons to change things but the solution might not be optimal. Let's talk about the individual changes to make this great project even better. |
a5fdc05
to
3d5f63e
Compare
I updated this PR to remove changes that were moved into other PRs and addressed comments. Anything else that should be factored into its own PR? Would be great to get this in, so projects using this are not impacted by dependencies for tests (e.g. searching for FFTW, libsndfile...) |
1b3d5b1
to
c10c819
Compare
Move checks to the folders where they are required Remove unused checks Move samplerate.h to include folder (check for self-containedness)
unistd.h is only required in tests/examples, so add it there The def file is only required for shared builds, so add it there Follows CMake best practices to use target_ functions
They are included as `#include "src_config.h"` so they should be in the same folder As an alternative: Add the `src` folder to include_directories but that violates the "clean includes" principle and fails to test that the public libsamplerate headers are self-contained
I like it.
Not sure. It is common practice to have one
Not sure. It seems to me that these are not such large projects to divide them into subdirectories. We have moved away from recursive makefiles and CMake files in libsndfile, to avoid a bunch of files here and there, it would be strange to have a project with subdirectories here. |
Sure but that config.h usually contains only macros required to make sure only features actually contained in the binary so consumers don't use stuff that isn't there. In this case (IIRC) there is more. Especially mixing in stuff that is only required for the tests is IMO reason enough to split it. Or to remove the test-only stuff and use plain preprocessor definitions on command line
It isn't that much about size but about organization. E.g. some dependencies are only required for tests or examples. With the 1-folder approach the boundaries are hard to define, with subdirs it is easy. (Example: With CMake I see it searching for FFTW and that might confuse new people into thinking FFTW is required for this library to work or provide some benefit) |
Ok, let's start from |
Extracted from libsndfile#69
I suggest another option - why not move the checks required for the library and tests to their subprojects, and call |
Given that config.h is not installed this is certainly an option. The downside I see is that this way it can't be tested whether the main header is "clean", i.e. doesn't rely on anything from config.h. Additionally it can't be tested that the main binary doesn't rely on anything from the tests, i.e. something (a dependency search) was wrongly added in the test/example folder but should have been in the main instead. But given that there is only a single header this can be verified manually and your approach is quite easy to do. |
This is too much even for a paranoid person like me 😄 |
I looked over the CMake files and implemented a couple of enhancements and fixes.
This should now allow to use this project as a submodule from CMake with
add_subdirectory
allowing broader use without pulling in unnecessary dependencies or checks (FFTW, sndfile...)