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

Add websockets to list of users. #27

Merged
merged 1 commit into from
Aug 21, 2017
Merged

Add websockets to list of users. #27

merged 1 commit into from
Aug 21, 2017

Conversation

aaugustin
Copy link
Contributor

No description provided.

@joerick
Copy link
Contributor

joerick commented Aug 21, 2017

Yay! Thanks @aaugustin! Any feedback on cibuildwheel, or the setup process?

@joerick joerick merged commit 9faa8f8 into pypa:master Aug 21, 2017
@aaugustin
Copy link
Contributor Author

Generally speaking it went pretty well.

It would be nice if the latest Python 3 was the default. I think people who care about building wheels for all platforms are more likely to run the latest Python 3 than Python 2.7.

AFAIK the pain points we encountered were #7 and #16. @mayeut took care of the whole setup. Perhaps he'll have additional insights.

You can take a look at the whole discussion here if you're curious: python-websockets/websockets#239

@mayeut
Copy link
Member

mayeut commented Aug 21, 2017

Hi @joerick , some additional comments.

Regarding #16, there were 2 issues:

  1. Being able to pass an environment variable when building to make the C extension build mandatory (optional in websockets).
    This was solved adding touch .cibuildwheel to the ci configuration (the idea came from Add CIBW_ENVIRONMENT option #16 (comment), thanks @joerick).
    Now that I think about this specific issue, it might be a good thing if cibuildwheel defined an environment variable e.g. CIBUILDWHEEL=1 for the commands it starts.
  2. Being able to pass environment variables when testing => tests are not run for now

Regarding #7, this would have allowed to run the tests ignoring the error code by adding || true.

Another thing is that the sample appveyor.yml (https://github.com/joerick/cibuildwheel-autopypi-example/blob/4656c3b0de4ee1f776252a62c9c1cd8bbd8c5dec/appveyor.yml) results in a failed build when uploading to PyPI: https://ci.appveyor.com/project/mayeut/websockets/build/3.3.25
I changed the appveyor.yml upload script. The console output is still "error like" but the build doesn't fail: https://ci.appveyor.com/project/mayeut/websockets/build/3.3.33

Regarding the Python 2/3 comment from @aaugustin, since Python default environment is setup by ci services, something could be thought like cibuildwheel respawning itself in the right Python environment depending on the CIBW_SKIPenvironment variable (i.e. run from minimum Python supported by the project being built). This might be a challenge on travis-ci linux (I don't remember if all Python versions are already installed in default image). Given how easy it is to change default Python environment for this specific CI environment, maybe a simple check could be added to report misconfiguration in this case.

Another issue is that travis-ci slave seems to have trouble to retrieve a docker image from time to time (c.f. https://travis-ci.org/aaugustin/websockets/jobs/266956026). It would be a good thing to have a retry strategy for this case. What would be even neater (even if it adds one line to *.yml files) is to have the build errored instead of failed in these cases. Something like cibuildwheel --setup-environment that would take care of retrieving docker images (Linux) / Python (macOS) in the install step.

I can open separate issues for that long wishlist or I can let you open only what you deem necessary.
I might even be able to lend a hand to resolve some of them. Just let me know.

Anyway, thanks for that tool !

@mayeut
Copy link
Member

mayeut commented Aug 21, 2017

@joerick, I forgot to ask, and this is really minor since python 3.3 is EOL next month but why aren't wheels built for macOS with Python 3.3 ?

@joerick
Copy link
Contributor

joerick commented Aug 29, 2017

Super, thanks so much for the feedback @mayeut and @aaugustin - it's reassuring that we're going in the right direction since most of the pain points will be solved by future features :)

it might be a good thing if cibuildwheel defined an environment variable e.g. CIBUILDWHEEL=1 for the commands it starts

Great idea!

It would be nice if the latest Python 3 was the default.

Also a good idea - I've just got to look at what's installed on the travis and appveyor machines by default.

cibuildwheel --setup-environment that would take care of retrieving docker images (Linux) / Python (macOS) in the install step.

I'm hesitant to add more lines to the travis.yml or appveyor.yml, but this is a good idea and I might consider it if we see a lot of these kinds of failures 👍

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