Skip to content

Android: adding swift suffix to icu generated libs to resolve conflict #11546

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

Merged
merged 1 commit into from
Aug 22, 2017

Conversation

amraboelela
Copy link
Contributor

Android: adding swift suffix to icu generated libs to resolve conflit with Android's internal icu libs

https://bugs.swift.org/browse/SR-5672

Resolves SR-5672.

@milseman
Copy link
Member

milseman commented Aug 22, 2017

I missed the original reasoning here. Why not use Android's ICU libraries? Are they not accessible, or not guaranteed to be available? Also, is this shipping ICU with or without symbol renaming? Either way, there will likely be symbol clashes when both are loaded into memory.

edit: I may just be misunderstanding U_HAVE_LIB_SUFFIX. Does that also add the suffix for all symbols too?
edit: For posterity, yes, U_HAVE_LIB_SUFFIX seems to trigger a symbol renaming too.

@amraboelela
Copy link
Contributor Author

LibswiftCore uses symboles which doesn't exist in Android icu libs, but they exist in our generated icu libs. My PR makes sure that we add suffix to libs name only and not to internal symbols

@milseman
Copy link
Member

Was that due to a mismatch in symbol renaming configuration, or does Android suppress ICU symbols? I'm asking because Linux may want to make a similar choice of trade offs.

Either way, this PR LGTM.

@milseman
Copy link
Member

@swift-ci please smoke test

@amraboelela
Copy link
Contributor Author

If I rename the ICU symbols, i'll get linking error, as libswiftCore call the original symbol names without my suffix. Android doesn't suppress ICU symbols.

@milseman
Copy link
Member

Then why not link against Android's?

@milseman
Copy link
Member

To be clear, shipping your own ICU is a totally valid approach and probably the right one for Android (and maybe Linux too). I'm just wondering if this is an intentional decision or an accidental result of misunderstanding ICU version pinning through symbol renaming. Either way, out of scope for this PR.

@milseman milseman merged commit 85c2a88 into swiftlang:master Aug 22, 2017
@amraboelela
Copy link
Contributor Author

Android uses a different version ICU libs which doesn't have all the symbols that libswiftCore uses.

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