Adopt swift-log

[kaascevich]

This at least partially solves #267 by adopting swift-log. The logger is initialized immediately after calling App.init(), such that users can use LoggingSystem.bootstrap(_:) in their app's initializer to redirect messages wherever they want. (This does mean that the logger is unavailable in extractSwiftBundlerMetadata(), since that's called before App.init().)

This PR also adds a small documentation article detailing how to customize the logger, and mentioning the caveat that doing so outside of the app's initializer will not work.

[stackotter]

Thanks for these changes, and all of the accompanying documentation! I've requested a few formatting/wording changes.

Loading Swift Bundler metadata is something that I can see going wrong and being important to be able to debug. I had a quick think about the possibility of introducing a new static method to the App protocol (with an empty default implementation) to house logger customisation. But I think it's likely that some logger implementations would need information from the app's metadata (such as a Sentry client token). I think that the best solution would be to store the metadata loading errors somewhere and then dump them to the logger as soon as it gets created. Would you be happy to implement that?

[kaascevich]

Okay, so. Whenever Swift 7 decides to come around, import declarations will be internal by default. This presents an issue for this implementation, since the user needs to have access to the Logging library in order to set the backend. Of course, this issue is trivially solved by:

#if compiler(>=6.0)
    // deliberately use `public import` here to allow the user access to
    // `LoggingSystem`
    public import Logging
#else
    // before 6.0, this behaves like `public import`
    import Logging
#endif

However, right now that results in a warning, because we don't use any of Logging's APIs in public declarations. As far as I can tell, there isn't any way to get rid of this warning without either:

• marking the import as internal (which is the exact opposite of what we want)
• actually using it in public API

So here's what I'm thinking. Instead of asking the user to set the backend in App.init(), we add a new requirement to App:

var logHandler: LogHandler { get }

(Logging calls backends "log handlers"). This requirement will have a default implementation that simply uses the built-in StreamLogHandler to log to the console. After the Swift Bundler metadata is parsed and the app struct is initialized, App.main() will call LoggingSystem.bootstrap(_:) with the value of this property.

With this mechanism, not only do we solve the problem of not using Logging in public API by adding this requirement, but we also give ourselves the freedom to use whatever backend we want by default. (With the current implementation, we can't use anything other than the default StreamLogHandler, because LoggingSystem.bootstrap(_:) can only be safely called once).

[kaascevich]

I noticed that Logger (the struct) has an initializer that takes a log handler, so we don't even need to use bootstrap(_:). With that, SCUI's logging system can be entirely decoupled from those of other libraries users may wish to use.

[stackotter]

Looks really good now. I've just requested some documentation wording changes, but the code looks great.

[stackotter]

Just a couple more requests (your throwing changes came through while I was doing my previous review)

[stackotter]

Okay, so. Whenever Swift 7 decides to come around, import declarations will be internal by default. This presents an issue for this implementation, since the user needs to have access to the Logging library in order to set the backend. Of course, this issue is trivially solved by:

#if compiler(>=6.0)
    // deliberately use `public import` here to allow the user access to
    // `LoggingSystem`
    public import Logging
#else
    // before 6.0, this behaves like `public import`
    import Logging
#endif

Are you intending for this to reexport the Logging API so that user's don't have to import it themselves? That's different to public import (which just lets us use the logging API in our own public APIs). I think we can just import Logging normally (treating it as internal), because any users who want to customise logging will likely already have swift-log as a dependency of their own project anyway and should be able to just import it.

[stackotter]

Sorry, just one more small change due to a logging statement that I accidentally left in #278, but other than that this is ready to merge.

[kaascevich]

Are you intending for this to reexport the Logging API so that user's don't have to import it themselves? That's different to public import (which just lets us use the logging API in our own public APIs).

No, public import was deliberate here. What I was going for was for users to not lose access to Logging entirely once the internal-by-default change is made, since @_implementationOnly import / internal import don't allow the package's clients access to that dependency. (At least, that's my understanding of the feature -- I might not be correct on that.)

[stackotter]

If my understanding is correct, internal import wouldn't stop someone from adding Logging as a dependency of their own package and importing it manually. What scenario would internal import stop, and public import restore? We may be talking about different scenarios

[kaascevich]

If my understanding is correct, internal import wouldn't stop someone from adding Logging as a dependency of their own package and importing it manually. What scenario would internal import stop, and public import restore? We may be talking about different scenarios

I think that's true, but I'd personally like to spare people the hassle of doing that and having to match their version of swift-log with the one we use. Either way, though, it's not really a problem we'll have to worry about for a while yet.

[stackotter]

Looks like the swift-log version might have to be 1.6.0 not 1.7.0 (the 5.10-based CIs have failed again)

[kaascevich]

Okay, so for whatever reason the CI seems to still be trying to use 1.8.0 even though I very explicitly set the swift-log version to 1.6.4...

[kaascevich]

It seems Package.resolved wasn't updated properly, that might be it.