1.23.19: Building against mupdf's C++/Python language bindings is still quite rough around the edges

[dvzrv]

Description of the bug

Hi!

I finally managed to upgrade mupdf to 1.23.9 and pymupdf to 1.23.19. I have tried this for weeks on and off and ran into a lot of issues. When using the usual PYMUPDF_SETUP_IMPLEMENTATIONS=a the tests now all fail because they can't import the fitz module, as it is renamed to old_fitz.

• Building the mupdf C++ and Python language bindings requires at least three separate patches to
  • disable the forced use of venvs which download stuff during build
  • honor LDFLAGS when building C++ bindings so that they have full RELRO
  • fix the install-shared-* targets to not call each other and fail on attempting to reinstalling files multiple times, reduce the install calls, prevent everything being rebuilt before installation and have the libmupdfcpp.so installed properly (with symlink)
• I was unable to have full RELRO for the Python language binding. In case you know how to achieve this with swig, I'd be very interested to hear your thoughts.
• edit: Somewhat unrelated we have just found, that building mupdf with more than one build job is brittle and breaks on some systems due to concurrency issues (linker calls fail). We have now limited the build to one job (-j1) because of this.

You can find what is currently required here: https://gitlab.archlinux.org/archlinux/packaging/packages/mupdf/-/tree/1861ae1e50532cc903f6cab66a3377a2e6204863 I can provide these patches to the mupdf upstream repo if you want.

Building with PYMUPDF_SETUP_IMPLEMENTATIONS=b now works for me and you can find the relevant files here: https://gitlab.archlinux.org/archlinux/packaging/packages/python-pymupdf/-/tree/a645b56162a3a4aaae7c440ad195fffeb602c7db I have disabled a few more tests, as I didn't have the energy to further work on these packages, which have now eaten quite a lot of my free time.

The integration is unfortunately still not where it could be and it would be great if the relevant mupdf make targets and scripts could be fixed.

How to reproduce the bug

This is the build script of mupdf: https://gitlab.archlinux.org/archlinux/packaging/packages/mupdf/-/blob/126844021933d836bb3def58001dbe8d27b790f1/PKGBUILD

This is the build script of pymupdf for 1.23.8 (since then I have tried building nearly all patch level releases without success): https://gitlab.archlinux.org/archlinux/packaging/packages/python-pymupdf/-/blob/e638e70e1ad7729b74ab0823e81a7611d459bf57/PKGBUILD

PyMuPDF version

1.23.19

Operating system

Linux

Python version

3.11

[julian-smith-artifex-com]

I finally managed to upgrade mupdf to 1.23.9 and pymupdf to 1.23.19. I have tried this for weeks on and off and ran into a lot of issues. When using the usual PYMUPDF_SETUP_IMPLEMENTATIONS=a the tests now all fail because they can't import the fitz module, as it is renamed to old_fitz.

I guess we could specially handle PYMUPDF_SETUP_IMPLEMENTATIONS=a to create module fitz, but at the point the classic implementation is pretty much deprecated, so i'd prefer not to do this.

• Building the mupdf C++ and Python language bindings requires at least three separate patches to

  • disable the forced use of venvs which download stuff during build

Agreed.

I've modified my MuPDF tree so that VENV_FLAG= removes the --venv when calling ./scripts/mupdfwrap.py. Hopefully this will avoid the need for this patch. The default is set using ?= so VENV_FLAG can be set in the environment or as a make parameter.

• honor LDFLAGS when building C++ bindings so that they have full RELRO

Agreed.

I've modified my MuPDF tree to add $LDFLAGS to link commands when building the bindings. [This will include the Python bindings library, _mupdf.so.]

• fix the install-shared-* targets to not call each other and fail on attempting to reinstalling files multiple times, reduce the install calls, prevent everything being rebuilt before installation and have the libmupdfcpp.so installed properly (with symlink)

I don't understand why you've seen these problems, the existing targets work fine for me and do not result in duplication that i can see, and we run regular tests that use the default make -j N (where N is the number of CPU cores).

Also, if i've understood this patch and the associated build command in PKGBUILD (make shared=yes build=release libs apps c++ python) correctly, it will now almost certainly break with make -j because the libs, c++ and python targets are highly dependent on each other - python requires c++ and c++ requires libs, as expressed in the original rules.

Regarding the related patch which changes Makefile's install commands such as:

-install-shared-c: install-shared-check shared install-headers
-	install -d $(DESTDIR)$(libdir)
-	install -m 644 $(OUT)/libmupdf.$(SO)$(SO_VERSION) $(DESTDIR)$(libdir)/
+install-shared-c: install-shared-check install-headers
+	install -vDm 755 $(OUT)/libmupdf.$(SO)$(SO_VERSION) -t $(DESTDIR)$(libdir)/
 ifneq ($(OS),OpenBSD)
 	ln -sf libmupdf.$(SO)$(SO_VERSION) $(DESTDIR)$(libdir)/libmupdf.$(SO)
 endif

• In my tree's Makefile, i've added support for new variables INSTALL_MODE, so INSTALL_MODE=755 will use install -m 755 ... instead of -m 644. On Devuan at least, files in /usr/lib seem to be mode 0644, but this should allow the required customisation here without a patch. The default is set using ?= so INSTALL_MODE can be set in the environment or as a make parameter.

• The above patch also seems to change how we install *.so.<version>, for example copying libmupdf.so (which will be a softlink) to$(DESTDIR)/libmupdf.so.1.23.9. As far as i can tell, this gives the same result as before, but arguably is confusing as it relies on install evaluating softlinks. So i don't think this part of the patch is necessary.

• I was unable to have full RELRO for the Python language binding. In case you know how to achieve this with swig, I'd be very interested to hear your thoughts.

My change described earlier to also use $LDFLAGS when linking the bindings, will also apply to the link command that creates the Python shared library _mupdf.so. Hopefully that will do what you require here.

• edit: Somewhat unrelated we have just found, that building mupdf with more than one build job is brittle and breaks on some systems due to concurrency issues (linker calls fail). We have now limited the build to one job (-j1) because of this.

As described earlier, i think this is an inevitable consequence of the changes to the Makefile rules in mupdf-1.23.9-install_targets.patch (which i think are unnecessary).

You can find what is currently required here: https://gitlab.archlinux.org/archlinux/packaging/packages/mupdf/-/tree/1861ae1e50532cc903f6cab66a3377a2e6204863 I can provide these patches to the mupdf upstream repo if you want.

Building with PYMUPDF_SETUP_IMPLEMENTATIONS=b now works for me and you can find the relevant files here: https://gitlab.archlinux.org/archlinux/packaging/packages/python-pymupdf/-/tree/a645b56162a3a4aaae7c440ad195fffeb602c7db I have disabled a few more tests, as I didn't have the energy to further work on these packages, which have now eaten quite a lot of my free time.

This is good to know.

Note that PyMuPDF 1.23.9 removed all use of pip install in tests (as a result of your issue #2950), so hopefully you'll just need to exclude the tests listed in https://pymupdf.readthedocs.io/en/latest/packaging.html.

The integration is unfortunately still not where it could be and it would be great if the relevant mupdf make targets and scripts could be fixed.

Thanks for the detailed report, i appreciate the effort that you're putting in here to make things work.

Hopefully the MuPDF changes i've described will give you what you need without any need for patches.

I'll try to get them into MuPDF in the next few days.

In the meantime, if you are able to try things out, i've pushed my MuPDF tree to: https://github.com/ArtifexSoftware/mupdf-julian

[dvzrv]

In my tree's Makefile, i've added support for new variables INSTALL_MODE, so INSTALL_MODE=755 will use install -m 755 ... instead of -m 644. On Devuan at least, files in /usr/lib seem to be mode 0644, but this should allow the required customisation here without a patch. The default is set using ?= so INSTALL_MODE can be set in the environment or as a make parameter.

Hm, INSTALL_MODE is a bit generic though. Maybe SO_INSTALL_MODE? Looking at https://github.com/ArtifexSoftware/mupdf-julian/commit/9503301d24ae768d3bb3f7b79131dad17aaccff9 I can see that you are blanket applying this to all files. That's definitely not what we want though! My fix was specifically only for the shared object files! :)

The above patch also seems to change how we install *.so., for example copying libmupdf.so (which will be a softlink) to$(DESTDIR)/libmupdf.so.1.23.9. As far as i can tell, this gives the same result as before, but arguably is confusing as it relies on install evaluating softlinks. So i don't think this part of the patch is necessary.

No, it only calls install only once, by using -t. Additionally we are not re-calling all the build targets anymore and we are only ever calling install-headers once, because the install targets are not relying on one another anymore.

FTR: The issue with the install targets has been, that they call build again and that they rely on calling one another, instead of requiring the output of another install target. Calling e.g. install-shared-c and install-shared-c++ together or after one another was not possible because the install-headers target would be triggered multiple times and then fail on trying to install already existing files. Generally, it is much cleaner to have these targets all separately do what they are supposed to do (e.g. build a thing, then install a thing). If an install target should rely on a build target having been run, then it needs to rely on its output, not on it being called again. Without the patch to the install target, the build was rerun during installation. That's not what should happen!

I don't understand why you've seen these problems, the existing targets work fine for me and do not result in duplication that i can see, and we run regular tests that use the default make -j N (where N is the number of CPU cores).

Also, if i've understood this patch and the associated build command in PKGBUILD (make shared=yes build=release libs apps c++ python) correctly, it will now almost certainly break with make -j because the libs, c++ and python targets are highly dependent on each other - python requires c++ and c++ requires libs, as expressed in the original rules.

Maybe there has been a misunderstanding about what I wrote: We had to disable building multiple jobs (-j N) and use one job (-j 1) because calling the build target was failing on multiple other machines (interestingly not my own). We've tested this on 6 core, 12 core (works), 48 core machines until arriving at that conclusion. The install targets (should) have nothing to do with this really, as they are being called after the build targets and we were seeing the linker issues right when e.g. mutools was attempted to be built.

I know this is not the right place, but looking at these types of concurrency issues, I can recommend looking into other build systems that handle this more gracefully (e.g. meson).

As a separate remark: It is odd having to set LINUX_OR_OPENBSD=yes, so that the Makefile would actually create symlinks for the shared object files. Looking at https://github.com/ArtifexSoftware/mupdf-julian/commit/82db7fcd5bdf59652d03a56b06bf2e853140282b I wonder why the detection is not automated instead (it seems to work for Linux). However, that commit does seem to fix the symlink issue I have been seeing with libmupdfcpp.so (from only looking at the change).

Thanks for looking also into the other fixes!

[julian-smith-artifex-com]

Hm, INSTALL_MODE is a bit generic though. Maybe SO_INSTALL_MODE? Looking at ArtifexSoftware/mupdf-julian@9503301 I can see that you are blanket applying this to all files. That's definitely not what we want though! My fix was specifically only for the shared object files! :)

Ah, i should have noticed your changes were only for shared librarues. I've fixed my tree to use SO_INSTALL_MODE and only for shared libraries.

The above patch also seems to change how we install *.so., for example copying libmupdf.so (which will be a softlink) to$(DESTDIR)/libmupdf.so.1.23.9. As far as i can tell, this gives the same result as before, but arguably is confusing as it relies on install evaluating softlinks. So i don't think this part of the patch is necessary.

No, it only calls install only once, by using -t. Additionally we are not re-calling all the build targets anymore and we are only ever calling install-headers once, because the install targets are not relying on one another anymore.

Ok, i understand why you might want to use install -t to avoid the need for a separate initial install -d.

But my question (and this isn't particularly important, so feel free to ignore) was why change this:

install $(OUT)/libmupdfcpp.$(SO)$(SO_VERSION) $(DESTDIR)$(libdir)/

to:

install $(OUT)/libmupdfcpp.$(SO) $(DESTDIR)$(libdir)/libmupdfcpp.$(SO)$(SO_VERSION)

Expanding in a particular case, we start with:

build/shared-release/
    libmupdf.so.24.0
    libmupdf.so -> libmupdf.so.24.0

The original install command is:

install build/shared-release/libmupdf.so.24.0 /usr/local/lib/

And your patch changes this to:

install build/shared-release/libmupdf.so /usr/local/lib/libmupdf.so.24.0

As i said, it's not particularly important because the end result is the same.

FTR: The issue with the install targets has been, that they call build again and that they rely on calling one another, instead of requiring the output of another install target. Calling e.g. install-shared-c and install-shared-c++ together or after one another was not possible because the install-headers target would be triggered multiple times and then fail on trying to install already existing files. Generally, it is much cleaner to have these targets all separately do what they are supposed to do (e.g. build a thing, then install a thing). If an install target should rely on a build target having been run, then it needs to rely on its output, not on it being called again. Without the patch to the install target, the build was rerun during installation. That's not what should happen!

I agree it can be preferable to make rule B depend on the output of rule A rather than rule A itself. But it can be non-trivial with rules that generate multiple files, and with makefiles simplicity is key i think.

And as long as rule commands can be re-run, perhaps unnecessarily, without breaking things, then everything will work fine.

Which leads to where you say:

the install-headers target would be triggered multiple times and then fail on trying to install already existing files

Why would this rule's install commands fail if the destination files already exist? In general Makefile rules often have to deal with overwriting generated files, so i'm not understanding why it's going wrong for you.

With your patch it looks like, for exampe, the make install-shared-c++ rule will fail if one hasn't previously done make install-shared-c. This is surely not ok - the whole point of Makefile rules is to avoid this sort of problem. And i much prefer to have the build err on the side of potentially running more commands than is strictly necessary, than not building enough like this.

[I guess all we actually need is to make make install-shared-c++ depend on install-shared-check shared, but in practise it seems simpler to use install-shared-c.]

Maybe there has been a misunderstanding about what I wrote: We had to disable building multiple jobs (-j N) and use one job (-j 1) because calling the build target was failing on multiple other machines (interestingly not my own). We've tested this on 6 core, 12 core (works), 48 core machines until arriving at that conclusion. The install targets (should) have nothing to do with this really, as they are being called after the build targets and we were seeing the linker issues right when e.g. mutools was attempted to be built.

Ah, i see, it's the basic MuPDF C build that's going wrong. If you have the logs available, i'd like to take a look, there might be enough information there to figure out where the dependencies are going wrong.

I know this is not the right place, but looking at these types of concurrency issues, I can recommend looking into other build systems that handle this more gracefully (e.g. meson).

I agree make is a pain in many ways for things like this, but it also has advantages for something like MuPDF; the MuPDF people will probably make a good case for why they use make if you were to ask them.

As a separate remark: It is odd having to set LINUX_OR_OPENBSD=yes, so that the Makefile would actually create symlinks for the shared object files. Looking at ArtifexSoftware/mupdf-julian@82db7fc I wonder why the detection is not automated instead (it seems to work for Linux). However, that commit does seem to fix the symlink issue I have been seeing with libmupdfcpp.so (from only looking at the change).

Oh, you shouldn't have to set LINUX_OR_OPENBSD - it is set automatically in the included Makerules with:

ifeq ($(OS),Linux)
    LINUX_OR_OPENBSD := yes
endif
ifeq ($(OS),OpenBSD)
    LINUX_OR_OPENBSD := yes
endif

Thanks for looking also into the other fixes!

Welcome :-)

[julian-smith-artifex-com]

Closing this because no new posts in a long time. Please feel free to reopen as necessary.