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

Fix build with BUILD_LIBRARY_FOR_DISTRIBUTION #82

Merged
merged 1 commit into from
Feb 4, 2021
Merged

Fix build with BUILD_LIBRARY_FOR_DISTRIBUTION #82

merged 1 commit into from
Feb 4, 2021

Conversation

NikolayJuly
Copy link
Contributor

If open pods project and set BUILD_LIBRARY_FOR_DISTRIBUTION = YES, this framework not gonna compile. error will say

initializer for class 'CGAngle' is '@inlinable' and must delegate to another initializer

I was also surprised, but best guess is need to create swiftinterface in this mode, which has strict interface.

If open pods project and set `BUILD_LIBRARY_FOR_DISTRIBUTION = YES`, this framework not gonna compile. error will say 
```
initializer for class 'CGAngle' is '@inlinable' and must delegate to another initializer
```

I was also surprised, but best guess is need to create `swiftinterface`, which has strict interface.
@guoyingtao
Copy link
Owner

guoyingtao commented Feb 4, 2021

@NikolayJuly Thanks for the PR!

I am using @inlinable for the performance reason.

I think ABI / Module stability is not essential here because we can always rebuild it since we have all source code.
So I tend not to modify this part.

If you have any specific reason to set BUILD_LIBRARY_FOR_DISTRIBUTION to yes, just let me know.

@NikolayJuly
Copy link
Contributor Author

NikolayJuly commented Feb 4, 2021

@guoyingtao I understand why people use @inlinable . But here is a few points, why you should delete it.

  1. @inlinable do nto guarantee inline in build time, we simply don't know it. This error from building for distribution, actually saying that external usage of this method won't be inlined at all. So, are you sure it is at least inlined internally? Did you make any test. Might appear you wanna have improvements, but compiler not gonna do inlines for init, which kinda have sense... init in swift also do alloc with memory layout, and apple might not allow inline it, because different versions of compiler.
  2. If your project has 100 pods as dependency and tons of internal frameworks, at some point you will try to minize clean build time. One of the way, precompile all what you can. This is how I figured this out. And options are: drop and replace pod or fork and forget about any update, supporting pod on our own.
  3. There are other dependency managers, some of them uses binary distribution, and they likely might start using xcframeworks soon. Won't it be cool to support all of them?

Taking into account all of this, please remove inline or provide numerical proof, that at least internal usage of this inits with @inlinable actually faster. I'm pretty sure, they not inlined at all after seeing this error.

@NikolayJuly
Copy link
Contributor Author

@guoyingtao If there is numeric proof of at least internal speed up, would it make sense to create 2 more inits for internal usage and make them @inlinable? And 2 public not inlined to support xcframeworks?

Copy link
Owner

@guoyingtao guoyingtao left a comment

Choose a reason for hiding this comment

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

@NikolayJuly I didn't make any test for it but I think you are right. Thanks for pointing it out.

@guoyingtao guoyingtao merged commit 28d0ab8 into guoyingtao:master Feb 4, 2021
@NikolayJuly
Copy link
Contributor Author

Thank you 🙇

@guoyingtao
Copy link
Owner

I released Mantis 1.4.9. Can you check if it solves the issue?

@NikolayJuly
Copy link
Contributor Author

@guoyingtao I checked and it works for me now. Thank you

@NikolayJuly NikolayJuly deleted the patch-1 branch February 6, 2021 18:41
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