search

Bouke / HAP

Swift implementation of the Homekit Accessory Protocol
https://boukehaarsma.nl/HAP/
MIT License
366 stars 50 forks source link

Migrate CLibSodium to system library target #85

Closed Bouke closed 5 years ago

gbrooker commented 5 years ago

It would be preferable on macOS not to have to install a third party library at system level just to compile. Best keep this library to the local directory.

Bouke commented 5 years ago

I'm not sure what you mean. libsodium has been a dependency for quite some time, even on macOS. This PR only changes how that library is referenced, moving from a separate package to an inline target.

gbrooker commented 5 years ago

If it is installed as a systemTarget, won't that mean it will be added to /usr/lib or /usr/local/lib ?

If an end user runs an app using HAP, they are unlikely to have this third party library installed. Therefore it is better to treat this library just like CQRcode, and bundle it with the other dependencies.

I force my builds to use a libsodium.dylib from the local directory and copy it into the app bundle for distribution.

Bouke commented 5 years ago

I suspect this change doesn't require any changes to the consumer. The library will probably still be picked up if provided alongside the executable, same as it is now.

Bouke commented 5 years ago

So the current reference is what's called in SwiftPM a "system library package"; which is essentially a git wrapped pointer to a system library. There's a new thing called "system library target". Which is the same thing, however doesn't require a git wrapped package. So the upside to this is that using such a target requires less boilerplate.

gbrooker commented 5 years ago

I can understand its use on linux, where /usr/local is a common space, however on macOS it really would be much preferable if this library dependency can just be built locally, and not installed in a system directory. sodium is not a common library used by the system, so it should just be treated as part of this package build and not be allowed leave files all over the system, outside the build folder...

Bouke commented 5 years ago

Are you proposing we drop the dependency on libsodium?

gbrooker commented 5 years ago

Ideally, yes

Bouke commented 5 years ago

Well I don’t see that happening anytime soon. If binary distribution is your concern, just bundling the library with your app is the correct way IMO.

Op 2 apr. 2019 om 02:11 heeft Guy Brooker notifications@github.com het volgende geschreven:

Ideally, yes

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

gbrooker commented 5 years ago

Indeed I do. I just think that it is not good practice, at least on macOS, that building a swift library requires code to be installed in system directories. Everything should be self contained in the build directories.

Bouke commented 5 years ago

From my experience it's pretty common to have certain system libraries available. Most of those however are already preinstalled, think libxml and libcurl for example. When developing applications its pretty common in my experience to rely on system libraries, even libraries that aren't available by default. That's what all the linker options are for in Xcode after all, and why pkg-config exists.

App bundles however cannot assume certain additional libraries to be present, and asking the end-user to install those libraries using e.g. homebrew is not a good user experience. Hence the need to bundle those with the app distribution. When distributing an application you, the distributor, should ensure the bundle includes everything that's needed to run the application: image assets, storyboards etc, as well as non-default libraries.

gbrooker commented 5 years ago

Totally agree, but libsodium is rather esoteric compared to libxml or libcurl. I'm sure you agree that the CQRCode library should not be placed in system directories.

There is no need or reason to install libsodium in a system directory on macOS to build or run HAP (I don't have it installed at system level on my own build machine), and I suggest that the default PackageManager setup should not require that either.

Bouke commented 5 years ago

Interestingly on a side-note, swift-nio-ssl decided to vendor BoringSSL. The down-side of vendoring would be the maintenance burden of keeping the vendored version up to date.