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

mac/manylinux deps updates 2024 #3118

Merged
merged 8 commits into from
Oct 13, 2024
Merged

mac/manylinux deps updates 2024 #3118

merged 8 commits into from
Oct 13, 2024

Conversation

ankith26
Copy link
Member

@ankith26 ankith26 commented Sep 24, 2024

It's that time of the year again, this is the deps update PR of 2024.

Most of the dependency updates are straightforward, the only non-trivial changes here are change in buildsystem for

  • SDL and friends: SDL3 is going to exclusively support cmake, so I figured we should make the jump right away. I believe it is better maintained these days anyways.
  • harfbuzz now supports meson only
  • flac/ogg/vorbis had some issues, so jump to CMake on that one as well
  • Also fixed a couple of other misc issues with libxmp and LD_LIBRARY_PATH handling

@ankith26 ankith26 requested a review from a team as a code owner September 24, 2024 18:05
@ankith26 ankith26 force-pushed the ankith26-deps-updates2024 branch from 2812689 to ba5a7f7 Compare September 24, 2024 20:10
@ankith26 ankith26 force-pushed the ankith26-deps-updates2024 branch from ba5a7f7 to 02da12f Compare September 24, 2024 20:57
@yunline yunline added dependencies build Compiling stuff labels Sep 25, 2024
@ankith26 ankith26 force-pushed the ankith26-deps-updates2024 branch from 02da12f to 005a566 Compare September 25, 2024 06:50
@ankith26 ankith26 force-pushed the ankith26-deps-updates2024 branch from 005a566 to 83380ed Compare September 25, 2024 06:57
@ankith26
Copy link
Member Author

This PR slightly increases wheel size, but that is because so far SDL and friends were probably not being compiled with optimization flags, but now they are. So that is a plus of this PR.

Also, I am hopeful that this PR should help with reviving my win deps PR from last year (#2505)

Copy link
Member

Choose a reason for hiding this comment

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

The fact that we're building some of these libraries ourselves at all is crazy.

Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

Alright, all looks good to me, but holy heck that is a lot of version bumps. Still the CI is happy, so I'm happy?

I can also reveal that non of these many linux build changes had any effect whatsoever on the windows build (which I'm sure you were wondering).

@Starbuck5
Copy link
Member

Going through a wheel produced on this branch and a wheel produced on main recently, I notice the one on main has libxmp in the pygame_ce.libs folder, and the wheel from this branch does not.

Is that a problem?

@ankith26
Copy link
Member Author

I noticed that too. But then I realised that it is somewhat intentional with the cmake build: libxmp gets statically linked against SDL_image. You can also see that SDL_image size goes up the same amount as the old libxmp shared lib.

We have a libxmp unittest, that passed on this branch too, so we should be all good here

@Starbuck5
Copy link
Member

You mean SDL_mixer.

Now 75% (at least) of SDL_mixer's file size is this one library! And they're bigger together than they were separately!*

Also a test run of the dep on the machine that had all the dev libraries to build the thing in the first place is less convincing.

*assuming the switch to position independent code in libxmp compilation didn't grab the 10 additional kb

@ankith26
Copy link
Member Author

Also a test run of the dep on the machine that had all the dev libraries to build the thing in the first place is less convincing.

We never need to worry about this on macs and linuxs because

  • mac builds delete all the dev files on the CI before running the tests
  • I tested the linux wheels locally and saw no issues

Also like I mentioned above

This PR slightly increases wheel size, but that is because so far SDL and friends were probably not being compiled with optimization flags, but now they are. So that is a plus of this PR.

I attribute binary size increases to this

@Starbuck5
Copy link
Member

I tested the linux wheels locally and saw no issues

You also have the libraries installed to build a bunch of things from source, so that's not convincing either.

I believe you that it works, but because of the filesize, not either of those tests.

@ankith26
Copy link
Member Author

trust auditwheel and delocate devs 😄

So far the issue you have talked about has only ever bitten us on windows (some random missing DLL), atleast in my memory

@Starbuck5
Copy link
Member

Starbuck5 commented Oct 12, 2024

Also would it be easy to not have libxmp statically in SDL_mixer? It's the only one that is, this seems like it could confuse us in the future.

Or add some comments at least?

@ankith26
Copy link
Member Author

I can open an issue/PR to SDL_mixer about it, I feel like they would also see it as an inconsistency

@Starbuck5
Copy link
Member

CMake doesn't give us control of that?

Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

Thanks for working this up!

@Starbuck5 Starbuck5 added this to the 2.5.2 milestone Oct 13, 2024
@Starbuck5 Starbuck5 merged commit 40e7671 into main Oct 13, 2024
24 of 25 checks passed
@Starbuck5 Starbuck5 deleted the ankith26-deps-updates2024 branch October 13, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Compiling stuff dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants