| Summary: | media-sound/denemo-2.5.0 remove hard dependency to app-text/evince | ||
|---|---|---|---|
| Product: | Gentoo Linux | Reporter: | marius.spix |
| Component: | Current packages | Assignee: | Bernd <waebbl-gentoo> |
| Status: | CONFIRMED --- | ||
| Severity: | enhancement | CC: | gnome, proxy-maint, sam, waebbl-gentoo |
| Priority: | Normal | Keywords: | EBUILD, PATCH |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
| Attachments: |
media-sound/denemo-2.5.0-r1 ebuild
Patch, which adds Atril support |
||
Created attachment 720396 [details, diff]
Patch, which adds Atril support
Thanks for your request. If it hasn't changed, the dependency on evince is a hard one, in the sense that it needs to have a previewer installed. On my tests some time ago, the package won't configure, if evince isn't installed. If there's now an alternative available, the ebuild could be improved by using it alternatively. I'm not a mate user and wasn't aware of the atril package. Have you checked upstream, whether there's a topic already open for this issue or if upstream devs want to include the support for it in general? Upstream bug is http://savannah.gnu.org/bugs/index.php?60827 Thank you! I searched github for the removal of the evince USE flag. It is tied to the removal of gtk2 support for denemo (see https://github.com/gentoo/gentoo/pull/15029#issuecomment-602088170). Denemo supports two graphical interfaces: gtk-2 and gtk-3. In the meantime, gtk-2 has been deprecated in Gentoo, so we need to stick to gtk-3. I don't believe we want to ship with a denemo without a user-interface, if it's even possible to build it. Your patch is a good starting point, but currently looks incomplete to me at a first glance. You don't define an option for the have_atril you are using (no AC_ARG_ENABLE). Also the logic seems to somewhat inconsistent. From my understanding the logic should be we use either evince>=3 or atril>=1.24, each of which can be enabled independently. We also need tests to ensure the configuration works correct if none or both of them are enabled. For the latter case, is there a preference for which one has priority? From your patches of the source code files, I deduct, the latter is not really an option. There are either evince header files or atril header files included, which means we need either of them. But you prefer atril, which is inconvenient from my POV. I believe, there are more users which use gnome, than user that are using mate, so the test for evince should come first. Your patch currently still needs evince installed (ignoring the evince-2/gtk-2 case) if I get this right: if test "x$useevince" = "xyes"; then CFLAGS="$CFLAGS -DUSE_EVINCE" LIBS="$LIBS -DUSE_EVINCE" + if test "x$have_atril" = "xyes"; then + CFLAGS="$CFLAGS -DUSE_ATRIL" + LIBS="$LIBS $ATRIL_LIBS" + fi fi Shouldn't the condition for have_atril be in the else-fork of the useevince condition? The same is true with the ebuild. When the evince USE flag is given, it pulls in both, evince *and* atril. Do I get you right, you want to remove the burden of installing lot's of gnome dependencies for mate users by building with atril? If the package still depends on evince we haven't won anything, but instead need to depend on another package, which normally many users wont install. @Bernd Thank you for pointing out. I also added conditional code which switches the included header file. So if USE_EVINCE is set, it is also checked whether USE_ATRIL is set and the corresponding header is included. + if test "x$have_atril" = "xyes"; then + CFLAGS="$CFLAGS -DUSE_ATRIL" + LIBS="$LIBS $ATRIL_LIBS" + fi That must be a mistake. I mean + if test "x$have_atril" = "xyes"; then + CFLAGS="$CFLAGS -DUSE_ATRIL" + LIBS="$LIBS -DUSE_ATRIL" + fi Because Atril is ABI-compatible Evince, I consider it as a special case of Evince. If neither Evince nor Atril exist, a message is printed that indicates that you need Evince. There are multiple cases in the code where USE_EVINCE is checked. (This could be refactored to a macro to make it easier to translate, but I did not want to change so much code.) But I agree in that case that actually makes no sense to use Denemo without any PDF viewer. Frescobaldi uses dev-python/PyQt5[printsupport,svg] and should be a reasonable alternative on non-GNOME environments. The preference priority is Evince > Atril, because Evince is the more heavy application. If both exist on a system, it makes no sense to use Atril. A possibility to activate both independent would require much more change in the code without having much benefit, as both PDF viewers are too similar. The evince useflag does not fetch both, but only one package, whereas Evince is preferred. Evince has a lot of GNOME specific dependencies, which are not needed in MATE. Having both Evince and Atril installed has no added value. (In reply to marius.spix from comment #5) > That must be a mistake. I mean > > + if test "x$have_atril" = "xyes"; then > + CFLAGS="$CFLAGS -DUSE_ATRIL" > + LIBS="$LIBS -DUSE_ATRIL" > + fi > I think your initial code was correct with respect to the flags used, and the upstream code for useevince is wrong. I think, there's no need to have -DUSE_ATRIL both in CFLAGS and LIBS, this looks redundant to me. What I actually wanted to say is the logic of this snippet. If evince is enabled you check whether atril is also enabled. Shouldn't this be like if evince = "yes"; then ... elif atril = "yes"; then ... else ... fi? So far I only looked at the code. Need to test it before I can say more. > The evince useflag does not fetch both, but only one package, whereas Evince > is preferred. > > Evince has a lot of GNOME specific dependencies, which are not needed in > MATE. Having both Evince and Atril installed has no added value. evince? ( >=app-text/evince-3.22.1-r1:= >=app-text/atril-1.5:= ) This code means, if evince USE flag is set, pull in both evince *and* atril. If you want to include only one, depending on whether the USE flag is set or not you would use something like evince? ( app-text/evince:= ) !evince? ( app-text/atril:= ) (In reply to Bernd from comment #6) > (In reply to marius.spix from comment #5) > > That must be a mistake. I mean > > > > + if test "x$have_atril" = "xyes"; then > > + CFLAGS="$CFLAGS -DUSE_ATRIL" > > + LIBS="$LIBS -DUSE_ATRIL" > > + fi > > > I think your initial code was correct with respect to the flags used, and > the upstream code for useevince is wrong. I think, there's no need to have > -DUSE_ATRIL both in CFLAGS and LIBS, this looks redundant to me. > What I actually wanted to say is the logic of this snippet. If evince is > enabled you check whether atril is also enabled. Shouldn't this be like > if evince = "yes"; then > ... > elif atril = "yes"; then > ... > else > ... > fi? > > So far I only looked at the code. Need to test it before I can say more. > > > > The evince useflag does not fetch both, but only one package, whereas Evince > > is preferred. > > > > Evince has a lot of GNOME specific dependencies, which are not needed in > > MATE. Having both Evince and Atril installed has no added value. > > evince? ( >=app-text/evince-3.22.1-r1:= >=app-text/atril-1.5:= ) > > This code means, if evince USE flag is set, pull in both evince *and* atril. > If you want to include only one, depending on whether the USE flag is set or > not you would use something like > > evince? ( app-text/evince:= ) > !evince? ( app-text/atril:= ) That is not what I meant. The whole conditional code in denemo depends on the evince use flag. The only difference is the name of the header file. So Denemo is using "evince", even if it is actually atril. (This is similar to the integration of MariaDB into applications build for use with MySQL.) > !evince? ( app-text/atril:= ) That is not what I meant, because it does not allow you to build without evince and atril. Of course, it should say: evince ( || ( app-text/evince:= app-text/atril:= ) ) (In reply to marius.spix from comment #7) > (In reply to Bernd from comment #6) > > (In reply to marius.spix from comment #5) > > evince? ( app-text/evince:= ) > > !evince? ( app-text/atril:= ) > > That is not what I meant. The whole conditional code in denemo depends on > the evince use flag. The only difference is the name of the header file. So > Denemo is using "evince", even if it is actually atril. (This is similar to > the integration of MariaDB into applications build for use with MySQL.) > This doesn't sound like a clean implementation. IMO, if we add support for an alternative viewer and upstream decides to add this, it should be a clean and solid implementation, which doesn't have a chance of breaking at one of the next updates. We wouldn't do Gentoo a favor then. From my understanding of autotools, there should be an option to select either evince or atril and not have both depend on the same option selected. So we would need either an option for atril, or we choose to select atril, if evince option is disabled. > > !evince? ( app-text/atril:= ) > That is not what I meant, because it does not allow you to build without > evince and atril. Of course, it should say: > > evince ( || ( app-text/evince:= app-text/atril:= ) ) I see. This code however wouldn't work. What happens, if USE=-enable is set? Then neither evince, nor atril is available and ./configure will fail, because gtk3 support in denenmo unconditionally needs evince (or atril FWIW). Would you enjoy in preparing a PR for this patch? I think it would be easier if we can work with the sources directly and improve on the patch as we go. You will, of course, get support if you decide to do so. You don't need to, however. It's ok, if you don't want to take this step. The support for atril is in upstream since release 2.5.5 http://savannah.gnu.org/bugs/?func=detailitem&item_id=60827 https://github.com/denemo/denemo/commit/7266d56ceb4c69a75934a2b8c575b59eb135e6a6 However, the ebuild media-sound/denemo-2.6.0 still depends on app-text/evince, even when app-text/atril is installed. Is here someone who could help to fix the ebuild? Maybe a virtual/evince would be helpful? Thank you a lot! |
Created attachment 720387 [details] media-sound/denemo-2.5.0-r1 ebuild media-sound/denemo should not depend on app-text/evince app-text/evince is only an optional dependency for media-sound/denemo if a print preview is requested. app-text/atril is an ABI-compatible fork of app-text/evince and the default PDF viewer in MATE Desktop. The attached ebuild and patch remove the hard dependency to Evince and add a check for Atril. When Evince is not found, Atril is used.