search

readium / swift-toolkit

A toolkit for ebooks, audiobooks and comics written in Swift
https://readium.org/mobile/
BSD 3-Clause "New" or "Revised" License
223 stars 96 forks source link

Name collision caused by GCDWebServer dependency #402

Closed ettore closed 3 months ago

ettore commented 3 months ago

Describe the bug

Our app depends on the official GCDWebServer package for reasons unrelated to Readium. The Readium swift-toolkit also depends on a fork of GCDWebServer. When we build and run our app that depends on both we encounter a runtime error detailing that classes in the GCDWebServer module are implemented in 2 separate libraries, and one of the two will be used (likely randomly). Since there are no guarantees that the Readium fork will remain upstream compatible, an app that integrates the Readium toolkit must be able to reliably use both versions of GCDWebServer.

How to reproduce?

  1. Create an app that depends on the official GCDWebServer distribution.
  2. Integrate Readium Swift-toolkit.
  3. Build and run the app
  4. Observe the following runtime warning in console:
objc[96473]: Class GCDWebServerHandler is implemented in both /path/to/appbundle/Frameworks/GCDWebServer.framework/GCDWebServer (0x1051a3870) and /path/to/appbundle/AppExecutable (0x1033d8dc8). One of the two will be used. Which one is undefined.

objc[96473]: Class GCDWebServerConnection is implemented in both ... (same error as above)

... and so on for all classes in the `GCDWebServer` package.

Readium version

2.6.1

OS version

iOS 17

Testing device

IPhone 11 and simulators

Environment

macOS: 13.5
platform: arm64
carthage: 0.39.1
Build version 15A507

Additional context

This was discussed during the monthly call on 6 March 2024. We agreed that the version shipped with Readium should be renamed. I volunteered to do this change.

ettore commented 3 months ago

I thought it would be possible to simply rename the library product from the GCDWebServer package but I forgot that GCDWebServer is implemented in Objective-C and Objective-C has no namespacing. So it doesn't matter if the library is called in a different name, once the loader loads the classes they will collide. Changing the name of the repo, package or target makes no difference either. I also looked into @compatibility_alias but it doesn't really apply here because it's meant to provide an alternate / equivalent implementation, which is not the case here.

Then with this in mind I thought that every single symbol defined in readium/GCDWebServer would need to be renamed, but I am actually only seeing a warning for the following symbols when I run my app:

GCDWebServerHandler GCDWebServer GCDWebServerConnection GCDWebServerBodyDecoder GCDWebServerBodyEncoder GCDWebServerGZipDecoder GCDWebServerRequest GCDWebServerGZipEncoder GCDWebServerResponse GCDWebServerDataRequest GCDWebServerFileRequest GCDWebServerMultiPart GCDWebServerMultiPartArgument GCDWebServerMultiPartFile GCDWebServerMIMEStreamParser GCDWebServerMultiPartFormRequest GCDWebServerURLEncodedFormRequest GCDWebServerDataResponse GCDWebServerErrorResponse GCDWebServerFileResponse GCDWebServerStreamedResponse

so for instance GCDWebServerCompletionBlock or GCDWebServerGetMimeTypeForExtension etc do not generate a warning. I'm not sure why.

Do you have any preference for the prefix to be used? Using GCDWebServerRequest as an example:

  1. R2GCDWebServerRequest
  2. ReadiumGCDWebServerRequest
  3. R2WebServerRequest
  4. ReadiumWebServerRequest

Given that we have a GCDHTTPServer class in swift, maybe we should keep the GCD part of the name, so either (1) or (2) ? I am open to other ideas.

mickael-menu commented 3 months ago

I have a preference for the Readium prefix, or maybe RD if we really need something short. The R2 prefix is deprecated nowadays.

ettore commented 3 months ago

Readium prefix it is!

we should probably also rename the repo to ReadiumGCDWebServer for consistency? I don't have the power to do that but I did it on my fork and it's a couple clicks change. If you agree i can add a PR on swift-toolkit to take care of this too.

mickael-menu commented 3 months ago

As it's hosted on https://github.com/readium/gcdwebserver, I think it's fine. Our convention so far is to use the organisation name as part of the name.

readium/swift-toolkit -> Readium Swift toolkit readium/gcdwebserver -> ReadiumGCDWebServer

ettore commented 3 months ago

I ended up modifying ALL the symbols. It just looked too weird to have some symbols prefixed with Readium and some others not. But as discussed in the PR above, I had to leave the module name as GCDWebServer, so we'll need to keep importing it as import GCDWebServer. This is the only inconsistency, I guess. If we want to use import ReadiumGCDWebServer we'll need to change the name of the repo (see PR).