search

bitfieldaudio / OTTO

Sampler, Sequencer, Multi-engine synth and effects - in a box! [WIP]
https://bitfieldaudio.com
Other
2.63k stars 144 forks source link

Move External Dependencies To CPM #179

Closed brianmichel closed 4 years ago

brianmichel commented 4 years ago

We'd like to move all of our dependencies to CPM to more easily manage them.

topisani commented 4 years ago

https://github.com/OTTO-project/OTTO/tree/feature/use-cpm-for-external-dependencies I made a few changes here, does it look ok to you?

I cant get nlohmann_json to work for some reason, maybe you can done

I thought external/CMakeLists.txt made more sense, but not really sure.

I'd prefer to have all the single header includes we currently have added to external/include as CPM packages as well over time.

brianmichel commented 4 years ago

I made a few changes here, does it look ok to you?

Yup, that looks fine to me. I can close this PR if you want to merge that instead. Sorry for the delay.

Yeah I think putting these definitions into the external/CMakeLists.txt makes more sense. You can see in my latest commit I realized that putting it all the way at the top level didn't really make sense, but I think you are correct in having these be in the external/ folder to create a library out of all the things in there.

I can move the 1 off files that exist into the external/include if you want me to do that in a different PR, just let me know how you'd like to proceed.

topisani commented 4 years ago

I just pushed another commit to that branch - I think just resetting this branch to that one would do the trick, I believe i have all the same changes you have in the last missing commit.

For the single header includes, id rather use CPM packages than the external/include folder. This makes it easier for us to update the packages and helps to make sure we don't "accidentally" change something in our local copies.

I also changed CPMFindPackage to CPMAddPackage, to make sure we don't use locally installed versions of these dependencies if they exist. This is to help make sure builds are consistent and reproducible. I have left glfw as CPMFindPackage, since it's only used on desktop, takes a long time to build, and some systems use a special build of it.

Sorry for steamrolling your PR btw :wink:

topisani commented 4 years ago

I think im gonna merge my feature/use-cpm-for-external-dependencies, its a nice change as is, and i have to add some dependencies for another feature anyway, so id rather base that on this. But lets just keep this PR open (and rebase it when the other branch is merged), and if you want to turn all the external/include single-header libraries into CPM packages as well, just do it on the same PR/branch and I'll merge it again afterwards

brianmichel commented 4 years ago

@topisani sounds good to me, thanks!

topisani commented 4 years ago

Merged and rebased to develop - it can be reopened whenever you push new commits