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

Remove cython gen files, generate in meson build #2831

Merged
merged 2 commits into from
May 11, 2024
Merged

Conversation

ankith26
Copy link
Member

Tracking cython generated files on git certainly has its advantages (makes builds faster, reduces a compile time dependency, etc), it also has certain disadvantages like us having to push cython changes for every edit we do, and also us having to push cython changes when we have to support a new python version or something.

I think going forward we should not track cython files on git. Now that we have the pyproject.toml based setup, builds will take care of the cython install by default.

Here is the new logic that the meson build uses to handle the cython files: if the cython generated C file is missing, builds the module directly from the cython pyx source file. However if it finds the generated C file in the source tree, it compiles that file instead. The reason for doing this is so that if a developer needs to manually tweak a cython file for whatever reason, they may generate the cython file themselves and then do any tweaks they want.

This PR is the meson-buildconfig based successor to #1877

@ankith26 ankith26 requested a review from a team as a code owner April 28, 2024 12:09
@ankith26 ankith26 force-pushed the ankith26-remove-cython branch from f06a334 to 5096473 Compare April 28, 2024 12:20
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.

LGTM. I'm a general fan of getting rid of cython in the long term anyway so any steps down that long, long road are welcome. Should make searching/refactoring the code in an IDE nicer as well without throwing up lots of bits of code in the generated cython files.

@Starbuck5
Copy link
Member

I don't understand how we can do this. Don't we still use setup.py on certain platforms? Does this remove setup.py support or require a manual build step before running setup.py?

@ankith26
Copy link
Member Author

ankith26 commented Apr 29, 2024

Don't we still use setup.py on certain platforms? Does this remove setup.py support or require a manual build step before running setup.py?

This PR in it's current state requires python3 setup.py cython to be run instead of just python3 setup.py. Though I could change this by cherry picking a few commits from yunline's older PR.

EDIT: I am updating setup.py to exactly match what the meson buildconfig does

@ankith26 ankith26 force-pushed the ankith26-remove-cython branch 4 times, most recently from d617df0 to 1391cb4 Compare April 29, 2024 20:12
Copy link
Member Author

Choose a reason for hiding this comment

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

Why am I doing changes in this file?

Well these changes are not technically needed to make this PR pass this CI, but this is the only file still using the legacy setup.py based install so naturally I had to take a look here. Now, all this PR does here is bumps cython to the stable 3.0.10 release, the older code was building cython from source based on some commit in the cython 3.0.0 development history.

@ankith26 ankith26 force-pushed the ankith26-remove-cython branch from 1391cb4 to fe3d546 Compare April 29, 2024 20:22
Copy link
Member

@oddbookworm oddbookworm left a comment

Choose a reason for hiding this comment

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

LGTM! But I'm not pushing the merge button quite yet because it seems like @Starbuck5 might still have some reservations

@oddbookworm oddbookworm added build Compiling stuff meson Meson build system labels May 8, 2024
@Starbuck5
Copy link
Member

Ok, I've done some testing.

On my system, compiling pygame-ce takes 35 seconds, and this PR makes it take 5-10 seconds longer.

It seems like the meson build system should be in charge of tracking targets and recompiling only when necessary. Like if the pyx file to c file conversion was a target, the output is gitignored, and then that target is compiled into a py extension. It would be nice to keep the fast build advantage of the cached cython even when it is not committed.

@Starbuck5
Copy link
Member

Ok, in C projects I've used meson with, it stores intermediate steps in a permanent folder and can recompile only the changed objects very quickly.

I found meson-python settings about that: https://meson-python.readthedocs.io/en/latest/reference/config-settings.html#cmdoption-arg-build-dir

Currently it has uses a temporary build folder but it can allegedly be set to a permanent one, and I bet that would help with recompile times.

There's also editable installs, on the vein of improving our iteration times.

Since there isn't an easy way of keeping the cython speed and it's not too bad of a slowdown, I'm okay merging this PR.

@Starbuck5 Starbuck5 added this to the 2.5.0 milestone May 10, 2024
@ankith26
Copy link
Member Author

It seems like the meson build system should be in charge of tracking targets and recompiling only when necessary.

So now that we have the meson build system, "editable" installs are supported out of the box, and this a must know for all pygame-ce contributors (I forgot to mention it earlier).

So the thing with pip install . now is that it always creates an isolated venv, installs its dependencies, rebuilds the whole project from scratch and then discards the venv, so essentially there is no caching between builds. But this command is actually meant to be run on places like CI, not a development environment.

Whereas devs should be using the "editable" install: python -m pip install --no-build-isolation --editable ., which is a command you only have to run once, and then the install just keeps track of all your changes, and dynamically rebuilds the changed stuff on import, while caching the unchanged files (so this is very fast, and saves you the effort of having to do a manual reinstall).

I should probably update this info on a wiki somewhere, and let everyone know of this.

@ankith26
Copy link
Member Author

PS: I didn't see your latest comment before putting up my above comment :)

@ankith26 ankith26 merged commit 7a6d4a7 into main May 11, 2024
39 checks passed
@ankith26 ankith26 deleted the ankith26-remove-cython branch May 11, 2024 04:23
@ankith26 ankith26 mentioned this pull request Jul 8, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Compiling stuff meson Meson build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants