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

[sdl_image] Fix static sdl_image with shared sdl #22575

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Ahajha
Copy link
Contributor

@Ahajha Ahajha commented Jan 28, 2024

Specify library name and version: sdl_image/2.6.3

Resolves #22532

This only affects 2.6.3, neither 2.0.5 nor the upcoming 2.8.x is affected.

The issue was that sdl_image's CMake would try to read from ${SDL2_INCLUDE_DIR}/SDL_version.h, where the variable was a list, so it failed. The only thing this ultimately was used for was to get the version as a string, which Conan already provides as a variable directly.

This seems to also be bitten by #22485.
Xorg is used by pulseaudio and xcbcommon, which both specify xorg components, but don't have -Wl,--as-needed in their build process. Should be two simple PRs to get those fixed.


@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

recipes/sdl_image/all/conanfile.py Outdated Show resolved Hide resolved
danimtb
danimtb previously approved these changes Apr 25, 2024
jcar87
jcar87 previously requested changes Apr 25, 2024
Copy link
Contributor

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

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

(see comments)

@Ahajha
Copy link
Contributor Author

Ahajha commented Apr 25, 2024

@jcar87 What comments? Unless you're referring to @valgur's comment?

@jcar87
Copy link
Contributor

jcar87 commented Apr 25, 2024

@jcar87 What comments? Unless you're referring to @valgur's comment?

oops sorry! my comments regarding how --as-needed has been the default in most Linux distros for years (or should be!) - so it's surprising that it's needed (pun intended) at all - would like to investigate in case that's an actual issue with our gcc docker images

Edit: oh, apologies I didn't see the referenced issue here #22485

Reading that thread, I think it all points to our gcc/Docker images not passing this correctly - is this still a problem right now? that is, does the recipe not build on our CI without passing this --as-needed here?

@jcar87
Copy link
Contributor

jcar87 commented Apr 25, 2024

Continuing the discussion here: #22485 - really well diagnosed :D

As for this PR, does adding --as-needed fix a CI issue that prevents this PR from being merged? If that's the case let me know, otherwise I would suggest removing that line, as by all accounts the issue seems to be somewhere ese

@Ahajha
Copy link
Contributor Author

Ahajha commented Apr 25, 2024

I will check - I don't think it's entirely necessary at this point (there's been some changes to the xorg recipe since the issue arose). I will try removing it and see what happens.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 1 (1a4ea53b4595aae8d79eca62a411484b074812c0):

  • sdl_image/2.6.3:
    All packages built successfully! (All logs)

  • sdl_image/2.0.5:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 1 (1a4ea53b4595aae8d79eca62a411484b074812c0):

  • sdl_image/2.6.3:
    All packages built successfully! (All logs)

  • sdl_image/2.0.5:
    All packages built successfully! (All logs)

@jcar87 jcar87 dismissed their stale review April 25, 2024 19:06

Clarified

@Ahajha
Copy link
Contributor Author

Ahajha commented Apr 26, 2024

@jcar87 Seems like it works, perhaps it would be worth checking if the resulting binaries are still linking to all the extra xorg libraries. Maybe the issue still exists, just it doesn't actually cause a problem since all the requisite libraries are there now.

@jcar87 jcar87 self-assigned this May 20, 2024
@jcar87
Copy link
Contributor

jcar87 commented May 20, 2024

See my comment in #22532 (comment) -
static sdl_image and shared SDL fails because when SDL sdl_image is being built as static, it expects the SDL2::SDL2-static target to be satisfied by find_package(SDL2) - which is not the case when sdl is shared. we can fix this in sdl image by creating an alias (without a patch) in a single line, such that SDL2::SDL2-static is an alias to SDL2::SDL2, however this seems off - I'd be more inclined to mark this as an unsupported/invalid configuration in the validate() method - or to report this upstream first.

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.

[package] sdl_image/2.6.3: failed to build with sdl/*:shared=True
5 participants