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

Feature/enable appveyor #75

Merged
merged 12 commits into from
Feb 23, 2017
Merged

Feature/enable appveyor #75

merged 12 commits into from
Feb 23, 2017

Conversation

drnlm
Copy link
Member

@drnlm drnlm commented Feb 19, 2017

This adds appveyor support for testing pygame_cffi on windows.

A number of tests faill, at least partly because RWops isn't correct on windows platforms, but at least the tests run.

Copy link
Member

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Looks good. Left some minor comments.

- "python -m test"

#artifacts:
# - path: dist\*
Copy link
Member

Choose a reason for hiding this comment

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

Should this be uncommented? Or removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

My plan is to have appveyor build wheels for us. It already runs bdist_wheel, but I still need to work out how to include the required dlls. Once that's in place, I'll uncomment this so we can grab the produced wheels.

Copy link
Member

Choose a reason for hiding this comment

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

Cool.

@@ -0,0 +1,3 @@
Appveyor is a service we use to build windows binaries for pygame_cffi.

We borrow varioys helpers, like the install.psi script, from the pygame project.
Copy link
Member

Choose a reason for hiding this comment

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

s/psi/ps1/?

$destpath = $basedir + "pygame"

Write-Host "dlls from" $dlls
Write-Host "destiation" $destpath
Copy link
Member

Choose a reason for hiding this comment

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

s/destiation/destination/

return $filepath
}

# Download and retry up to 5 times in case of network transient errors.
Copy link
Member

Choose a reason for hiding this comment

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

s/5/3/?

Copy link
Member Author

Choose a reason for hiding this comment

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

We copy the install.ps1 script verbatim from pygame, comment errors and all.

I'd prefer to keep the two in sync, so it's trivial to spot when pygame updates something.

Copy link
Member

Choose a reason for hiding this comment

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

Cool.

else:
return ['prebuilt-x64/include', 'prebuilt-x64/include/SDL']
return ['/usr/include', '/usr/include/SDL', '/usr/local/include/SDL']

Copy link
Member

Choose a reason for hiding this comment

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

I think we might be missing a newline between these two functions. Should we perhaps add docstrings to these?

Copy link
Member

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Found one small typo, otherwise looks good.

README.md Outdated

## Building on windows

To build on windows, you need to download the appropriate dependancy libraries.
Copy link
Member

Choose a reason for hiding this comment

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

s/dependancy/dependency/

- "python -m test"

#artifacts:
# - path: dist\*
Copy link
Member

Choose a reason for hiding this comment

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

Cool.

@drnlm drnlm merged commit 7245758 into master Feb 23, 2017
@drnlm drnlm deleted the feature/enable_appveyor branch February 23, 2017 17:30
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.

2 participants