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

Build Windows wheels #191

Closed
wants to merge 8 commits into from
Closed

Conversation

naveen521kk
Copy link
Contributor

Fixes #19
This makes Travis build wheels and upload it to a draft Github Release. I tried downloading one that was built into my computer and it ran without any error for the example in ReadMe. This would for the installation of Pycairo for windows users who don't have C compilers.

@Daltz333
Copy link

+1 to this.

@EriKWDev
Copy link

+1!

@cqann
Copy link

cqann commented Jul 22, 2020

+1 👍

@stuaxo
Copy link
Collaborator

stuaxo commented Jul 22, 2020

Thanks, for taking the time to put this together, having WHLs is a major help for Windows users.

@naveen521kk
Copy link
Contributor Author

Any idea when will this be merged? Or maybe I should do any improvement or something?

@stuaxo
Copy link
Collaborator

stuaxo commented Jul 26, 2020

I have a chunk of time tomorrow I can give this a try and give any feedback.

What's the best way to grab the WHLs, from Travis somehow?

@Daltz333
Copy link

Ideally wouldn't you publish them to pypi? I assume Travis also has an artifact feature for publishing temporarily.

@naveen521kk
Copy link
Contributor Author

naveen521kk commented Jul 26, 2020

For now, I have implemented it to put the whl's to a Draft Github Release. I think that is enough. Maybe if required I will implement it to upload it to PyPI directly. @stuaxo

@EriKWDev
Copy link

Having the wheels published to PyPi would solve all of my problems. I would then be able to have pycairo as a dependency without having to worry about my users having a C++ builder installed on their windows systems.... Currently, I have to resolve to the unofficial wheels on https://www.lfd.uci.edu/~gohlke/pythonlibs/#pycairo

@stuaxo
Copy link
Collaborator

stuaxo commented Jul 26, 2020

Don't worry about publishing directly, I just want to have a quick poke at it... I've been a Cairo user on and off on Windows and Linux for a while so would like to give it a go. :)

@naveen521kk
Copy link
Contributor Author

If you want to test it out you can visit this and download one for your system. I generated using this in my Fork.:)

setup.py Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,4 @@

curl -L https://github.com/preshing/cairo-windows/releases/download/$CAIRO_VERSION/cairo-windows-$CAIRO_VERSION.zip -o cairocomplied.zip
Copy link
Member

Choose a reason for hiding this comment

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

This is an outdated cairo release :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing we can do now is to compile Cairo if we need the latest one. And btw the install script in cairo-windows is actually batch which Travis doesn't support :(

Copy link
Member

Choose a reason for hiding this comment

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

yeah. It's probably best to wait until https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/37 is merged and then build it ourselves using meson. But, yeah, let's not block this..

Copy link

Choose a reason for hiding this comment

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

yeah. It's probably best to wait until https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/37 is merged and then build it ourselves using meson. But, yeah, let's not block this..

FYI: It's been merged.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the idea that we would use meson to do the builds here, or is there some buildfarm attached to freedesk that we get the builds from ?

OT (ish)
About a week ago, I had a chat with dependencies.io and they came up with a patch that uses their bot to generate PRs to update cairo + freedesktop, for preshing, cairo when upstream changes. This isn't so relevant if we don't use preshing cairo, but I guess we can still setup something to generate PRs here when upstream updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SO what should be done?

@naveen521kk
Copy link
Contributor Author

Anything else should be done @lazka ?

@stuaxo
Copy link
Collaborator

stuaxo commented Jul 27, 2020

I seem to recall, the older windows pycairo builds included cairo.dll named as _cairo.dll

Should this be the case for this WHL too?

@naveen521kk
Copy link
Contributor Author

@stuaxo I tried renaming it so, but it failed, see screenshot below.
image
But this doesn't happen when it is the original state.
image
So, it should be cairo.dll and not _cairo.dll.
PS. I used the WHL from here which I generated using this script. :)

.travis.yml Outdated Show resolved Hide resolved
@naveen521kk
Copy link
Contributor Author

Anything else should be done @lazka ?

@stuaxo
Copy link
Collaborator

stuaxo commented Jul 28, 2020

Hi
The upstream windows-cairo updated to CAIRO_VERSION to 1.17.2
preshing/cairo-windows#3

S

@naveen521kk
Copy link
Contributor Author

Ok, then let me update. :)

@stuaxo
Copy link
Collaborator

stuaxo commented Jul 28, 2020

Cheers, I should have linked the ticket before, but I had no idea @preshing would be so responsive :)

@naveen521kk
Copy link
Contributor Author

naveen521kk commented Aug 1, 2020

I am curious does this work the same on other operating systems?

I tried the same in Google Colab(I don't have a Linux). But found it weird installing the wheel which generated using this script had no errors which running the examples. Idk whether this works on all systems.

@naveen521kk
Copy link
Contributor Author

@stuaxo Done it. Any other thing to be done?

@naveen521kk
Copy link
Contributor Author

naveen521kk commented Aug 17, 2020

Opps. I'm sorry if I'm late on this.
The maintainer, should set up a secret variable called GITHUBOAUTHTOKEN in Travis settings , a personal access token, with permission read:packages, repo, write:packages for GitHub release it is making.

@stuaxo
Copy link
Collaborator

stuaxo commented Aug 18, 2020

Hi @naveen521kk
I'm putting patch to simplify your PR and move everything from your bash script into travis.yml and was wondering, is it intentional that the PR to add windows wheels removes this line -

- if [[ "$TRAVIS_OS_NAME" != "osx" ]] && [[ "$TRAVIS_PYTHON_VERSION" != "3.8" ]] && [[ "${TRAVIS_PYTHON_VERSION:0:4}" != "pypy" ]]; then python -m pip install --upgrade pygame; fi

?

EDIT: Is the reason that the PR is based on an older version of .travis.yml - the scripts: section looks a little different too ?

@naveen521kk
Copy link
Contributor Author

Is the reason that the PR is based on an older version of .travis.yml

Yup that's why I left it so.

@naveen521kk
Copy link
Contributor Author

I'm putting patch to simplify your PR and move everything from your bash script into travis.yml

It's neat to have a batch script for it instead of having all in Travis.yml and would also be easy to maintain. :)

@stuaxo
Copy link
Collaborator

stuaxo commented Aug 19, 2020

It's neat to have a batch script for it instead of having all in Travis.yml and would also be easy to maintain. :)
This may be handy, I looked at various travis runners, in the end I paste everything in 'install' and 'script' to a shell script, remove the - and use it.
Adding something like this can go in a different PR.

Ideally we should make each PR should add just one thing, with minimal changes (and of course passing tests).
In that vein there may be a few PRs in this, we can split out - the obvious two are building windows WHLs and deployment.

Windows WHLs:

This is good work you've done getting all this all up and running, in some ways I made it a little dumber moving it to .travis.yml, I hope you don't mind.

I made a PR to port your script to plain .travis.yml naveen521kk#1 which has a few advantages -

  • We no longer duplicate the content of travis.yml in runPycairo.sh and have removed all the != 'windows' lines in the script section.
  • Travis is now in charge of building everything (runPycairo.sh had to build twice - 32 and 64 bit)

Everything else is the same as the script, just moved into travis.yml some minor changes:

  • Added the newer tests from more pycairo
  • Downloads are grouped with downloads from other platforms (in theory we could fail early here if they fail)
  • Files are downloaded to another folder (coverage was trying to check the python installation!)

TODO

  • Adding the new tests, came with a drawback though ... test_typing fails ! - We need to work out why that is and if needs fixing.
  • I don't have super amounts of free time, so this needs some testing before applying.

Deployment

  • This needs to be a different PR, we can see all tests passing before setting this up.

Meson

  • You started looking at building with meson, this is great + can go in a different PR

@naveen521kk
Copy link
Contributor Author

You started looking at building with meson, this is great + can go in a different PR

I really couldn't go much about it. Any help is appreciated.

@stuaxo
Copy link
Collaborator

stuaxo commented Aug 19, 2020

I don't think that's a big deal if we can't get meson working, for this.

@naveen521kk
Copy link
Contributor Author

Adding the new tests, came with a drawback though ... test_typing fails ! - We need to work out why that is and if needs fixing.

Yes, now the upstream repo has a release with tee surface. And all test pass.

Also, I prefer GitHub Action over Travis(because powershell is lot familiar to me than bash). If you allow me @lazka can I use it.

@stuaxo
Copy link
Collaborator

stuaxo commented Aug 30, 2020

Thanks :)

This can be two parts.

I'll check the WHL stuff later when in from of a computer + hopefully squash and merge to here.

I'm the meantime feel free to open a seperate ticket to discuss github actions.

@stuaxo
Copy link
Collaborator

stuaxo commented Aug 31, 2020

Since all tests are passing, I squashed and merged this, from the PR where with the bits and pieces we worked on to get it all into .travis.yml.

Thanks for all the work to get this into shape.

@stuaxo stuaxo closed this Aug 31, 2020
@naveen521kk
Copy link
Contributor Author

So @stuaxo when will we get wheels in pip?

@naveen521kk naveen521kk deleted the windows-wheels branch August 31, 2020 11:28
@stuaxo
Copy link
Collaborator

stuaxo commented Aug 31, 2020

I don't have access to that, we will see if @lazka appears :)

If I want to try this on my own branch should I just creating a release ?

I tried this, creating a new tag, but it only has the source files in it, not the WHLs - is my setup missing something?

@naveen521kk
Copy link
Contributor Author

naveen521kk commented Aug 31, 2020

Maybe I will contact @lazka on gitter and see. Also, I can see you as a maintainer in PyPI. Looks like release should have appeared as a draft for @lazka maybe. And he should have configured the tokens.

@stuaxo
Copy link
Collaborator

stuaxo commented Aug 31, 2020

I either missed being a maintainer earlier or it changed.
I'm starting to think 'attention to detail' may not be accurate when I wrote it on my C.v. :D

Even if I could release I would still double check. This is either my first commit on pycairo or first in years, so I did double check it was all good first.

If I can generate WHLs that I can download + then try them in a Windows VM then I'm happy to ask if we can pull the trigger, OTOH sounds like you already are.

I'm probably over cautious as last place I worked was a huge corporation where we tried double hard not to break stuff, so hope I'm not slowing stuff down too much :)

I'm definitely interested in the direction it goes, github actions look promising for instance.

@naveen521kk
Copy link
Contributor Author

If I can generate WHLs that I can download + then try them in a Windows VM then I'm happy

@stuaxo You can download the artifact here and check yourself. I have checked for my part on x64 and x86 and certify it works( Based of example in readme and tee surface test, so don't blame me). If you want or @lazka I will make an PR of that script of GitHub Action I made.( Maybe tomorrow). Just saying I need it on PyPi as soon as possible.

@stuaxo
Copy link
Collaborator

stuaxo commented Sep 1, 2020

Is it OK to rebase (or just recreate this branch), based off the pygobject/pycairo ?

This make the histories consistent (imagine if I broke something when I squashed the previous commits) - it's also a good chance to squash or get rid of the commits like "another try"*.

*I write a lot of these in my own branches :)

@naveen521kk
Copy link
Contributor Author

I will do this in a new PR. You talking about actions right?

@stuaxo
Copy link
Collaborator

stuaxo commented Sep 1, 2020

Yep, what's currently the msys_actions branch.

@naveen521kk
Copy link
Contributor Author

I will make a new PR in some 10 min.

@lazka
Copy link
Member

lazka commented Oct 5, 2020

I've made a new release with those wheels now

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.

Provide Windows Wheels
7 participants