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 meson build system [phase 1] #2557

Merged
merged 8 commits into from
Apr 12, 2024
Merged

Add meson build system [phase 1] #2557

merged 8 commits into from
Apr 12, 2024

Conversation

ankith26
Copy link
Member

@ankith26 ankith26 commented Nov 9, 2023

This is intended to be phase 1 of a 4 phase PR I plan to make over the months.

Why this change?

Well, distutils is not going to be with us forever. Python 3.12 has officially removed it, though at the moment the setuptools package still provides it, though this could change in the future.

Most of the big C-API using python projects (numpy, scipy, etc) seem have gone the same way already, so following their footsteps should be safe.

This is intended to improve a lot of things, some benefits listed below

  • hopefully smaller code footprint than old buildconfig
  • dependency handling more generic, simple and correct
  • give more flexibility on per file flags without much hacks
  • in the long term, it should make it easier to support other build configurations

It is also worth mention a few potential and subjective downsides, and some of my own notes on this

  • meson does not give as much power as python? This is an intentional meson design choice. In most places we can make do with whatever meson provides, but incase we need to do something not possible within meson, we can always write a python script, and call it from meson.
  • The meson code looks very explicit? We are manually mentioning every file/folder that needs to be bundled/built when previously we just used globs? Again, intentional meson design choice. Kinda inline with pythons own "explicit is better than implicit".
  • This adds a build time dependency? Can it make builds slower or harder? Yes it does add a build-time dependency. But the nice part of using pyproject.toml is that a single pip wheel ./pip install . will just do the right thing and handle the meson build dependency. This does add a bit of build overhead before the actual compilation, but then the actual compilation should be faster (another neat thing, the builds would now default to using all cores on a machine by default, no need for extra flags)

Roadmap

Here is a phase-wise tentative plan, it is intentionally "slow" (it is aligned to when I get free time lol)

Phase 1 (December 2023)

That would be this PR. The goal of this PR is to just introduce the new buildconfig, and keep its logic as identical to the old buildconfig as possible.

This PR adds support for all common platforms, that is, binary distributions on windows, mac and manylinux.

At this point the old buildconfig would be left as-is, so people could still use it if their specific workflow does not work. However, right from its introduction, the new buildconfig is going to take "precedence" over the old (by this I mean, all pip commands will stop running setup.py, if you need to use legacy buildconfig you would have to invoke it directly)

Phase 2 (May 2024 or earlier)

The goal of this phase is to remove the need for legacy buildconfig for anything build-related. In this phase, the old buildconfig should start to throw deprecation warnings. Other changes here could be

  • Implement sdist wheel generation
  • Add support for platforms not covered in phase 1 (most notably, android and wasm) and I might need help on this.
  • Implement and improve windows dependency handling in a way independent from legacy buildconfig
  • cython files to be generated at build time by this point

Phase 3 (July 2024 or earlier)

The goal of this phase is to remove the need for legacy buildconfig entirely.

  • Implement the remaining legacy buildconfig/setup.py features like python setup.py docs|lint|format|stubcheck

Phase 4 (November 2024 or earlier)

Removal of the legacy buildconfig.

@ankith26 ankith26 force-pushed the ankith26-build-meson branch 17 times, most recently from 9133076 to d981699 Compare November 18, 2023 12:37
@Starbuck5
Copy link
Member

Gave this a quick test on my system.

I needed to run pip install . from the x64 native tools command prompt, so it would pick up MSVC (cl) before my local installation of Mingw. It didn't work to just add MSVC to the path, I had to build in that entire environment.

I also noticed the addition of .lib files after the build process that makes the new installation slightly larger:
extralibs

@ankith26
Copy link
Member Author

ankith26 commented Nov 19, 2023

I needed to run pip install . from the x64 native tools command prompt, so it would pick up MSVC (cl) before my local installation of Mingw. It didn't work to just add MSVC to the path, I had to build in that entire environment.

Hmmm, perhaps in your global environment you have mingw "higher up" than msvc on PATH? Anyways I'm curious what that resulted in, I'm assuming the build failed for some reason or the other (like not finding dependencies), but I guess we could actually make it work somehow. If not in this PR maybe in a future one
EDIT: This PR now supports building on MinGW

I also noticed the addition of .lib files after the build process that makes the new installation slightly larger:

Yeah I don't know what's up with those. But those should only add a few kb atmost. Any larger difference you are seeing is probably coming in due to the fact that the new buildconfig bundles the tutorials that are under the docs (the old buildconfig has a bug/typo due to which it doesn't over there)

@ankith26
Copy link
Member Author

So it still tries to use mingw on my system, and still fails.

Hmm, that's probably happening due to an older mingw version AFAICT. I can add guards to prevent the error you are seeing.

After this PR, does the CI generated meson-built wheels?

Yes, it does.

@robertpfeiffer
Copy link
Contributor

None of the things I complained about are the fault of the PR, I just didn't understand that meson uses GCC bootstrap terminology.

@Starbuck5
Copy link
Member

After this PR, does the CI generated meson-built wheels?
Yes, it does.

That ups the amount of scrutiny a lot, because we need to be confident in these wheels being ready for release.

I built two wheels, one before this PR, one after, and went through them with a fine-toothed comb.

  • The pyd files in _sdl2 seem to be slightly larger on meson. Not sure what's up with that, if anything.
  • examples/ now has editorconfig copied in, normally doesn't
  • tests/fixtures/fonts now has PyGameMono.sfd copied in, normally doesn't
  • docs folder now has pyi files for all top level python files for some reason
  • docs/generated/_sources now has a c_api folder and a tutorial folder instead of just a ref folder
  • docs/generated now comes with the tutorials folder, it normally doesn't.

These differences end up as the meson build having 73 more files and taking up 1.2 mb more disk space than the standard build on my test.

@ankith26
Copy link
Member Author

The pyd files in _sdl2 seem to be slightly larger on meson. Not sure what's up with that, if anything.

I would attribute that to different compiler flags used by meson implicitly.

examples/ now has editorconfig copied in, normally doesn't

This file can probably be removed from the source code itself, what do you think?

tests/fixtures/fonts now has PyGameMono.sfd copied in, normally doesn't

This file is 6.5 KB so I don't think adding it should be a problem.

docs folder now has pyi files for all top level python files for some reason

This is where those pyi files should be. Is the legacy buildconfig ignoring these files entirely?

docs/generated/_sources now has a c_api folder and a tutorial folder instead of just a ref folder
docs/generated now comes with the tutorials folder, it normally doesn't.

I consider these changes a "fix" of this PR. At some point these files were infact bundled in our wheels, but then "tut" folder was renamed to "tutorials" while not changing the old buildconfig, so these files were accidentally removed.

@Starbuck5
Copy link
Member

This file can probably be removed from the source code itself, what do you think?

Probably

This is where those pyi files should be. Is the legacy buildconfig ignoring these files entirely?

image

I've never seen these pyi files before in my life. They're not in source control. https://github.com/pygame-community/pygame-ce/tree/main/docs

I consider these changes a "fix" of this PR. At some point these files were infact bundled in our wheels, but then "tut" folder was renamed to "tutorials" while not changing the old buildconfig, so these files were accidentally removed.

You're right, but I'm very file size sensitive right now given pypi support's lack of raising our limit, so it pains me to see it increase.

@ankith26
Copy link
Member Author

ankith26 commented Apr 2, 2024

I've never seen these pyi files before in my life. They're not in source control.

They are in source control, but here. They were recently added, and the author forgot to update buildconfig to handle the new subtree.

You're right, but I'm very file size sensitive right now given pypi support's lack of raising our limit, so it pains me to see it increase.

The real fix of course, is to move docs/tests/examples/stubs into a separate package as we've spoken about. I would like to work on that post this PR. In the mean time, it's probably fine to be leaving out this subtree in this PR.

I will make a new push with the changes.

@ankith26
Copy link
Member Author

ankith26 commented Apr 2, 2024

After the latest changes:

ankith@AnkithLaptop:~/Downloads/pygame-wheels$ wc -c 2.5dev/pygame_ce-2.5.0.dev1-cp310-cp310-win32.whl meson-pr/pygame_ce-2.5.0.dev1-cp310-cp310-win32.whl
11739744 2.5dev/pygame_ce-2.5.0.dev1-cp310-cp310-win32.whl 
11650529 meson-pr/pygame_ce-2.5.0.dev1-cp310-cp310-win32.whl

So essentially this PR is trimming about 89 KB for this particular wheel (mostly due to skipping headers in the wheels).

@Starbuck5
Copy link
Member

They are in source control, but here. They were recently added, and the author forgot to update buildconfig to handle the new subtree.

These are bizarre, this is not a public API, why would it get pyi files?

They cause the editor to know about these things:

from pygame import docs
docs.has_local_docs()

Which then crash:

pygame-ce 2.5.0.dev1 (SDL 2.28.5, Python 3.12.0)
Traceback (most recent call last):
  File "C:\Users\charl\Desktop\testr2.py", line 8, in <module>
    docs.has_local_docs()
    ^^^^^^^^^^^^^^^^^^^
AttributeError: module 'pygame.docs' has no attribute 'has_local_docs'

The real fix of course, is to move docs/tests/examples/stubs into a separate package as we've spoken about. I would like to work on that post this PR. In the mean time, it's probably fine to be leaving out this subtree in this PR.

Thank you. Yes I've been thinking about that as well. I'd like to keep stubs and tests in the main package. Docs work best standalone and provide the best benefit, I could see examples going with the docs. pygame-ce-docs on pypi?

@ankith26
Copy link
Member Author

ankith26 commented Apr 3, 2024

Which then crash:

I tested locally and I don't get the crash. How are you building the wheel?
I have by building off this PR and also the 2.4.1 release on PyPI

I'd like to keep stubs and tests in the main package

I mean, if we are going to create a new package with the aim of saving PyPI space, we might as well go all out and move as many things as possible. The reasoning for moving docs/examples/tests into a separate package is that the average user definitely doesn't have to care if these packages are installed or not. The stubs are in the gray area (useful for people to have installed), and I don't really have a strong opinion on where they should go.

@Starbuck5
Copy link
Member

Really, this doesn't crash for you?

I got pygame-ce from pip.

>>> from pygame import docs
pygame-ce 2.4.1 (SDL 2.28.5, Python 3.9.5)
>>> docs.has_local_path()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'pygame.docs' has no attribute 'has_local_path'
>>>

I have a strong opinion that the stubs should stay in this package.

@Starbuck5
Copy link
Member

Found this bit of docs on meson build about MSVC: https://meson-python.readthedocs.io/en/latest/how-to-guides/meson-args.html#force-the-use-of-the-msvc-compilers-on-windows

Tested this and confirmed it worked without CL needing to be in my path.

Putting cl in my path led to compile errors I wasn't able to resolve. This error: https://stackoverflow.com/questions/77029786/meson-error-compiler-cl-cannot-compile-programs-windows-10

@ankith26
Copy link
Member Author

ankith26 commented Apr 4, 2024

Really, this doesn't crash for you?

I got pygame-ce from pip.

>>> from pygame import docs
pygame-ce 2.4.1 (SDL 2.28.5, Python 3.9.5)
>>> docs.has_local_path()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'pygame.docs' has no attribute 'has_local_path'
>>>

I have a strong opinion that the stubs should stay in this package.

Well, you mistyped docs.has_local_path for docs.has_local_docs

@Starbuck5
Copy link
Member

Well, you mistyped docs.has_local_path for docs.has_local_docs

Well, that would do it.

My typo aside, I don't see why a non public API like this would have type stubs.

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.

The last test I wanted to do locally was make sure the SIMD dispatch still works,

Tested grayscale and alpha blitting with printfs internally, made sure it was still reaching those places properly.

Let's get this out! I think any teething problems will need to come up in usage and in time, rather than holding this PR in wait longer.

@MyreMylar MyreMylar merged commit b8120fb into main Apr 12, 2024
36 checks passed
@MyreMylar MyreMylar deleted the ankith26-build-meson branch April 12, 2024 08:14
@ankith26 ankith26 added this to the 2.5.0 milestone Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants