-
-
Notifications
You must be signed in to change notification settings - Fork 70
Pluginval as dependency #141
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
base: develop
Are you sure you want to change the base?
Conversation
89740ac to
1b833be
Compare
dcbe3cf to
db9523e
Compare
|
Am I supposed to be reviewing or merging this? You might notice I switched to CPM for a few other dependancies on develop. Can this branch be pulled in line with that? |
|
@drowaudio I've been using this branch for a year as a dependency of my synth but never got the example plugins running in CI. I'll need to get that going and resolve recent conflicts before it's mergable. |
|
Ah ok. Poke me if/when you get to that. I might switch to juce via CPM at some point soon anyway... |
dc9e598 to
b596c78
Compare
| with: | ||
| path: ${{ env.PLUGIN_CACHE_PATH }} | ||
| # Increment the version in the key below to manually break plugin cache | ||
| key: v7-${{ runner.os }}-${{ env.JUCE_SHA1 }} |
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.
It's harder to grab this SHA1 when JUCE is coming from CPM. I looked at the history and decided this feature is unnecessary:
- In most cases it's fine to bump JUCE without clearing cache
- When bumping JUCE, the cache version can be manually bumped if desired
|
@drowaudio I merged in develop, aligned CPM, fixed the plugin tests and added tests specifically for this new type of usage (as a dependency of a JUCE project). I have been using pluginval this way, but there is one drawback for you/us as maintainers, which is noted in the description. I won't be offended if you don't want to take that additional burden on. |
|
I think if you're using it this way you have two options:
It's basically impossible to do anything else for libraries that don't maintain a stable API and ABI (which is very hard to do). |
|
I've been maintaining JUCE 7/8 compatibility with some other open source projects and I would classify it as "mildly annoying" (for example the JUCE 8 font changes). But yeah, I don't think pluginval itself needs to make any big promises. I haven't tested JUCE 7 and I noted in the README changes that it would be JUCE 8 only. |
Closes #137
This improves support for pluginval as a CMake sub-project in a JUCE project.
After this PR, all one needs to do is
add_subdirectory ("modules/pluginval")to a plugin project's CMakeLists.txt.PLUGINVAL_FETCH_JUCEoption (no longer needed, superseded by not being top level)To review, the benefits are:
Note:
Technically, this feature adds an additional maintenance burden to pluginval — before this change, pluginval only had to care about working for the single JUCE version it came bundled with. Now, it would need some preprocessor
#ifstatements to maintain some backwards compatibility with older JUCE versions (like I did in this PR for JUCE < 8.0.11)