ProtonMail / proton-bridge

Proton Mail Bridge application
GNU General Public License v3.0
1.14k stars 155 forks source link

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

Open themobiusproject opened 1 year ago

themobiusproject commented 1 year ago

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

jameshoulahan commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

themobiusproject commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 5 months ago

Any update on this?