michaeltyson / TPCircularBuffer

A simple, fast circular buffer implementation
http://atastypixel.com/blog/a-simple-fast-circular-buffer-implementation-for-audio-processing/
839 stars 141 forks source link

Add support for Swift Package Manager #36

Closed michal-tomlein closed 1 year ago

michal-tomlein commented 1 year ago

I can see there are already two open merge requests adding this, but I think this is the least invasive way. It is purely additive (no files moved). I tested and no code changes are required on the client side when switching to the package.

I named the target CTPCircularBuffer based on a naming convention used in Server-Side Swift packages, but a name different from TPCircularBuffer is required anyway, because TPCircularBuffer.h is not an umbrella header.

The library is named TPCircularBuffer, and #includes can use header names like before, so the only places a client of the package would see the CTPCircularBuffer name is if using module imports (e.g. @import CTPCircularBuffer;) and in Swift.

michaeltyson commented 1 year ago

Thanks, @michal-tomlein - I do like this a lot more than the other proposals. Are you sure the "CTPCircularBuffer" thing is necessary though? I believe you can specify an umbrella header so it can have a name other than "TPCircularBuffer.h", and if so that seems like the way to go.

michal-tomlein commented 1 year ago

Yes, that might be possible with a custom module map. Let me try that and update the pull request.

michal-tomlein commented 1 year ago

It does work with a module.modulemap file with the following contents:

module TPCircularBuffer {
    header "TPCircularBuffer.h"
    header "TPCircularBuffer+AudioBufferList.h"
    export *
}

However, only if I move all the sources including the module map into a subfolder, e.g. Sources/TPCircularBuffer. If I keep them in their current location (repository root), I get the following warnings in Xcode, which I assume is a bug:

ignoring declared target(s) 'TPCircularBuffer' in the system package system packages are deprecated; use system library targets instead

The only workaround I found for this is symlinking all the sources one by one. I’m not a fan of this at all (even though it does seem to work with no downside other than the ugliness of it).

Other than that, it seems like the choice is between CTPCircularBuffer and moving the sources (or at least renaming the header). What do you think?

In my opinion, moving files is more disruptive than the original PR (the only real downside there being import CTPCircularBuffer from Swift), as I have to assume at least some users of the library are using it as a git submodule with the files added to their projects directly. But if we’re moving, we should consider renaming the header instead, which would avoid introducing a minor maintenance burden in the form of a module map (or providing an actual umbrella header with the original name).

michaeltyson commented 1 year ago

Ah, blast. Okay... I'l have to think about it. I don't love the CTPCircularBuffer thing a great deal...

Thanks heaps for giving it a go! Looks like some more digging may be necessary, but I don't have the time for it at the moment.

michal-tomlein commented 1 year ago

I agree, it’s not great. In practical terms, all of the options work fine for me.

I found the offending part of Swift Package Manager source code (both warnings emitted a couple of lines later). A module map in package root was the old way of declaring a system package (now replaced with system library targets).

michaeltyson commented 1 year ago

You know what, let’s just go with it! It’ll do! Thanks heaps for your efforts!