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

pygame recipe #2019

Closed
wants to merge 22 commits into from
Closed

pygame recipe #2019

wants to merge 22 commits into from

Conversation

robertpfeiffer
Copy link
Contributor

As discussed on the Discord, this is a recipe to build apps based on SDL2-based pygame.

It currently references https://github.com/pygame/pygame/tree/android, a branch that will continue to live until we have an official stable release of pygame based on SDL2 with android support. Is that OK? Should recipes only refer to tarballs/tagged snapshots/releases, or is it allowed to have a moving target in there?

Simple examples have been tested by other pygame users (it doesn't just build on my own machine) but some pygame functionality is still untested, and some dependencies like freetype, postmidi and libjpeg are currently not part of the build. It's usable, but not complete.

Your feedback is appreciated.

@inclement
Copy link
Member

Nice work, I'm pleased to see the recipe is pretty simple in the end!

It currently references https://github.com/pygame/pygame/tree/android, a branch that will continue to live until we have an official stable release of pygame based on SDL2 with android support. Is that OK? Should recipes only refer to tarballs/tagged snapshots/releases, or is it allowed to have a moving target in there?

In general we'd like to tag specific versions, but for something like this it's reasonable to have a moving target. I think the obvious options are:

  • leave this as a PR until you feel some stable target is ready
  • merge with a recipe name like "pygame2beta" that will go away once a stable target is available
  • merge with the name "pygame2" and just assume nobody will use it much until it's stable

I don't really mind which we do. I wouldn't normally like the last one, but good pygame support is a big target so it's reasonable to compromise for ease of use.

@robertpfeiffer robertpfeiffer marked this pull request as ready for review April 13, 2020 15:51
Copy link
Member

@inclement inclement left a comment

Choose a reason for hiding this comment

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

Looks like there are some merge conflicts, but since the assets stuff is being covered by the other PR I hope it should be straightforward to resolve.

The recipe looks good to me, happy to merge as soon as there are no conflicts (and probably let's merge the assets dir PR first to keep things simple).

One minor thing: could you add the pygame recipe to one of our CI tests? Seems worth testing that we don't break the build.


def get_recipe_env(self, arch):
env = super(Pygame2Recipe, self).get_recipe_env(arch)
if 'sdl2' in self.ctx.recipe_build_order:
Copy link
Member

Choose a reason for hiding this comment

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

Won't this always be True, or in any case you always want to set the cross compile and android env vars?

url = 'https://github.com/pygame/pygame/archive/android-2.0.0-dev7.tar.gz'

site_packages_name = 'pygame'
name = 'pygame2'
Copy link
Member

Choose a reason for hiding this comment

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

Are you going to officially make the new release pygame2, with e.g. a different name in pip? If so then fine, but if not it's probably more obvious to just call the recipe "pygame". Happy to go with your preference though.

install_in_hostpython = False

def prebuild_arch(self, arch):
super(Pygame2Recipe, self).prebuild_arch(arch)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
super(Pygame2Recipe, self).prebuild_arch(arch)
super().prebuild_arch(arch)

@AndreMiras
Copy link
Member

Sorry for missing that one. This is also looking good to me. I think we could make it to the tree pretty easily by:

  1. rebasing fixing merge conflicts
  2. remove the parts already addressed in Adding more assets #2132 even though it's not yet merged they don't strongly depend on one another
  3. addressing the couple of minor comments inclement and I added on the pygame recipe itself

If you're busy I'd be happy to help out

@inclement
Copy link
Member

Looks good to me. The one failure looks like maybe a network issue, @AndreMiras do you know about it?

@AndreMiras
Copy link
Member

Yes it's seems to be a network hiccups on the macOS build. I think we're covered by the other builds. I'll restart the failing build now.
I haven't verify if the changes aren't conflicting with the recently merged #2132
Best to make sure would be a rebasing.
Things look good for a merge otherwise

@inclement
Copy link
Member

Looks like the test failure is due to a test target not existing. @AndreMiras is this due to a recent change? If it's something simple I'm can push the change and merge.

@AndreMiras
Copy link
Member

AndreMiras commented May 1, 2020

Yes it's due to the recent changes. Basically it's still running on an old .travis.yml, but somehow it picked up the new Makefile. It's like if half of upstream/develop (specially #2159) was merged back into the branch.
If it was rebased, things like #2132 would not appear in the diff of this PR like we currently see. I would be happy to spend 10 minutes with @robertpfeiffer to walk him through it, but I didn't see him on Discord.
Anyway I think this is safe to (squash &) merge

Edit: ideally this is what I imagine the PR could look like #2164 I've started from your branch, squashed the commits, made a comprehensive commit message and rebased to upstream develop. I've also edited the version to be configurable. The only thing that always annoys me is the coauthoring that I don't find how to remove

class Pygame2Recipe(CompiledComponentsPythonRecipe):

version = '2.0.0-dev7'
url = 'https://github.com/pygame/pygame/archive/android-2.0.0-dev7.tar.gz'
Copy link
Member

Choose a reason for hiding this comment

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

Usually the version should be configurable

Suggested change
url = 'https://github.com/pygame/pygame/archive/android-2.0.0-dev7.tar.gz'
url = 'https://github.com/pygame/pygame/archive/android-{version}.tar.gz'

open("Setup", "w").write(setup_file)

def get_recipe_env(self, arch):
env = super(Pygame2Recipe, self).get_recipe_env(arch)
Copy link
Member

Choose a reason for hiding this comment

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

Nitpicking

Suggested change
env = super(Pygame2Recipe, self).get_recipe_env(arch)
env = super().get_recipe_env(arch)

@AndreMiras
Copy link
Member

Merged via #2164 thanks again

@AndreMiras AndreMiras closed this May 1, 2020
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.

3 participants