search

RalfG / python-wheels-manylinux-build

GitHub Action to build Python manylinux wheels
https://github.com/marketplace/actions/python-wheels-manylinux-build
Apache License 2.0
92 stars 44 forks source link

Add escape hatch to run arbitrary setup script #12

Closed exarkun closed 4 years ago

exarkun commented 4 years ago

Fixes #11

exarkun commented 4 years ago

I think pre-build-script would be a better name for the option, as it requires a full command, not just a list of requirements, such as build-requirements and system-packages. Can you implement the suggested changes, and additonally update README.md and full_workflow_example.yml accordingly? Thanks!

Thanks. I think that is a better name, indeed.

Since I submitted the PR I went ahead and did some more testing of the changes for my project and I found I needed to make one further change - executing the pre-build script was not sufficient because I also needed to get a new directory into PATH for the build step (my use-case is installing the Rust cargo tool).

I fixed this by making the pre-build script get sourced instead of executed so it can export environment changes to entrypoint.sh. I'm not sure how sound this approach is, though. I wonder if you have any thoughts on this requirement?

RalfG commented 4 years ago

Exporting would be yet another option then, next to executing... But I noticed that there is no execution command (i.e. no sh) in front of the "$PRE_BUILD_SCRIPT" variable:

if [ ! -z "$PRE_BUILD_SCRIPT" ]; then
    "$PRE_BUILD_SCRIPT" || { echo "Pre-build script failed."; exit 1; }
fi

So instead of positioning this option as the path to a script, this could just be a custom command. In your case pre-build-script: "source <path_to_script.sh>" should work, right? For others this could be sh <path_to_script.sh>, or even a simple command, e.g. modifying an environment variable: "CFLAGS=${CFLAGS//-pipe/}".