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

Make sure third-party libraries are found in debug build #8016

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mrbean-bremen
Copy link

@mrbean-bremen mrbean-bremen commented Apr 25, 2024

  • _find_library_file now also checks debug libraries if in debug mode and returns the library name
  • the returned library name is used for linking instead of the hardcoded name

We build Pillow in debug mode and had the problem that the debug libraries (ending with "_d") were not found.
Using this patch fixed this.

I did not add release notes, as this is internal build stuff only - let me know if they are needed (in case this PR will get accepted).

- _find_library_file now also checks debug libraries
  if in debug mode and returns the library name
- the returned library name is used for linking
  instead of the hardcoded name
# as the library prefix differs depending on library type and platform,
# we ignore it by looking for the actual library name
start_index = lib_name_base.find(library)
return lib_name_base[start_index:]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit hacky, but I didn't find a better way to get the library name from the file name. I could not use splitext, as that would not handle something like libfoo.so.6.
As this is an internal function only, I hope it is ok.

@radarhere
Copy link
Member

Are you able to put together a Dockerfile that demonstrates the need for this?

@mrbean-bremen
Copy link
Author

Are you able to put together a Dockerfile that demonstrates the need for this?

Sorry, missed that comment.

I have to check... we are building using conan (we also build the other dependencies), that would probably be overkill here. I first thaught to add a CI wheel build in debug mode, but that is hardly needed by anyone, so it would only make the build process even longer. Will think about it...

@mrbean-bremen
Copy link
Author

The problem happens only if building against the debug libraries, which is not a common scenario (especially under Linux), except you build the libraries yourself (which we do). To reproduce this in a container, we would have to do this first (e.g. build the thirdparty libraries), as to my knowledge there is no ready container that already has them. As I wrote, we use conan to build the thirdparty libraries (and pillow) and store the build libraries in an artifactory, which would not be an option in the CI.

So, at the moment I don't have an idea how to test this with reasonable effort. I still think that the scenario is not completely outlandish, and it would make sense to support it (given that the underlying find_library_file from distutils supports it), but I can understand if you don't want to merge it without a test. We would patch it ourselves in this case.

And, by the way, thanks for all the work you put into this project, and for the fast and helpful feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants