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

WIP: Port bash script naveen521kk to .travis.yml #1

Closed

Conversation

stuaxo
Copy link
Collaborator

@stuaxo stuaxo commented Aug 19, 2020

Port WHL build script @naveen521kk to plain .travis.yml

  • Delete build script and move to .travis.yml

Minor changes:

  • Introduce "ARCH" variable for windows, x86 or x64
  • Rename PYVER to PYTHON_VERSION to be consistent with CAIRO_VERSION
  • 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!)
  • Added the newer tests from newer pycairo

TODO
Having added the newer tests, test_typing fails.

@naveen521kk
Copy link
Owner

Question: Any necessity for it to be merged in master? Because I used the branch windows-wheels for the other PR. Also, I allowed maintainer to commit directly in that branch. SO you could directly commit if needed. Or if you want me to review the code make a PR to windows-wheels branch.
Cheers

@stuaxo stuaxo changed the base branch from master to windows-wheels August 19, 2020 14:00
@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 19, 2020

Question: Any necessity for it to be merged in master? Because I used the branch windows-wheels for the other PR. Also, I allowed maintainer to commit directly in that branch. SO you could directly commit if needed. Or if you want me to review the code make a PR to windows-wheels branch.
Cheers

Nope, this was definitely my error ! In a bit of a rush now :)
It would be great if you can sanity check this... I think it works, but I haven't tested the whls for a couple of days (when I was using a modified version of your script).

@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 19, 2020

I've got to be AFK for a while so can't wait to see tests pass, it's possible I've done something silly like bodged the merge with the latest version of your work.

@naveen521kk
Copy link
Owner

Currently I am busy I will look into this maybe by tomorrow.

Copy link
Owner

@naveen521kk naveen521kk left a comment

Choose a reason for hiding this comment

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

This was some quick questions I came up. Also, can we create a GitHub Action (As it is the only one I know has scheduled build) which checks for upstream for cairo version and other things? A python script would do. Just execute the py script and then create an issue if it not update. Any ideas about this?

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated
name: Build on Python 3.8.3 in Windows
env: PYVER="3.8.3" CAIRO_VERSION=1.17.2
name: Build on x64 Python 3.8.3 in Windows
env: ARCH=x64 CAIRO_VERSION=1.17.2 PYTHON_PACKAGE=python PYTHON_VERSION="3.8.3"
Copy link
Owner

Choose a reason for hiding this comment

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

This can be updated to 3.8.5 as it is released now.

Suggested change
env: ARCH=x64 CAIRO_VERSION=1.17.2 PYTHON_PACKAGE=python PYTHON_VERSION="3.8.3"
env: ARCH=x64 CAIRO_VERSION=1.17.2 PYTHON_PACKAGE=python PYTHON_VERSION="3.8.5"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good spot...

We can automate this, I spoke to dependencies.io who gave me an account for open source stuff + that can generate diffs and PRs for this sort of thing.

It's annoying nuget doesn't accept a query like 3.7.x and give the highest match, I wonder if chocolatey can do it?

Copy link
Owner

Choose a reason for hiding this comment

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

You can use a query like 3.7 I think.

Copy link
Owner

Choose a reason for hiding this comment

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

Also I just found that choco and nuget are one and the same thing in structure. Choco uses nugets structure.

Copy link
Collaborator Author

@stuaxo stuaxo Aug 20, 2020

Choose a reason for hiding this comment

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

Chocolatey has some extensions, I'm inclined to switch back to it, as it has some advantages.
Native support in Travis: one less thing to install, and Travis' cache support seems to include it.

You can force it to install for 32 or 64 bit as needed.

Semver: not supported now, but they have a ticket.
chocolatey/choco#1610

In the meantime I think we might be able to do semver support with a combination of powershell and the output of choco search or hit their API directly with cURL.

This would be good as we could specify the python version with 2 digit versions, not 3, so one less thing to support and patch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can pass - -x86 to install 32 bit versions, I haven't tried it yet.

Copy link
Owner

@naveen521kk naveen521kk Aug 21, 2020

Choose a reason for hiding this comment

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

That doesn't help much. Nuget python is build for CI ,but choco is using default py installer which may be troublesome. So better to use nuget for x86.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough, will stay on nuget, if there are questions around chocolatey.

Do you know what the issues with the default installer are?

I haven't used windows as my default OS for a while, well aware windows + python + C extensions can get pretty painful.

Copy link
Owner

Choose a reason for hiding this comment

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

I kinda use windows as my default one. Maybe because I don't know C or something. I just know quite a few basic of python and got to know about this because of manim. Some video tool which use pycairo as it's base. I don't even have C compilers in my PC and that made me do this. But yeah Nuget x86 py build should be used or we should use something kinada docker or something but not required.

Copy link
Owner

Choose a reason for hiding this comment

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

This can be easily fixed if we were to use GitHub Actions.

.travis.yml Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@naveen521kk
Copy link
Owner

naveen521kk commented Aug 19, 2020

Also, can we create a GitHub Action (As it is the only one I know has scheduled build) which checks for upstream for cairo version and other things? A python script would do. Just execute the py script and then create an issue if it not update. Any ideas about this?

What do you think about this? @stuaxo

@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 19, 2020

I can def do the github action to generate patch PRs, I have one I need to push upstream to cairo-windows that already does this, so fairly straightforward.

I'm a bit new to github actions so wasn't sure the best events for it to run on, so am thinking scheduled daily, and also available from a webhook (I imagine only @lazka will have the token for that).

As soon as someone merges the PR then I guess that will trigger a build and release (on github at least).

Copy link
Owner

@naveen521kk naveen521kk left a comment

Choose a reason for hiding this comment

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

Some suggetsions

.travis.yml Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@naveen521kk
Copy link
Owner

naveen521kk commented Aug 20, 2020

About test failing I remember I read somewhere where lazka said about tee surface not implemented in windows or something like that. So its good to ping(@lazka) to help us on this or can we exempt these test on windows. @stuaxo

We can skip that test for windows see https://docs.pytest.org/en/latest/skipping.html#id1

stuaxo and others added 2 commits August 20, 2020 21:52
Fix adding python to PATH

Co-authored-by: Naveen M K <naveen@syrusdark.website>
Install wheel for all OSs, - via @naveen521kk

Co-authored-by: Naveen M K <naveen@syrusdark.website>
@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 20, 2020

Also, can we create a GitHub Action (As it is the only one I know has scheduled build) which checks for upstream for cairo version and other things? A python script would do. Just execute the py script and then create an issue if it not update. Any ideas about this?

What do you think about this? @stuaxo

Definitely doable, I think I wrote somewhere else we can do with dependencies.io and their deps tool.

I wonder which things we want to check on... if we can get rid of the need for an exact python build. then that leaves cairo-windows, at least until we make whls for other platforms.

@naveen521kk
Copy link
Owner

How about py 3.9 binary?

@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 21, 2020

We're OK until October the 5th for 3.9
https://www.python.org/dev/peps/pep-0596/#schedule

@naveen521kk
Copy link
Owner

I will need to check and work something and will do in two days and later it can be merged.

@naveen521kk
Copy link
Owner

Now tests should be passing.

@naveen521kk
Copy link
Owner

@stuaxo We messed up something with x86 builds.

@naveen521kk
Copy link
Owner

@stuaxo If the tests pass this is ready to be merged.

Copy link
Collaborator Author

@stuaxo stuaxo left a comment

Choose a reason for hiding this comment

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

I think we can change the test to check for minimum version to also check for Windows...

Do we know everything works OK with TeeSurface not enabled?

We might be able to just enable TeeSurface upstream, I don't have the dev environment setup to try this right now, but if it works we could always patch upstream and this will be fixed for good (it seems like msys has this enabled).

@naveen521kk
Copy link
Owner

naveen521kk commented Aug 23, 2020

it seems like msys has this enabled

I noticed about that. Maybe let's use that? Include the cairo.dll it generates to the .whl files. That would do. Also, I have tested this with the site which provided wheel that TeeSurface doesn't work on it also. Also, maybe it would be better to chat on gitter that these comments.

@naveen521kk
Copy link
Owner

Do we know everything works OK with TeeSurface not enabled?

I am such a noob here. I don't know C lol. So, I have no much experience in pycairo also.

@naveen521kk
Copy link
Owner

naveen521kk commented Aug 24, 2020

I got some weird ideas. @stuaxo

  • Why not move from Travis to GitHub Actions because Travis kinda sucks me ( I have more experience with Actions).
  • Not to depend on preshing repo instead use msys for our needs( There a gh-actions for that which makes it simple than Travis).

@naveen521kk
Copy link
Owner

Looks like I won't be able to spend much time on this in the upcoming week and I plan to take a long break. So I would be working on this PR next week.
Just tell me what to do it would be good. If I couldn't complete it then you(or anyone else) should be taking care of this 🙂. Also, I am adding you as collaborators to this fork. Please don't delete any release as some project depend on them.
Yes, I am running out of time and this would be the last week. Next, I won't be available.
CC
@stuaxo

@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 24, 2020

I've got a similar time problem.. Will reply to this shortly though :)

@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 24, 2020

I got some weird ideas. @stuaxo

* Why not move from Travis to GitHub Actions because Travis kinda sucks me ( I have more experience with Actions).

Yeah, definitely if @lazka is up for it.

* Not to depend on preshing repo instead use msys for our needs( There a gh-actions for that which makes it simple than Travis).

This makes sense, but I reckon we can probably be able to land this first, as it does seem to work.

We should verify there are no new issues using this when pygobject is already installed, then it should be good to go.

I'm also going to have less time, but should be able to help land the WHL support at least.

@naveen521kk
Copy link
Owner

Question: Does the whl available on https://www.lfd.uci.edu/~gohlke/pythonlibs/#pycairo enables tee surface? We need to look into it.

@naveen521kk
Copy link
Owner

Does the whl available on https://www.lfd.uci.edu/~gohlke/pythonlibs/#pycairo enables tee surface?

Yes it works there.

@naveen521kk
Copy link
Owner

@stuaxo If you could https://github.com/naveen521kk/pycairo/runs/1035437105?check_suite_focus=true#step:6:165 help me fix this I would be very happy and also it can be used by preshing/cairo-windows and use automatic releasing using Github actions. After that, maybe lets depend on then.

@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 27, 2020

I'll have a look, I'm back @ work at the moment so won't be until later.

I've been looking at getting TeeSurface enabled on cairo-windows, which would mean we don't nee to change any tests
preshing/cairo-windows#7

@preshing has suggested a solution to get it working, but I won't have time until @ least later on,
If you have a VS2017 build environment it might be worth trying.

I started a PR there to generate make PRs when there are new releases of cairo, freetype etc update using dependencies.io
preshing/cairo-windows#10
I probably need to add some instructions on what setup is needed so it's easier for @preshing to land it.

@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 27, 2020

Quick feedback: I can see it's building with VS2019, I don't really know the differences between 2017 + 2019, but @preshing told me that cairo-windows is currently built on 2017, not 19, so there may be some compatibility issue.

@naveen521kk
Copy link
Owner

naveen521kk commented Aug 30, 2020

Yes @stuaxo does https://github.com/naveen521kk/pycairo/runs/1046803166?check_suite_focus=true work? I am asking on tee surface did that work? If you any test environment download the wheels from artifact's and test it once locally. It that works we can tell @preshing to release it so. And yes tee surface problem will be over.
Edit: I have done something stupid for x86 builds. So please ignore that one instead use the one from the latest commit.

@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 30, 2020

@naveen521kk I'm away from my computer, any idea why the x64 bit build fails?

All I tried was telling Travis to rebuild from my phone, think I need to verify from windows VM when in front of computer later.

Copy link
Owner

@naveen521kk naveen521kk left a comment

Choose a reason for hiding this comment

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

Here is what happens.

.travis.yml Outdated
- if [ "$TRAVIS_OS_NAME" == "windows" ]; then $TMP/nuget install python -Version $PYTHON_VERSION -OutputDirectory $TMP/nuget-$ARCH; fi
- if [ "$TRAVIS_OS_NAME" == "windows" ]; then export INCLUDE="$TMP/cairo-windows-$CAIRO_VERSION/include"; fi
- if [ "$TRAVIS_OS_NAME" == "windows" ]; then export LIB=$TMP/cairo-windows-$CAIRO_VERSION/lib/$ARCH; fi
- if [ "$TRAVIS_OS_NAME" == "windows" ]; then export PATH=$TMP/nuget-$ARCH/python.$PYTHON_VERSION/tools/:$TMP/nuget-$ARCH/python.$PYTHON_VERSION/tools/Scripts:$PATH; fi
Copy link
Owner

Choose a reason for hiding this comment

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

I found why it fails. Because the nuget package pythonx86 contains x86 build of python and it does pull x64 python and then later tries to build for x86 which makes is fail. :( Check my shell script.

Update cairo-windows version to "with-tee" tag.
Revert cairo-windows version, need to do this in front of computer with proper time :)
Broke version numbers as trying to type with 2 mins to spare :)
Fix cairo-windows URL
@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 30, 2020

I need to stop trying to edit this at the same time as trying to get a 3 year old ready to get out of the house :) I'll come back to this when I can give it my full attention and stop adding duff commits :)

Don't worry, I will squash the commits before we push anything to the pycairo proper :)

@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 30, 2020

I need to stop trying to edit this at the same time as trying to get a 3 year old ready to get out of the house :) I'll come back to this when I can give it my full attention and stop adding duff commits :)

Don't worry, I will squash the commits before we push anything to the pycairo proper :)

There's a pretty good chance it might be the when the work week starts (Tuesday it's a long weekend) when I can look at stuff properly, as it's almost all small person time, then just tiny bits of adult time during the weekend (and some small bits of coding time)!

@naveen521kk
Copy link
Owner

Maybe I will work if I get some time.

Specified the variable `$PYTHON_PACKAGE` to install. 
cc. @stuaxo
@naveen521kk
Copy link
Owner

naveen521kk commented Aug 31, 2020

Any idea why https://travis-ci.com/github/naveen521kk/pycairo/jobs/379364608 fail? @stuaxo
Gonna rerun it.

@stuaxo
Copy link
Collaborator Author

stuaxo commented Aug 31, 2020

Squashed and merged to pygobject/pycairo.

@stuaxo stuaxo closed this Aug 31, 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.

2 participants