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

add sdl2_image/2.0.5 and sdl2_mixer/2.0.4 #1317

Merged
merged 22 commits into from
Jul 21, 2021
Merged

add sdl2_image/2.0.5 and sdl2_mixer/2.0.4 #1317

merged 22 commits into from
Jul 21, 2021

Conversation

Croydon
Copy link
Member

@Croydon Croydon commented Dec 13, 2020

@Croydon
Copy link
Member Author

Croydon commented Dec 13, 2020

Hm. I do except a few build errors, but apparently it can't find SDL2 packages for any configuration.

Do we need to rebuild SDL2?

@Croydon
Copy link
Member Author

Croydon commented Dec 13, 2020

conans.errors.ConanException: Missing prebuilt package for 'libmikmod/3.3.11.1@bincrafters/stable', 'libmpg123/1.25.13@bincrafters/stable'

I don't understand why we suddenly lack many packages 🤔

@ericLemanissier
Copy link
Contributor

ericLemanissier commented Dec 14, 2020

it could be a consequence of conan-io/conan-center-index@8fa0843

Would it be possible to make a PR per package in the future, because here it's hard to keep track of which CI job failed

@Croydon
Copy link
Member Author

Croydon commented Dec 14, 2020

Would it be possible to make a PR per package in the future, because here it's hard to keep track of which CI job failed

Sure, I just wanted to test once with a real example that it works to update several recipes at once. This will be handy when it comes to mass convention updates

@Croydon
Copy link
Member Author

Croydon commented Dec 14, 2020

Still the same error

it could be a consequence of conan-io/conan-center-index@8fa0843

The PR does not seem to have changed the default dependencies or the dependency versions and both libmikmod and libmpg123 are now rebuild

@ericLemanissier
Copy link
Contributor

ericLemanissier commented Dec 14, 2020

I can see missing binaries for SDL2. the Clang5 and GCC7 build of libmikmod failed on https://github.com/bincrafters/community/actions/runs/420980056

Still the same error

it could be a consequence of conan-io/conan-center-index@8fa0843

The PR does not seem to have changed the default dependencies or the dependency versions and both libmikmod and libmpg123 are now rebuild

yes, it's the option modification which invalidated hashes

@Croydon
Copy link
Member Author

Croydon commented Dec 14, 2020

the Clang5 and GCC7 build of libmikmod failed on https://github.com/bincrafters/community/actions/runs/420980056

I restarted. Now only GCC 9 and Clang 9 are missing, due to the container image issues

sdl2_mixer fails on all Linux configurations

@ericLemanissier
Copy link
Contributor

@Croydon
Copy link
Member Author

Croydon commented Dec 15, 2020

I believe I had looked at the logs from the previous commit again and came to the conclusion that it is still the same error.

I think you are right. But probably it is better to rewrite the test_package in a way that works for all cases, being it local, Bincrafters' CI and C3I.

@Croydon
Copy link
Member Author

Croydon commented Dec 15, 2020

Maybe the missing packages are caused by our regular connection timeouts to Bintray, and we didn't notice this before so strongly because we used outdated packages (which we are now getting deleted again).

@ericLemanissier
Copy link
Contributor

Yes, few minutes ago I manually restarted the SDL2 merge commit build cab627a because the clang 7 build originally failed with a timeout. it should be ok now so I just restarted this PR's builds

@Croydon
Copy link
Member Author

Croydon commented Dec 15, 2020

fluidsynth packages are missing too. I just started a new build there too

@ericLemanissier
Copy link
Contributor

So now we only have the MSVC link error of sdl2_image test recipe SDL2.lib(SDL_string.obj) : error LNK2005: memset already defined in vcruntime.lib(VCRUNTIME140.dll) [D:\a\1\s\recipes\sdl2_image\all\test_package\build\00e278e71bbf6eaa9abab912b397d40627769cc4\test_package.vcxproj] , I'm not sure what can be done

@Croydon
Copy link
Member Author

Croydon commented Dec 15, 2020

Neither do I. This error is around for a long time and I believe that was even discussed upstream once. Let me look if I can find that again

@Croydon
Copy link
Member Author

Croydon commented Dec 15, 2020

We are adding a patch, defining memset to SDL2

Upstream seems to have fixed that in the meantime, there are several commits regarding memset https://github.com/SDL-mirror/SDL/commits/master/src/stdlib/SDL_string.c

I'm not sure yet which parts should be backported in a patch

@Croydon
Copy link
Member Author

Croydon commented Dec 15, 2020

https://developercommunity.visualstudio.com/content/problem/711317/unexpected-implicit-memset-in-release-optimized-bu.html
https://stackoverflow.com/questions/58288692/cant-build-solution-in-release-mode-for-sdl-library-on-vs-2019

memset is defined in the vcruntime

SDL does not want to link against vcruntime by default. The preprocessor flags HAVE_MEMSET do fail for some reason.

Two ways to solve this:

  • define memset via patch ourselves (that is what we currently do)
  • link SDL against vcruntime

Our current way works for SDL itself. But SDL_image links against vcruntime. Hence we have two definitions of memset and it fails.

in the next versions

right now we use custom CMake files
Comment on lines 102 to 106
self._cmake.definitions["TIF_DYNAMIC"] = self.options["libtiff"].shared if self.options.tif else False
self._cmake.definitions["JPG_DYNAMIC"] = self.options["libjpeg"].shared if self.options.jpg else False
self._cmake.definitions["PNG_DYNAMIC"] = self.options["libpng"].shared if self.options.png else False
self._cmake.definitions["WEBP_DYNAMIC"] = self.options["libwebp"].shared if self.options.webp else False
self._cmake.definitions["SDL2_DYNAMIC"] = self.options["sdl2"].shared
Copy link
Contributor

@SpaceIm SpaceIm Feb 6, 2021

Choose a reason for hiding this comment

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

It's not because libtiff is shared that consumer wants dynamic load. In upstream configure.in, there is a specific option (true by default) for each dependency for which sdl2_image supports dynamic load (libtiff, libjpeg, libpng and libwebp).
So, you can link to static, to shared, or dynamically load shared.

Moreover, there is no dynamic load of sdl2 in sdl2_image, it's always linked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the catch. We are currently using a custom CMake file for sdl2_image, only the next version will have official CMake support.

I will rename the SDL2_DYNAMIC option name as it only seems to make a switch between static and shared linking. Not entirely sure how to proceed with the rest.

Is dynamic loading just working, despite our custom CMake file?
If yes, we don't necessarily need special handling I guess.
If not, it doesn't work right now

Copy link
Contributor

@SpaceIm SpaceIm Jul 21, 2021

Choose a reason for hiding this comment

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

From what I see in this custom CMake file, nothing is done with these definitions (except SDL2_DYNAMIC, but it's a different purpose and this CMake option could be avoided), so well let's say that the "dynamic" options in upstream configure.in is not supported for the moment in this recipe.

Is dynamic loading just working, despite our custom CMake file?

It can't work with this custom CMakeLists, there is an explicit branching with macros for dynamic load handling at runtime:
https://github.com/SDL-mirror/SDL_image/blob/b4f28c1fc9ab2f91ae594fc3e86622c5c13ffbbb/configure.in#L358-L364
https://github.com/SDL-mirror/SDL_image/blob/f4f5caf21d6f18de85baee945348f56697d1813a/IMG_jpg.c#L70-L77

@ericLemanissier

This comment has been minimized.

Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>
@ericLemanissier
Copy link
Contributor

So we are now missing binaries for fluidsynth => #1404
and for sdl => #1405

@Croydon
Copy link
Member Author

Croydon commented Jul 20, 2021

Damn. I need to update the CI files. But I'm really confused why this error did not happen sooner

@ericLemanissier
Copy link
Contributor

Which files are you talking about?

@Croydon
Copy link
Member Author

Croydon commented Jul 21, 2021

I was talking about the GitHub Workflow CI file. The push CI jobs fail, because this branch is too old and does not have the newest files, which was confusing me for a moment. Let's rebase this branch.

Edit: Or lets just ignore those CI jobs for the moment :D

@Croydon
Copy link
Member Author

Croydon commented Jul 21, 2021

While there are still a lot of small and huge things to improve, I would be fine with merging this state finally

@CroydonBot CroydonBot merged commit 72ff676 into main Jul 21, 2021
@CroydonBot CroydonBot deleted the sdls branch July 21, 2021 09:07
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.

4 participants