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

Add 'Readium' prefix to all symbols #12

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

ettore
Copy link

@ettore ettore commented Mar 20, 2024

Since this fork is not guaranteed to be compatible with the upstream repo, we need to ensure there are no symbols name collisions in case a host app needs to integrate our fork together with the original GCDWebServer as a dependency.

This is part 1 to solving issue readium/swift-toolkit#402

Notes:

  • File names remained the same.
  • Build product name also remained the same. On the client side we'll continue to use import GCDWebServer. This is unfortunate because it seems like SPM uses the Package.name only for display purposes and instead uses the repo name (or rather the last path component of the package URL) as identifier for the module. This is discussed in a few places online (link 1 | link 2). Modifying Package.swift to have ReadiumGCDWebServer as package and library name will produce package resolution errors when this is used as a dependency (e.g. in swift-toolkit) unless we also modify the repo name (I verified this would works).

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

Together these solve issue readium#402
Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for tackling the name changes Ettore!

Build product name also remained the same. On the client side we'll continue to use import GCDWebServer. This is unfortunate because it seems like SPM uses the Package.name only for display purposes and instead uses the repo name (or rather the last path component of the package URL) as identifier for the module. This is discussed in a few places online (link 1 | Iterable/iterable-swift-sdk#537 (comment)). Modifying Package.swift to have ReadiumGCDWebServer as package and library name will produce package resolution errors when this is used as a dependency (e.g. in swift-toolkit) unless we also modify the repo name (I verified this would works).

What about changing only the library and target names? This should allow us to use import ReadiumGCDWebServer even though in the Package Dependencies it will still appear as GCDWebServer.

AFAIK in a Package.swift, we never directly reference a package name. We use the full URL to a third-party package, then reference the names of its products.

I also merged your other PR on this repo, would you mind tacking a look at the conflicts? Thanks.

Once this PR is merged, I'll publish a 4.0.0 version of ReadiumGCDWebServer.

Since this fork is not guaranteed to be compatible with the upstream repo, we need to ensure there are no symbols name collisions in case a host app needs to integrate our fork together with the original GCDWebServer as a dependency.
@ettore ettore force-pushed the fix/symbols-name-collision branch from c53c25f to 8c555d6 Compare March 23, 2024 17:48
@ettore
Copy link
Author

ettore commented Mar 23, 2024

What about changing only the library and target names? This should allow us to use import ReadiumGCDWebServer even though in the Package Dependencies it will still appear as GCDWebServer.

AFAIK in a Package.swift, we never directly reference a package name. We use the full URL to a third-party package, then reference the names of its products.

yep, I tried that too but this is what happens. If I understand well I think you meant this:

let package = Package(
    name: "GCDWebServer",
    platforms: [.iOS(.v11)],
    products: [
        .library(name: "ReadiumGCDWebServer", targets: ["ReadiumGCDWebServer"]),
    ],
    targets: [
        .target(
            name: "ReadiumGCDWebServer",
            path: "GCDWebServer",
            cSettings: [
                .define("SWIFT_PACKAGE")
            ]
        )
    ]
)

Then, if I leave swift-toolkit's Package.swift unaltered of course I get the following pkg resolution error:

product 'GCDWebServer' required by package 'swift-toolkit' target 'ReadiumAdapterGCDWebServer' not found.

If I then modify swift-toolkit's Package.swift to use ReadiumGCDWebServer like so:

        .target(
            name: "R2Streamer",
            dependencies: [
                "CryptoSwift",
                "Fuzi",
                "ReadiumGCDWebServer",
                "Zip",
              .......
        )
...
        .target(
            name: "ReadiumAdapterGCDWebServer",
            dependencies: [
                "ReadiumGCDWebServer",
                "R2Shared",
            ],
            path: "Sources/Adapters/GCDWebServer"
        ),

I misteriouly get these errors:

unknown dependency 'ReadiumGCDWebServer' in target 'R2Streamer'; valid dependencies are: 'Fuzi', 'CryptoSwift', 'Zip', 'DifferenceKit', 'GCDWebServer', 'SwiftSoup', 'SQLite.swift', 'ZIPFoundation'

unknown dependency 'ReadiumGCDWebServer' in target 'ReadiumAdapterGCDWebServer'; valid dependencies are: 'Fuzi', 'CryptoSwift', 'Zip', 'DifferenceKit', 'GCDWebServer', 'SwiftSoup', 'SQLite.swift', 'ZIPFoundation'

As you can see these last 2 errors are "incompatible" with the previous one. The only way that I have found that solves them and allow us to use ReadiumGCDWebServer (which IMO would be preferable and clearer for clients) is to change the repo name to ReadiumGCDWebServer. I agree with the people commenting on the links I provided in the description: this looks like a bug in Xcode (or maybe SPM).

@ettore
Copy link
Author

ettore commented Mar 23, 2024

I also merged your other PR on this repo, would you mind tacking a look at the conflicts? Thanks.

Done!

Once this PR is merged, I'll publish a 4.0.0 version of ReadiumGCDWebServer.

👍

@mickael-menu
Copy link
Member

Maybe you're having some cache issues? I tried forking the project, changed the Package.swift and it worked fine on a test project.

https://github.com/mickael-menu/GCDWebServer

Test.zip

@ettore
Copy link
Author

ettore commented Mar 24, 2024

No, I have cleared derived data every time I attempted something new SPM related. But maybe it's because I'm using Xcode 15.0.1 (15A507) with macOS 13.5 (M1), so I'm not up to the latest. I wonder if this was a bug on my Xcode version which got fixed recently. Supporting this hunch I think it's also the fact that your Test project doesn't open for me. I get the following error when I try to resolve packages:

Package.resolved file is corrupted or malformed; fix or delete the file to continue: unknown 'PinsStorage' version '3' at '/Users/ettorepasquini/Downloads/Test/Test.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved'.

Removing and re-adding the package from your fork doesn't work either.

Which version of Xcode are you using? I'll try using the latest.

@mickael-menu
Copy link
Member

I'm using Xcode 15.3, but I think you might need to remove ~/.swiftpm as well as the derived data.

@ettore
Copy link
Author

ettore commented Mar 24, 2024

yep, I cleared ~/.swiftpm too... I just re-did all the clearing one more time on both my project and yours and I always get the same error.

However, I did miss something. I was still referencing v3.7.5 in my package definition which maybe (I thought) was confusing Xcode. So I pushed the package/library rename to a new branch on my fork and also created a new 4.0.0 version: https://github.com/ettore/GCDWebServer/releases/tag/4.0.0
Then changed swift-toolkit to use

dependencies: [
    ...
    .package(url: "https://github.com/ettore/GCDWebServer.git", from: "4.0.0"),
   ...

I quit Xcode, re-cleared derived data and ~/.swiftpm, but still, Xcode complains with the "unknown dependency 'ReadiumGCDWebServer' in target 'R2Streamer'; valid dependencies are: ... 'GCDWebServer',..."

I DID find a way to make it work by using a different package dependency method, e.g.:

        .package(name: "ReadiumGCDWebServer",
                 url: "https://github.com/ettore/GCDWebServer.git",
                 from: "4.0.0"),

however this requires // swift-tools-version:5.5 and most importantly it is deprecated, so it's a non-starter.

I'm still using Xcode 15.0.1 and I know I need to upgrade.... I hope to be able to do that soon.

@ettore
Copy link
Author

ettore commented Mar 25, 2024

I upgraded to Sonoma 14.4 and Xcode 15.3. I am now able to open your Test project and that works fine.

But that project specifies the GCDWebServer dependency only from the project configuration, not from a swift package. If I also add swift-toolkit as a SPM dependency, swift-toolkit will get the same package resolution errors I described earlier for Xcode 15.0.1: neither ReadiumGCDWebServer nor GCDWebServer can be referenced from swift-toolkit if GCDWebServer.git uses a Package manifest like the one we'd like to use.

@mickael-menu
Copy link
Member

I did more tests and found a solution that seems to work at least on Xcode 15.3.

In GCDWebServer/Package.swift, keep the package name as GCDWebServer, but prefix the product and target with Readium

let package = Package(
    name: "GCDWebServer",
    platforms: [.iOS(.v11)],
    products: [
        .library(name: "ReadiumGCDWebServer", targets: ["ReadiumGCDWebServer"]),
    ],
    targets: [
        .target(
            name: "ReadiumGCDWebServer",
            path: "GCDWebServer",
            cSettings: [
                .define("SWIFT_PACKAGE")
            ]
        )
    ]
)

Then in swift-toolkit/Package.swift, fully expand the ReadiumGCDWebServer dependency with .product(name: "ReadiumGCDWebServer", package: "GCDWebServer"). For example:

.target(
    name: "ReadiumAdapterGCDWebServer",
    dependencies: [
        .product(name: "ReadiumGCDWebServer", package: "GCDWebServer"),
        "R2Shared",
    ],
    path: "Sources/Adapters/GCDWebServer"
),

Not sure why "ReadiumGCDWebServer" is not enough for Xcode, probably a bug since it's unrelated to the name of the package or repo.

You can try it out in my fork with the test-spm branch: https://github.com/mickael-menu/swift-toolkit/blob/test-spm/Package.swift

We could of course rename the repo, but then it would break the older versions of the toolkit for the same reason. I prefer not to maintain two separate GCDWebServer repositories in the Readium organization. :/

@ettore
Copy link
Author

ettore commented Mar 25, 2024

that worked! 🎉 🎉 I just pushed my changes to this branch and to the other PR on swift-toolkit.

Thank you for working with me on this, I was not familiar with that way of specifying product dependencies in targets!

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

Together these solve issue readium#402
Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ettore!

@mickael-menu mickael-menu merged commit 8dcab90 into readium:master Mar 25, 2024
@ettore ettore deleted the fix/symbols-name-collision branch March 25, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants