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

DEV: Build wheels using cibuildwheel. #8

Merged
merged 10 commits into from
Oct 12, 2021

Conversation

jvkersch
Copy link
Collaborator

Adds a command to the README to build vpsearch wheels.

Under normal circumstances cibuildwheel is most useful when it is run automatically from within Travis or GitHub actions, so that it can build wheels for all supported Python versions and platforms. If there is interest (and if GitHub actions can be enabled on the admin side) I can set that up as well. If not, building the wheels manually for Linux also works.

@rahulporuri rahulporuri self-requested a review August 12, 2021 08:48
@jvkersch
Copy link
Collaborator Author

Some background information, to facilitate reviewing:

  1. cibuildwheel abstracts away a lot of low-level operations that otherwise require some insight into build processes on different platforms. For example, auditwheel repair (a step run after the wheel has been built) will detect that the wheel depends on an external library (Parasail), will copy that library in, and fix up rpath to point to the patched location. I've verified this on Linux but I think this also works for macOS. With some elbow grease it may be adaptable to Windows as well (e.g. sklearn has a package to do this).
  2. I think the command quoted in the README only works on Linux, but I am not sure.

@jvkersch
Copy link
Collaborator Author

jvkersch commented Oct 6, 2021

@rahulporuri Did you have a chance to try this out? If not let's merge this as-is -- it's just an addition to the docs anyway.

@rahulporuri
Copy link

I'll definitely review over this weekend.

@jvkersch
Copy link
Collaborator Author

jvkersch commented Oct 8, 2021

Thanks @rahulporuri. I've added one further GitHub action to build wheels via GitHub actions, using cibuildwheel. This makes the job of building wheels for various Python versions/platforms really easy. The workflow currently has to be triggered manually (via the GitHub interface) and uploads an artifact with the various wheels upon successful completion.

image

@jvkersch
Copy link
Collaborator Author

I've updated the README to mention the build-wheels workflow.

Copy link

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

@jvkersch I have a bunch of comments. Apologies if I'm missing something trivial in the documentation.

ci/build-parasail.sh Show resolved Hide resolved
.github/workflows/build-wheels.yml Outdated Show resolved Hide resolved
.github/workflows/build-wheels.yml Outdated Show resolved Hide resolved
.github/workflows/build-wheels.yml Show resolved Hide resolved
.github/workflows/build-wheels.yml Show resolved Hide resolved
@rahulporuri
Copy link

Also, it looks like the job didn't run earlier - https://github.com/enthought/vpsearch/actions/runs/1320491203. Do you want to try running it again to see whether or not the Action works as expected?

@jvkersch
Copy link
Collaborator Author

@rahulporuri Thanks for the review! I've replied to your comments -- please resolve the questions that you think have been answered, and I'd be happy to provide more input for the others.

Regarding running the action, I think we may be again in the same situation where an action contributed from a fork does not get run automatically. I will reach out to you offline about this. In the meantime, I have a different clone of the repo that I set up to develop this workflow, and where you can see the workflow in action: https://github.com/jvkersch/vpsearch-throwaway/actions/runs/1319590181.

@rahulporuri
Copy link

In the meantime, I have a different clone of the repo that I set up to develop this workflow, and where you can see the workflow in action: https://github.com/jvkersch/vpsearch-throwaway/actions/runs/1319590181.

I downloaded the artifacts from that job and everything looks good.

Copy link

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM

@rahulporuri
Copy link

@jvkersch you might want @mdickinson to take a look - but it's quite possible that @mdickinson doesnt have the bandwidth at the moment to review this PR.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM; a couple of nitpick-level comments.

@jvkersch
Copy link
Collaborator Author

Thanks @mdickinson and @rahulporuri for the extensive comments! I think this is ready to go.

@jvkersch jvkersch merged commit f9fa792 into enthought:master Oct 12, 2021
@jvkersch jvkersch deleted the dev/build-wheels branch October 12, 2021 12:40
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