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

Depend on and import XCTestDynamicOverlay explicitly #65

Merged
merged 1 commit into from May 13, 2023

Conversation

myihsan
Copy link
Contributor

@myihsan myihsan commented May 9, 2023

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

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 myihsan changed the title Depend on and import XCTestDynamicOverlay specifically Depend on and import XCTestDynamicOverlay explicitly May 13, 2023
@myihsan
Copy link
Contributor Author

myihsan commented May 13, 2023

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

This is not required, but it can be a workaround for a build error when dynamic linking XCTestDynamicOverlay is required.
@tgrapperon
Copy link
Owner

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

@tgrapperon tgrapperon merged commit 9b483d4 into tgrapperon:main May 13, 2023
8 checks passed
@tgrapperon
Copy link
Owner

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

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.

None yet

2 participants