Update flucoma-core for hisstools header-only #265
Conversation
|
It would be wise also to clang-format any relevant files. |
tremblap
left a comment
tremblap
left a comment
There was a problem hiding this comment.
The SC repo doesn't compile - loads of:
clang: error: linker command failed with exit code 1 (use -v to see invocation)
so there must be a few changes needed in wrappers. please advise.
sclang_format was done then undone for most of it.
|
(and thanks for doing this for @fearn-e I reckon :) ) |
|
I am far from a CMake specialist, but commenting lines 136-138 in CMakeLists.txt in the SC wrapper seems not to sort it. Am I supposed to find other problems? Should I do similar unblinking (if that is what I do there) in the other 3 repos? |
|
I'm also commenting line 45 of MakePluginSource.cmake - it seems to behave better. Can you please have a check in the SC repos (to start with) to see if that is the only problem @AlexHarker - then we can troubleshoot the other 3 repos (max, pd, cli) and do PRs in all 4 wrappers and then we have a potential merger. |
|
(it compiled at last, running the SC UTs in MacOS now) |
|
ok this is working perfectly now. I'll wait for approval of code on the other repos - I'm pushing them now |
|
it seems only SC needed some manual linking. @weefuzzy am I wrong? The other 3 CCE wrappers compile... so far :) |
|
You've only given the most outer error (linker fail) but it's the detail that I'd need - what was/is the symbol that fails to link or was it that it expected a lib that didn't exist? I'm not a cmake expert I don't know what has happened - there aren't links to the lines you cite above (which would have been helpful) but I'll go and look and see if any of this makes sense of why it would now link... |
|
OK - thanks - the PR for sc clarifies this - those would be required changes (removing references to HISSTools_FFT in cmake in the dependent repos). I didn't experience any required changes in the max cmake, but we would need to check in all 4? repos for each CCE plus command line |
|
already there :) Max compiled, Pd compiled, cli is failing with an easy one : |
|
Off the top of my head I can't think of any reason flucoma-sc would need special treatment for this. As @AlexHarker says, we'd need to see the error diagnostics to to be more helpful. If you're using FWIW, I'm surprised that flucoma-cli is building (?) without further changes, given that it uses all the hisstools audio file stuff. |
|
SC needed -fPIC - PA has dealt with the changes there. The CLI shold break (pd and max are fine) but is it pulling in HISSTools separately to core (and therefore on its own version). That repo needs code updates as @weefuzzy rightly points out - I did quick searches of the repos on GitHub to confirm all the above. |
|
Oh good to know that the CLI is failing (missed that above) - @tremblap - you can try to follow similar changes to the PR here, but if you struggle I may need to do that one, as there are also changes to snake case and method renamings (plus the namespace) that will mess it up. describe.cpp or the single tests template file in this PR and the ones to look at. |
|
Let's fix CLI before we merge. I'm trying to find all the new spiel |
|
describe is good but only has #include <AudioFile/IAudioFile.h> there is also #include <AudioFile/OAudioFile.h> in the wrapper, plus calls to BaseAudioFile there are only 6 instances of HISSTools::* so if you could look at it now that would be ace (and allow me to adapt the other 2 examples in core in a PR that will break) |
|
it would be good if CLI wasn't calling its own version of it too - just so we are sync now (it might diverge again in the future but hey, sync is good) |
|
No the CLI pulls it's version from the cmake in core, so it is aligned (cmake builds are separate, however). I'm updating the CLI now, but it looks like the out file code has not been tested anywhere so I'd advise some sanity checking (at least through listening) to the output of the CLI. |
|
ok it all works now thanks for this |
|
Thanks for providing the testing to improve/sanity check header-only hisstools library! |
3 participants
This is a PR that updates code and CMake files to deal with HISSTools_Library in header only format, which will be the future of the library.
The changes in the repo have been tested for compilation, but it would be advisable to run some audio tests for sanity in the SC repo and also do some audio testing. In terms of impact on the flucoma ecosystem this pulls in the header-only version of the FFT, which is edited from the previous version - there is no specific reason to expect any issue, but any obvious error is likely to turn up in basic audio testing pretty easily.