AppImageCommunity / libappimage

Implements functionality for dealing with AppImage files
https://appimage.org
Other
46 stars 29 forks source link

Catch dlopen errors on libcairo and librsvg and trait them as warnigs #117

Closed azubieta closed 5 years ago

azubieta commented 5 years ago

This will avoid the whole integration process being stopped if libcairo, librsvg or libgobject are not available.

Contributes to #104

TheAssassin commented 5 years ago

Shouldn't there be tests that trigger the old wrong behavior that then get fixed by this new code?

azubieta commented 5 years ago

To properly implement this and many of the other tests you suggest it's required to perform a major architectural change on the code: apply the dependency inversion principle.

This patter requires to create an abstract class for every joint point in the project so everything will be duplicated. There will be an minor performance impact and a lot of more code. The gain is a more decoupled and testable project.

If we are going to dedicate an sprint to improve tests on libappimage would be nice to do this on it.

There is another option of faking an scenario where libgobject is not reachable but that wouldn't be too elegant.

TheAssassin commented 5 years ago

@probonopd are you fine with this workaround?

probonopd commented 5 years ago

If I understand the proposal right, then this will allow libappimage and tools that use it (e.g., appimaged and AppImageLauncher) to still run even if the dlopen'ed dependencies are not working.

This means that the result is broken, because the icon thumbnails will not be working. Correct? That is not a full solution. We need a solution that makes things work, not just "fail better".

Whatever dependencies we need in order for things to work correctly must be hard dependencies, otherwise we will have half-broken stuff on users' system with icons not working properly. Again, in order to create an experience that "just works" for everyone, we must not allow for too many options (=some people have the dependencies, some don't).

azubieta commented 5 years ago

@probonopd just to clarify, This PR allows to libappimage in defect of librsvg, libcairo or libgobject to behave the same way as in the previous versions (just copy the .DirIcon into thumbnails and share/icons)

probonopd commented 5 years ago

Yes I understand, this means that it will be "less broken" and it will make debugging harder since there are then two execution paths - one when librsvg, libcairo and libgobject are present and one when they are not. This adds complexity and makes support harder, also it makes the behavior inconsistent and some users will get a degraded experience. We should make it work the same in all cases by enforcing the dependencies to be there.

TheAssassin commented 5 years ago

Before, you didn't seem to notice the broken behavior, now there's a better implementation but shipping is problematic, so why not implement a fallback for now and try to come up with a better solution later on?

probonopd commented 5 years ago

As you say, now we have something that works better but we must ensure that people use it. Let's not degrade experience for anyone. At least, if dlopen() fails, print a clear error message and exit. Better "hard depend" on it. I don't want people with half-working systems.

TheAssassin commented 5 years ago

"Degrade" is the wrong term. It's basically "can not improve everywhere". Degrading means things would become worse compared to the current/previous situation, which is not the case.

Let's see if #118 works as intended.

probonopd commented 5 years ago

"Degrade" as in: lesser experience compared to be best we can do.

Let's see if #118 works as intended.

Yes!

azubieta commented 5 years ago

dl_open behavior was removed from the main workflow therefore this change is not required. Closing it.