tgrapperon / swift-dependencies-additions

More dependencies for `swift-dependencies`
MIT License
298 stars 39 forks source link

Depend on and import XCTestDynamicOverlay explicitly #65

Closed myihsan closed 1 year ago

myihsan commented 1 year ago

This is not required, but it can be a workaround for the build error below during dynamic linking XCTestDynamicOverlay.

We can work around this error by only adding XCTestDynamicOverlay to DependenciesAdditionsBasics, but it will be a workaround that should be deleted when this error is fixed. So I think it may be better to depend on and import XCTestDynamicOverlay explicitly for clarity, and incidentally work around the error. What do you think?

Steps for reproducing the error:

  1. Create an application project
  2. Add a (dynamic) framework target and embed it in the application target
  3. Add swift-dependencies and link it to the framework target (this step will cause the linking to be dynamic)
  4. Add swift-dependencies-additions and link it to the application target
  5. Build the application target
image
tgrapperon commented 1 year ago

Hey @myihsan Thanks for the PR and sorry for the late reply! I wasn't aware of the issue, but even if it wouldn't exist, I think that this change is positive. I'm however a little surprised that a few modules are not requiring this explicit import. For example, PathDependency or UserNotificationsDependency. The later can maybe inherit it from DependenciesAdditionsBasics, but the former doesn't depend on anything special (beside Dependencies) and yet use XCTDynamicOverlay. Do you know why?

myihsan commented 1 year ago

@tgrapperon Thanks for the reply!

I didn't notice that, but they should also require this explicit import. Let me fix it with a force push later.

We can work around this error by only adding XCTestDynamicOverlay to DependenciesAdditionsBasics

The reason is the same as this workaround. When building DependenciesAdditions, if a target depends on XCTDynamicOverlay, and XCTDynamicOverlay is build for dynamic linking, Xcode will dynamic link XCTDynamicOverlay to DependenciesAdditions. This will solve the issue for all the target DependenciesAdditions depends on since they (including PathDependency and UserNotificationsDependency) are static linked to DependenciesAdditions.

You should be able to reproduce the issue with PathDependency by declearing PathDependency as a library alone.

tgrapperon commented 1 year ago

Thanks again for the PR @myihsan! I'll merge once CI passes.

tgrapperon commented 1 year ago

@myihsan I've just cut v0.5.1 with these changes! Thanks!