GNOME Bugzilla – Bug 732920
Support building against external synctex library
Last modified: 2018-05-22 15:41:18 UTC
Created attachment 280181 [details] [review] patch against evince 3.12.1 Hello! Someone in Debian made a synctex library package and filed: http://bugs.debian.org/754136 I'm hoping that maybe you could consider supporting this option in the build system of evince. (I'm attaching a patch I created against 3.12.1...)
Created attachment 280182 [details] [review] patch against master (cc55d7383cf)
Review of attachment 280182 [details] [review]: Thanks for the patch, it makes sense to me. I've a few comments/questions, though. ::: configure.ac @@ +467,3 @@ +if test "x$has_synctex" = "xno"; then + SYNCTEX_LIBS="\$(top_builddir)/cut-n-paste/synctex/libsynctex.la" + AC_SUBST(SYNCTEX_LIBS) Don't we need to do this out of the if also for the case where this has been filled by PKG_CHECK_MODULES? @@ +470,3 @@ + + SYNCTEX_CFLAGS="-I\$(top_srcdir)/cut-n-paste/synctex" + AC_SUBST(SYNCTEX_CFLAGS) Ditto @@ +472,3 @@ + AC_SUBST(SYNCTEX_CFLAGS) + + SYNCTEX_CONFIG_FILES=cut-n-paste/synctex/Makefile Do we really need this? I guess it doesn't matter if the Makefile is created as long as the syntex dir is not added to the SUBDIRS of the parent Makefile. @@ +476,3 @@ + SYNCTEX_CONFIG_FILES="" +fi +AM_CONDITIONAL(ENABLE_BUNDLED_SYNCTEX, test x$has_synctex = xno) I prefer something like USE_INTERNAL_SYNTEX or USE_SYSTEM_SYNTEX @@ +947,3 @@ GTK+ Unix Print ..........: $with_gtk_unix_print Thumbnail cache ..........: $enable_gnome_desktop +Use external SyncTeX .....: $has_synctex What about something like: SyncTex ......: system/internal
Hello Carlos! Thanks for your review. I have to ask some questions back to you, probably because I just don't understand what you are saying. Sorry for that. (In reply to comment #2) > Review of attachment 280182 [details] [review]: > > Thanks for the patch, it makes sense to me. I've a few comments/questions, > though. > > ::: configure.ac > @@ +467,3 @@ > +if test "x$has_synctex" = "xno"; then > + SYNCTEX_LIBS="\$(top_builddir)/cut-n-paste/synctex/libsynctex.la" > + AC_SUBST(SYNCTEX_LIBS) > > Don't we need to do this out of the if also for the case where this has been > filled by PKG_CHECK_MODULES? No? Could you clarify what you mean by "this"? The cut-n-paste part should definitely only be set here, but maybe you mean only the AC_SUBST? The AC_SUBST is still not needed elsewhere. I have tested building this patch (for 3.12.1) both with and without external libsynctex. I'm definitely no autotools guru, but I can see during the bulid that the CFLAGS / LIBS are properly set up by PKG_CHECK_MODULES when using external libsynctex. > > @@ +470,3 @@ > + > + SYNCTEX_CFLAGS="-I\$(top_srcdir)/cut-n-paste/synctex" > + AC_SUBST(SYNCTEX_CFLAGS) > > Ditto > > @@ +472,3 @@ > + AC_SUBST(SYNCTEX_CFLAGS) > + > + SYNCTEX_CONFIG_FILES=cut-n-paste/synctex/Makefile > > Do we really need this? I guess it doesn't matter if the Makefile is created as > long as the syntex dir is not added to the SUBDIRS of the parent Makefile. I've tested dropping the SYNCTEX_CONFIG_FILES part and it still builds fine, so if you prefer always generating this file even when unused we could do so. > > @@ +476,3 @@ > + SYNCTEX_CONFIG_FILES="" > +fi > +AM_CONDITIONAL(ENABLE_BUNDLED_SYNCTEX, test x$has_synctex = xno) > > I prefer something like USE_INTERNAL_SYNTEX or USE_SYSTEM_SYNTEX > > @@ +947,3 @@ > GTK+ Unix Print ..........: $with_gtk_unix_print > Thumbnail cache ..........: $enable_gnome_desktop > +Use external SyncTeX .....: $has_synctex > > What about something like: > > SyncTex ......: system/internal You mean like replacing: PKG_CHECK_MODULES(SYNCTEX, [synctex], has_synctex=yes, has_synctex=no) with: PKG_CHECK_MODULES(SYNCTEX, [synctex], has_synctex=system, has_synctex=internal) (updating the if check) and using: SyncTex ......: $has_synctex or do you have some nice way to suggest for accomplishing the pretty output?
Created attachment 372206 [details] [review] Updated patch addressing Carlos' comments This patch addresses Carlos' comments. Even though Andreas is correct about PKG_CONFIG exporting CFLAGS and LIBS (using AC_SUBST), having them out of the condition is harmless, and easier to read.
Created attachment 372207 [details] [review] Update patch addressing Carlos' comments Oops. I attached the wrong one. Comments stay the same.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/evince/issues/479.