Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Name collision caused by GCDWebServer dependency #402

Closed
ettore opened this issue Mar 19, 2024 · 5 comments
Closed

Name collision caused by GCDWebServer dependency #402

ettore opened this issue Mar 19, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@ettore
Copy link
Contributor

ettore commented Mar 19, 2024

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 ettore added the bug Something isn't working label Mar 19, 2024
@ettore
Copy link
Contributor Author

ettore commented Mar 19, 2024

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
Copy link
Member

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

@ettore
Copy link
Contributor Author

ettore commented Mar 20, 2024

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
Copy link
Member

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
Copy link
Contributor Author

ettore commented Mar 20, 2024

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).

ettore added a commit to ettore/swift-toolkit that referenced this issue Mar 20, 2024
This depends on the following PR: readium/GCDWebServer#12

Together these solve issue readium#402
ettore added a commit to ettore/swift-toolkit that referenced this issue Mar 25, 2024
This depends on the following PR: readium/GCDWebServer#12

Together these solve issue readium#402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants