Skip to content

Generate Cython code at build time #1877

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

Closed
wants to merge 15 commits into from

Conversation

yunline
Copy link
Contributor

@yunline yunline commented Feb 16, 2023

  • Generate Cython code at build time.
  • Remove cython option from setup.py. (but keep the cython_only)
  • Add cython-generated files into .gitignore .
  • Bump cython to 3.0.0 #2395

Reference:
pygame/pygame#2916
#1862

@yunline yunline force-pushed the ignore-cython-cfile branch from 063497c to 0fb0612 Compare February 16, 2023 14:22
@yunline yunline changed the title Try Generate Cython code at build time again Generate Cython code at build time Feb 17, 2023
@yunline yunline marked this pull request as ready for review February 17, 2023 16:33
@yunline yunline requested a review from a team as a code owner February 17, 2023 16:33
@MightyJosip MightyJosip added the build Compiling stuff label Feb 18, 2023
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.

I'm generally approving of this as a stop gap before steadily turning cython files to C files.

@pmp-p
Copy link
Member

pmp-p commented Feb 22, 2023

Though the wasm CI is already doing that, this is probably a bad idea for testing against python git where cython files may need to be edited.

This is what i do locally after 3.11 and 3.12(git) are built:

  • i build with python3.11 : cython files + pygame
  • Then if i know cython files will fail on 3.12, i patch the 3.11 generated one
  • then continue build without running cython_only.

@yunline
Copy link
Contributor Author

yunline commented Feb 23, 2023

Though the wasm CI is already doing that, this is probably a bad idea for testing against python git where cython files may need to be edited.

Maybe we can solve this problem by adding a 'non-cython' option.

@MyreMylar
Copy link
Member

slightly longer term (but getting closer) the plan is to clear out all the cython code entirely.

Is this PR useful enough to current cython development to get over the downsides? Or should we close it. I tend to avoid cython entirely.

Comment on lines +285 to +286
if not no_compilation: # Compile cython before compilation
compile_cython()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are those lines not in the build command? Or are these files needed elsewhere too?

class CythonOnlyCommand(Command):

user_options = []
def initialize_options(self):pass
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a description:
description = "short doc what this command does"

can be seen with: python setup.py --help-commands

there is also python setup.py cython_only --help
not sure what that would produce at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
It seems other commands don't have descriptions too. Maybe we need another pr to fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, see #2360

@yunline yunline marked this pull request as draft August 14, 2023 00:36
@MyreMylar MyreMylar mentioned this pull request Aug 15, 2023
@ankith26
Copy link
Member

ankith26 commented Jul 8, 2024

This PR has been superseded by #2831 because we recently introduced the new buildconfig, so I'm closing this

But thanks for starting the work on this!

@ankith26 ankith26 closed this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Compiling stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants