Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restored CMAKE_INSTALL_LIBDIR to libjpeg-turbo #537

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

radarhere
Copy link
Collaborator

@radarhere radarhere commented Oct 23, 2024

#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
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator

mattip commented Oct 24, 2024

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
Copy link
Collaborator

mattip commented Oct 24, 2024

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
Copy link
Contributor

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.

@mattip mattip merged commit 22e8ad0 into multi-build:devel Oct 25, 2024
4 checks passed
@radarhere radarhere deleted the openjpeg branch October 25, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants