-
Notifications
You must be signed in to change notification settings - Fork 329
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 llvmdev and llvmlite build GHA workflows #1122
base: main
Are you sure you want to change the base?
Conversation
* add llvmdev gha workflow * update build recipe and script * add llvmlite build workflow * update artifact naming * update llvmlite build using conda recipe * update llvmlite build using conda recipe * update llvmlite build using conda recipe * update llvmlite build using conda recipe * update llvmlite build using conda recipe * add libllvm15 to recipe meta.yaml * remove libllvm15 to recipe meta.yaml * switch to using llvmdev from numba ci channel * update llvmlite build * update llvmlite build * update llvmlite build * update llvmlite build * update llvmlite build * update llvmlite build * build with setuptools * remove llvmdev * add llvmdev * add win builds * add win builds * update win builds * update win builds * Add macOS build workflow for multiple Python versions
cd buildscripts/manylinux | ||
./docker_run_x64.sh build_llvmdev.sh "${MINICONDA_FILE}" | ||
RECIPE_NAME=./conda-recipes/llvmdev_manylinux | ||
echo "OUTPUT=$(conda build --output $RECIPE_NAME)" >> $GITHUB_ENV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this serving a purpose? The OUTPUT
variable doesn't appear to be used in any subsequent step, and the command appears to be erroring:
conda: error: argument COMMAND: invalid choice: 'build' (choose from activate, clean, commands, compare, config, create, deactivate, env, export, info, init, install, list, notices, package, content-trust, doctor, repoquery, remove, uninstall, rename, run, search, update, upgrade)
(from https://github.com/swap357/llvmlite/actions/runs/12704394168/job/35413712445#step:3:27279)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, should’ve been removed since build is currently using script
echo "LLVM_CONFIG=C:/Miniconda3/envs/build_env/Library/bin/llvm-config" >> $GITHUB_ENV | ||
echo "LLVM_DIR=C:/Miniconda3/envs/build_env/Library/lib/cmake/llvm" >> $GITHUB_ENV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look like unix path separators rather than Windows... It seems to be working (I guess) but is it needed to be in this format? Or is it a slight inconsistency with other parts of this workflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these were set using bash, remains of experiments. probably should keep everything consistent and use cmd for windows, will change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I've had a look over the workflows and I have a few questions in addition to those on the diff:
- Is the intention to build manylinux wheels with this? If so, is building on Ubuntu 24.04 appropriate?
- The artifact retention of one day for llvmdev builds seems a bit short - I could imagine the artifact getting deleted before it can be used / inspected / finished with / whatever, for example if someone started the workflow on a Friday with the hope of continuing whatever task was in progress on the Monday. I imagine artifact storage limits are a consideration (though I'm not familiar with them, I don't know how easy it will be to hit them), but does it seem possible to bump the retention up to 7 days?
- Presently the llvmlite workflow uses an llvmdev package from the numba channel, rather than the llvmdev workflow. Is the eventual aim to be able to use an artifact from the llvmdev workflow to build llvmlite? (If so that would cut down a lot of hassle for future LLVM feature development in llvmlite - for example the present decoupling has been a huge drag on the compiler-rt work, which still hasn't got over the line yet)
- Will this be used to build artifacts for upload to PyPI eventually? If so, should sdists be added? Is the intention to add them in the future?
- The test env for llvmlite is the build env (
build_env
). Would it be better to use a fresh env for the test, so that there's less chance of something carrying over from the build env that would otherwise be missed in a fresh env? - In the past we discussed renaming the
manylinux
andllvmdev_manylinux
folders (I think with @sklam) because they're not just used to build manylinux wheels, it's confusing. I'm not suggesting we want to rename them for this PR (because it might make the review more complicated) but it looks like we ought to rationalise that name soon after this PR is merged.
|
@swap357 Thanks for the responses / updates - I'll take a look again on Monday. In the meantime, could I ask you to not force-push rewritten history please? It makes it very hard / impossible to track progress of the review and changes in the GitHub UI. |
accidentally pushed experimental commits onto main, when I wanted to checkin squashed commits for test env change, will be mindful onwards |
Would recommend to use feature branches: https://www.atlassian.com/git/tutorials/comparing-workflows/feature-branch-workflow |
This PR introduces a new GitHub Actions workflows (
llvmdev_build.yml
andllvmlite_build.yml
) to automate the building and testing of llvmdlv and llvmlite wheels across multiple platforms and Python versions.Key Features
tested workflows -
llvmlite : https://github.com/swap357/llvmlite/actions/runs/12722849384
llvmdev : https://github.com/swap357/llvmlite/actions/runs/12719219323
llvmdlv_manylinux : https://github.com/swap357/llvmlite/actions/runs/12722220311