Bug 216823 - graphics/mupdf: Build shared libraries
Summary: graphics/mupdf: Build shared libraries
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Tijl Coosemans
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-02-05 13:40 UTC by Tobias Kortkamp
Modified: 2017-02-07 13:42 UTC (History)
2 users (show)

See Also:
uzsolt: maintainer-feedback+


Attachments
libmupdf.diff (9.19 KB, patch)
2017-02-05 13:40 UTC, Tobias Kortkamp
no flags Details | Diff
libmupdf.diff (9.07 KB, patch)
2017-02-06 09:32 UTC, Tobias Kortkamp
no flags Details | Diff
libmupdf-unversioned.diff (8.41 KB, patch)
2017-02-06 12:33 UTC, Tobias Kortkamp
no flags Details | Diff
libmupdf.diff (9.77 KB, patch)
2017-02-06 19:25 UTC, Tobias Kortkamp
no flags Details | Diff
libmupdf.diff (9.75 KB, patch)
2017-02-06 19:44 UTC, Tobias Kortkamp
tobik: maintainer-approval?
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Kortkamp freebsd_committer 2017-02-05 13:40:22 UTC
Created attachment 179642 [details]
libmupdf.diff

Hi,

would it be okay if we switched graphics/mupdf to build shared
libraries instead of static ones?

It requires minor changes to the port and the immediate benefits are:

- Drastic installed size reduction:
  llpp-25_1                      34.3 MiB -> 1022 KiB
  mupdf-1.10.a,2                 285 MiB -> 42.8 MiB
  zathura-pdf-mupdf-0.3.1_1      33.4 MiB -> 26.0 KiB
- For security fixes we won't need to bump llpp's and zathura-pdf-mupdf's
  PORTREVISION anymore
- No need to build mupdf twice anymore (once with -fPIC and once without)

Open questions:

The idea comes from OpenBSD's textproc/mupdf port and they manually
increase the lib's major with every port update.  Since mupdf doesn't
guarantee ABI compatibility and breaks it with every release, I think
we'd need to do the same.  My first idea was to set libraries major to
a value based on the port version (for 1.10a the library's soname
would be libmupdf.so.110.0), but I think OpenBSD's way might be better
in the long run.

FreeBSD 11.0/amd64 test logs: https://pkg.tobik.me/logs/libmupdf/
Portlint: ok
Comment 1 Zsolt Udvari 2017-02-05 14:07:58 UTC
I think the mupdf's PORTEPOCH increase a small typo - you want increase PORTREVISION (I think) :)

It was a fix year 2014 about fPIC and the main patch was created by tijl: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=192397

I'm not expert in .so-files but why do you need SO_MAJOR?
Comment 2 Zsolt Udvari 2017-02-05 14:58:05 UTC
It seems the mupdf and zathura-pdf-mupdf builds&works well.
Comment 3 Tobias Kortkamp freebsd_committer 2017-02-06 09:32:25 UTC
Created attachment 179671 [details]
libmupdf.diff

(In reply to Zsolt Udvari from comment #2)
Thanks for testing!

(In reply to Zsolt Udvari from comment #1)
> I think the mupdf's PORTEPOCH increase a small typo - you want increase PORTREVISION (I think) :)

I changed PORTVERSION -> DISTVERSION and 1.10a > 1.10.a according to
`pkg version -t 1.10a 1.10.a`, so I think a PORTEPOCH bump is needed.

But maybe the DISTVERSION change isn't even needed in this case.
I'm mostly going by
https://www.freebsd.org/doc/en/books/porters-handbook/makefile-distfiles.html
and what I had to do with games/iortcw (bug #214959).

Anyway I reverted the change and am bumping PORTREVISION instead :)

> I'm not expert in .so-files but why do you need SO_MAJOR?

I'm not either.  AFAIUI, you need to have some kind of indicator for
the ABI version of the library.  In theory you could have two
incompatible libmupdf's on the system e.g. libmupdf.so.1.0 and
libmupdf.so.2.0 (e.g. from Mupdf 1.9a or 1.10a) with different
binaries linking to either one of them and the dynamic linker needs to
be able to distinguish the two.

Since Mupdf doesn't include a way to build a shared library in its
build system we have to invent your own versioning scheme and set the
libs soname appropriately.

Take a look at
https://www.freebsd.org/doc/en/books/developers-handbook/policies-shlib.html.

According to the article I probably should've started SO_MAJOR at 1,
so I changed that too.
Comment 4 Zsolt Udvari 2017-02-06 11:42:02 UTC
I don't see the benefit of SO_MAJOR. Why isn't enough a simple libmupdf.so?
Comment 5 Tobias Kortkamp freebsd_committer 2017-02-06 12:33:28 UTC
Created attachment 179674 [details]
libmupdf-unversioned.diff

I don't think I can offer a better explanation to why I thought
it was needed than what I tried to give in comment #3.

Anyway there is not much point in arguing about it, so here is a
version of the patch without SO_MAJOR and just libmupdf.so :)
Comment 6 Zsolt Udvari 2017-02-06 12:47:04 UTC
(In reply to Tobias Kortkamp from comment #5)
I want to understand the SO_MAJOR. I think if OpenBSD's developers use this idea it has benefit/meaning.

For example:
$ readelf -a libcurl.so | grep -i soname
 0x000000000000000e (SONAME)             Library soname: [libcurl.so.4]

The "SO_MAJOR" can read from libcurl.so and it matches with the filename.
Can we will read it from libmupdf.so? If the "SONAME" and "SO_MAJOR" doesn't match what will happen?
Comment 7 Tijl Coosemans freebsd_committer 2017-02-06 15:26:14 UTC
You cannot use DISTVERSION here because upstream released 1.10a after 1.10 and 1.10.a comes before 1.10.

There's an extra patch for graphics/llpp.  What is that about?

SO_MAJOR is just an arbitrary name for a variable.  What matters is that the library is now linked with -soname=$@ where $@ means the make target which is $(MUPDF_LIB) or $(THIRD_LIB).

If upstream does not guarantee ABI compatibility between versions (e.g. they may change function prototypes or change the size of types), then you must use a versioned soname.  Programs that use the library will then fail to start with a new version because they are still looking for the old library name.  This is a safer failure mode than the unpredictable failures that may happen when programs blindly use an unversioned and possibly incompatible library.

You can use any string as soname.  I think it would be better to replace ${SO_MAJOR}.0 with $(SOVERSION) in the OpenBSD patch and set SOVERSION=${PORTVERSION} in the port Makefile.
Comment 8 Zsolt Udvari 2017-02-06 16:04:45 UTC
(In reply to Tijl Coosemans from comment #7)
@Tijl Thanks for explanation. Will the changed soname causes re-build on servers?
And: does the linker think that the libmupdf.so.10a is newer than libmupdf.so.10? If yes I think too the "SONAME" can be PORTVERSION.
Comment 9 Tijl Coosemans freebsd_committer 2017-02-06 17:03:16 UTC
(In reply to Zsolt Udvari from comment #8)
Currently the package build servers always rebuild all ports that depend on an updated port, so they will pick up the changed library name, but you should not rely on this and always bump PORTREVISION on these ports.

The linker does not compare versions.  It only looks for exact matches.  All that matters is that incompatible versions of a library have unique names.
Comment 10 Zsolt Udvari 2017-02-06 18:32:22 UTC
(In reply to Tijl Coosemans from comment #9)
"The linker does not compare versions.  It only looks for exact matches."
Oh, indeed, I'm stupid. The 'ldd' always prints the exact filenames.
One more question: is it enough to create links and "USE_LDCONFIG=yes"?

@Tobias: if you use SOVERSION=${PORTVERSION}, I think, it's acceptable (both mupdf and zathura-pdf-mupdf) with the following modification:
"@cd ${STAGEDIR}..." should be "cd ${STAGEDIR}..." (without @, to be verbose)
Comment 11 Zsolt Udvari 2017-02-06 18:33:18 UTC
@Tobias: thanks for your idea and work :)
Comment 12 Tobias Kortkamp freebsd_committer 2017-02-06 19:25:13 UTC
Created attachment 179681 [details]
libmupdf.diff

Here is the patch with the requested changes :)

(In reply to Tijl Coosemans from comment #7)

> There's an extra patch for graphics/llpp.  What is that about?

It's a (very low priority) fix for the brightness decrease key in
llpp, which I haven't submitted yet and forgot to remove from the
patch.

I'm hoping it can go in with this change, but if that's
inappropriate just say the word and I'll remove it.
Comment 13 Zsolt Udvari 2017-02-06 19:36:25 UTC
(In reply to Tobias Kortkamp from comment #12)
Oh, I think I wasn't (enough) clear.

In this case IMHO shouldn't introduce new variable (SOVERSION) in Makefile. Maybe it's enough:
MAKE_ARGS=.... SOVERSION=${PORTVERSION}
PLIST_SUB= SOVERSION=${PORTVERSION}
I think we will not use SOVERSION which differs from PORTVERSION.
Do you agree?
Comment 14 Tobias Kortkamp freebsd_committer 2017-02-06 19:44:02 UTC
Created attachment 179682 [details]
libmupdf.diff
Comment 15 Tijl Coosemans freebsd_committer 2017-02-06 20:10:17 UTC
(In reply to Zsolt Udvari from comment #10)
> is it enough to create links and "USE_LDCONFIG=yes"?
ldconfig won't cache this library I think.  It's an old program that only looks for files named libNAME.so.N where N is a single number.  There are many libraries nowadays that don't match that pattern.  The port should still set USE_LDCONFIG=yes just in case ldconfig is ever fixed.

The unversioned links created in post-install aren't related to ldconfig.  They are used at compile-time when -lfoo becomes libfoo.so.
Comment 16 commit-hook freebsd_committer 2017-02-07 13:41:00 UTC
A commit references this bug:

Author: tijl
Date: Tue Feb  7 13:40:00 UTC 2017
New revision: 433550
URL: https://svnweb.freebsd.org/changeset/ports/433550

Log:
  Patch graphics/mupdf to build shared libraries instead of static ones.
  Because the libraries are faily big this significantly reduces the size of
  programs linking to it.  Use PORTVERSION as the library version because
  upstream does not guarantee compatibility between any two versions.

  Add an upstream patch to graphics/llpp to fix brightness increase key.

  PR:		216823
  Submitted by:	Tobias Kortkamp <t@tobik.me> (maintainer of llpp)
  Approved by:	Zsolt Udvari <udvzsolt@gmail.com> (maintainer)
  Obtained from:	OpenBSD

Changes:
  head/graphics/llpp/Makefile
  head/graphics/llpp/files/patch-build.sh
  head/graphics/llpp/files/patch-main.ml
  head/graphics/mupdf/Makefile
  head/graphics/mupdf/files/patch-Makefile
  head/graphics/mupdf/pkg-plist
  head/graphics/zathura-pdf-mupdf/Makefile