Fix build #176
Fix build #176
Conversation
There was a problem hiding this comment.
@Barabas5532 Thanks for the PR. It looks interesting, but I want to chat to @joefocusrite before I comment to far. I like the idea of adding linux support for sure, but I would want us to make sure we can run it up on CI (our CI does support linux runners, so that's doable) - but we might need to update any example projects to make sure they compile properly too. If you can help us with those areas that would be appreciated.
Ill get back to you ASAP once Ive chatted with Joe
There was a problem hiding this comment.
Firstly, thanks so much for taking the time to put together this PR!
If I understand the problem correctly, the way it has been implemented now means that the same header file could be compiled with different compilation flags depending on the library's consumer. This could lead to all kinds of problems, including multiple implementations of the same function.
So... I agree that this is a problem that we need to fix!
I have a couple of reservations about this solution.
Firstly, I haven't tried it, but I think the library/tests won't build on their own without the example app. It feels like it should be possible to build the library or the tests without having to build the example app.
Secondly, I'm not sure I've seen the pattern of supplying an extra module from the consumer of a library before in CMake. It would be nice if all consumers had to do was link to focusrite-e2e.
I think the root cause of this is JUCE's implementation using interface libraries. As I think some people mentioned in the forum post that you shared, it makes it difficult to build a library that depends on JUCE, when other libraries might depend on you.
Ideally, this library would simply link to JUCE modules, and consumers could change the behaviour of JUCE by changing compile flags in their projects.
| include/focusrite/e2e/Event.h | ||
| include/focusrite/e2e/Response.h | ||
| include/focusrite/e2e/TestCentre.h | ||
| add_library (focusrite-e2e STATIC |
There was a problem hiding this comment.
I read somewhere (I'll try to dig it out) that it's best practice not to include STATIC if you don't know who is consuming your library. This means that the consumer can change libraries globally with BUILD_SHARED_LIBS.
I'm not sure where I read this, so I'm happy to include STATIC if there's a good reason.
Describe what your code does
This is a breaking change, and all users need to update their CMake files to provide the
focusrite-e2e::juce_modules, which is used to link to JUCE using the same compile definitions used by the user application.Is this pull request to fix a bug?
Fixes #175. The change is based on advice from the forum: https://forum.juce.com/t/compile-juce-modules-to-static-libraries/43255/2. I use the same method in my own project where I have DSP libraries that depend on JUCE and an application that depends on those libraries.
Is this pull request to extend the functionality of the codebase?
Is this a feature you are proposing? If so, please describe why it is required, what it does etc.
Is the code tested?
We require a high level of code test coverage - please make sure that you have run tests on your code. You can learn more about running the tests in the readme.md in the root of the repository.
I have ran the build in my machine on Linux. I expect the rest can be covered in CI and by reviewers. At the moment I can not test on Windows or MacOS, but I can set up Windows if required. The changes should be compatible with all platforms.
I have compiled and tested the example app using the following commands:
By submitting a pull request to this repository you are agreeing to assign the copyright on your submitted code to Focusrite Audio Engineering Ltd. Please check the box below to show you accept these terms.