multi-build / multibuild

Machinery for building and testing Python Wheels for Linux, OSX and (less flexibly) Windows.
Other
241 stars 88 forks source link

Restored `CMAKE_INSTALL_LIBDIR` to libjpeg-turbo #537

Closed radarhere closed 1 month ago

radarhere commented 1 month ago

531 stated that

the CMAKE_INSTALL_LIBDIR value used for libjpeg-turbo is redundant, as the /lib suffix will be implied from the CMAKE_INSTALL_PREFIX value.

However, testing the updated multibuild with Pillow, I find that a job starts failing. If I restore CMAKE_INSTALL_LIBDIR and test that, the job passes.

If the only reason for removing the flag was that it is redundant and makes no difference, then I think I've demonstrated otherwise and would suggest it be restored.

freakboy3742 commented 1 month ago

My claim that it was redundant was based on my (admittedly, mostly on macOS) testing; plus my read of these docs, which seem to imply that the built-in default for library installs is lib.

Reading deeper into the GNUInstallDirs docs, I'm guessing the problem is that lib isn't always the default value - AlmaLinux 8/Centos 7 will be using lib64 as a default value. If the rest of Pillow's build system is relying on a predictable /lib suffix, then that would explain the problem you've observed, and why I didn't see any difference (most of my testing has been on macOS).

Regardless - I don't have any objection to the CMAKE_INSTALL_LIBDIR change being reverted; my only question would be whether the same option should be added to the other CMAKE usages (openjpeg and blosc), for exactly the same reason.

radarhere commented 1 month ago

If you think that the default behaviour of libjpeg-turbo is correct, and Pillow should be updated to handle it, that's fine - I was just saying that this change isn't without consequence, and wanted to make sure there was some motivation behind it.

freakboy3742 commented 1 month ago

My motivation was that it appeared to be redundant - and on macOS, it is. However, you've demonstrated it isn't on at least some Linuxes.

I'm currently elbow deep in Pillow's buildsystem; based on what I've seen so far, I'm sure there could be a fix in Pillow's build system - specifically, configuring BUILD_PREFIX to use lib64, rather than defaulting to lib. However the alternate fix is to revert the CMAKE_INSTALL_LIBDIR change as you've suggested - and that approach means there's no backwards incompatibility for any existing users.

However, if the fix is a revert, I'm arguing that the fix is also needed to the other CMAKE uses for consistency.

mattip commented 1 month ago

I think Pillow is a good use case, so maybe we can explore getting Pillow + multibuild working on a branch of both, and then resubmit needed fixes.

mattip commented 1 month ago

the fix is also needed to the other CMAKE uses for consistency

We have not so far received issues about CMAKE_INSTALL_LIBDIR, so I am inclined to merge this as-is, in the vein of "if it is not broken, don't touch".

freakboy3742 commented 1 month ago

We have not so far received issues about CMAKE_INSTALL_LIBDIR, so I am inclined to merge this as-is, in the vein of "if it is not broken, don't touch".

That sounds entirely reasonable to me. It's not impacting my usage, I was just flagging a potential inconsistency. If that inconsistency isn't proving a problem for others, I don't see a problem waiting until it does.