search

apple / swift-log

A Logging API for Swift
https://apple.github.io/swift-log/
Apache License 2.0
3.48k stars 284 forks source link

Explicitly set the type of the SPM library to be dynamic. #289

Closed jon-mccormack closed 7 months ago

jon-mccormack commented 7 months ago

Explicitly set the type of the SPM library to be dynamic.

Motivation:

When a binary target-type framework consumer specifies swift-log as a transitive dependency, the swift-log symbols that the framework depends on are statically linked in that framework's binary. As a result, consumers of said framework resolve the swift-log transitive dependency which is made available as a dynamic library at runtime - meaning duplicate symbols are present in two (or more) different dynamic libraries causing "Class is implemented in both..." warning logs.

Downstream app warning logs at runtime: image

Proof of swift-log symbols being statically linked in my binary framework consumer: image

Modifications:

To resolve this, I have explicitly set the SPM library type of swift-log to 'dynamic'. This means binary target-type consumers link swift-log dynamically.

Result:

Binary target-type framework consumers of swift-log will get the same experience as regular source-code consumers of swift-log as swift-log will be linked dynamically rather than statically.

I've tried and tested this fork in an app which resolves my binary target-type framework through SPM and swift-log (as a transitive dependency) and I no longer get those warnings.

Proof of swift-log symbols no longer being statically linked to binary target-type consumers after my change: image

yim-lee commented 7 months ago

@MaxDesiatov @neonichu Do you have any recommendation here?

neonichu commented 7 months ago

Not sure how the originator is planning to distribute swift-log with their XCFramework, typically the way to resolve this is to rename symbols. Relying on clients to bring their own copy of swift-log is not something that'll work in the general case.

MaxDesiatov commented 7 months ago

I don't think this should be merged. The status quo with the default .auto setting in this package is the right approach as it provides the flexibility to link it either statically or dynamically. Restricting this package only to dynamic linking will break a lot of things for packages depending on swift-log and degrade performance when compared to the currently available static linking option.

jon-mccormack commented 7 months ago

Hi all, thanks for spending the time looking at this! I should've clarified i'm using the SwiftPM client through XCode for developing an pre-compiled XCFramework for iOS (using "iphoneos" and "iphonesimulator" SDKs). Sadly, AFAICT, this means I have very limited control when resolving SwiftPM dependencies and deciding whether to link them statically/dynamically - under these conditions, XCode appears to link the necessary swift-log symbols statically within my framework's binary.

I can see how what i've proposed isn't ideal and would be a breaking change. I think this boils down to lack of SwiftPM configuration in XCode.

The only solution I think for people in a similar scenario as me is to just deal with the class implemented in both warning at runtime. The other option would be to update the Mach-o (linking) type of my binary framework to be static - but I think in this scenario, if a consumer of my framework also consumed another framework who statically linked swift-log symbols, then they'd get a compile-time linking error.

MaxDesiatov commented 7 months ago

The only solution I think for people in a similar scenario as me is to just deal with the class implemented in both warning at runtime.

There's no way to deal with duplicate symbols other than avoiding them in the first place. IMO in most cases those should be errors and not warnings. Loading multiple symbols with the same name is undefined behavior causing hard to diagnose and possibly non-deterministic bugs.

jon-mccormack commented 7 months ago

There's no way to deal with duplicate symbols other than avoiding them in the first place. IMO in most cases those should be errors and not warnings. Loading multiple symbols with the same name is undefined behavior causing hard to diagnose and possibly non-deterministic bugs.

Yeah, I agree with this completely. I still think, though, that this boils down to a lack of support for binary dependencies in SwiftPM/XCode. Perhaps I'll have to work around having any dependencies for my binary framework altogether, given the unsafe, non-deterministic nature of loading one of many class definitions at runtime.

Thanks everyone for your input on this! I'm happy for my PR to be rejected, given our collective findings.

MaxDesiatov commented 7 months ago

this boils down to a lack of support for binary dependencies in SwiftPM/XCode

No debating that, but then it's worth finding an existing issue or filing a new one on the SwiftPM repo for CLI swift build use cases, and then a separate one on https://feedbackassistant.apple.com for Xcode, since implementations for these two are separate.

yim-lee commented 7 months ago

Thanks folks for the discussion. Closing this PR.