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

Use cmake to require ICU version >= 56. #347

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

themobiusproject
Copy link
Contributor

Currently the cmake build is specifically requiring icu version 56. This will allow versions greater than 56 (I am using 72).

@jameshoulahan
Copy link
Contributor

Hi, thanks for the second contribution :)

Similar story -- we've opened an internal ticket (GODT-2410) to check and hopefully merge this. Will keep you posted.

@rlejeune74
Copy link
Contributor

Hello @themobiusproject .

Your pull request has been cherry picked and merged into our internal repository. It will be part of an upcoming release.
Thank you for your contribution!

@rlejeune74 rlejeune74 closed this Apr 26, 2023
@rlejeune74
Copy link
Contributor

Actually we need more work on that side since it appears that even if your PR builds perfectly.
At runtime it ends up with link issue since QtCore is trying to load exactly the one that it has been bundled with (eg: libicuxxx.so.56).

@rlejeune74 rlejeune74 reopened this Apr 27, 2023
@themobiusproject
Copy link
Contributor Author

themobiusproject commented Apr 27, 2023

@rlejeune74 I am trying to figure out the use case where this would be an issue. If the binary is prebuilt and distributed with libicuxxx.so.56 as it has been, then it should be able to continue running with .so.56. If, such as on my machine (Gentoo), I am building with libicuxxx.so.72.1, I already have it on my machine. If I upgrade icu, I am already going to have to rebuild a fair portion of my system as it is. Even though I am manually compiling proton-bridge (not using portage), when I started it up after an icu upgrade it would yell at me that it couldn't load the library and I would rebuild it with the current version of icu, which also wouldn't be a surprise since I have already recompiled many other packages due to this dependency.

I recognize that I am theorizing here and could very well be wrong, but the two main scenarios that I see are the binary being prebuilt and distributed with the correct version of icu or the end user (me) building it and dealing with it myself.

@rlejeune74
Copy link
Contributor

rlejeune74 commented Apr 27, 2023

@themobiusproject
The issue we have is that Qt6.3.2 installed from official Qt installer (the one we are using) wants to link with the exact ICU version it was bundled with (here 56).

$ ldd libQt6Core.so.6.3.2 | grep icu                                                                                                                                                                                                                                                                                                                
	libicui18n.so.56 => not found
	libicuuc.so.56 => not found
	libicudata.so.56 => not found

With your PR, and the current way we are bundling deps, we end up with libicudata.so and libicudata.so.56.1 bundled where QtCore is looking for libicudata.so.56.

@themobiusproject
Copy link
Contributor Author

themobiusproject commented Apr 27, 2023

@rlejeune74
Thank you for the explanation.

I am assuming something like

$ ls -l packaged/libs/libicudata.so*
lrwxrwxrwx 1 root root       18 Jan 26 15:33 packaged/libs/libicudata.so -> libicudata.so.56.1
-rwxr-xr-x 1 root root 31262192 Jan 26 15:33 packaged/libs/libicudata.so.56.1

Would it be sufficient to also have the a link for libicudata.so.56 -> libicudata.so.56.1? I believe the compilation process of proton-bridge would complain if you are trying to link qt6 compiled with a different major versions of icu, so it would prevent compiling proton-bridge with icu 72 using the qt6 libraries from the official installer with icu 56. Another option would be to query libQtCore to see which version of icu is was compiled with and then require that specific version in the cmake. I would be happy to explore either of those options unless you have an even better one.

@mrusme
Copy link

mrusme commented Apr 13, 2024

Any update on this?

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

4 participants