Skip to content

ENH: Add build script update step to download script #233

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 30, 2022
Merged

ENH: Add build script update step to download script #233

merged 3 commits into from
Nov 30, 2022

Conversation

tbirdso
Copy link
Contributor

@tbirdso tbirdso commented Nov 29, 2022 •
edited
Loading

Several related updates:

These changes are tested at https://github.com/InsightSoftwareConsortium/ITKSplitComponents/actions/runs/3586129456

@tbirdso tbirdso requested a review from dzenanz November 29, 2022 16:42
@tbirdso tbirdso linked an issue Nov 29, 2022 that may be closed by this pull request
Add ITKPythonPackage update step in download scripts #232
Closed
dzenanz
dzenanz reviewed Nov 29, 2022
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Code looks good. I updated zarr CI to match. Windows packages fail immediately.

@tbirdso
Copy link
Contributor Author

tbirdso commented Nov 29, 2022

Code looks good. I updated zarr CI to match. Windows packages fail immediately.

Thanks @dzenanz . Note that Windows CI workflows don't seem to use the windows-download-cache-and-build-module-wheels.ps1 script right now, I am looking into whether there are blocking issues preventing its use. For now you can continue to pull Python using the script from the scikit-build/scikit-build-addons master branch, this will not change with these updates.

dzenanz/ITKIOOMEZarrNGFF@c727689#diff-8b27c545141e883b71167a60f61e5a67a8776c84033f7aaf516fd7cafd2b4e02L233

@thewtex thewtex mentioned this pull request Nov 29, 2022
Closed
@tbirdso
Copy link
Contributor Author

tbirdso commented Nov 29, 2022 •
edited
Loading

Seeing an issue where ITK is being rebuilt on MacOS and Linux after a different version of ITKPythonPackage is checked out. Investigating.

EDIT: There is a disparity where ITKPythonPackage is a Git repo in Windows archives but not MacOS or Linux (i.e. is packaged with a .git/ subdirectory). Will revisit the approach for Linux and MacOS, seems like it should be sufficient to clone into a separate repo and replace the scripts/ folder in the unpackaged build folder as appropriate.

@tbirdso tbirdso mentioned this pull request Nov 30, 2022
Closed
dzenanz
dzenanz reviewed Nov 30, 2022
View reviewed changes
@tbirdso tbirdso force-pushed the archive-update-step branch from 1f29eb4 to 4ecbb4f Compare November 30, 2022 17:49
@dzenanz
Copy link
Member

dzenanz commented Nov 30, 2022

After a minor update, now even 3.7 fails with /opt/rh/gcc-toolset-12/root/usr/bin/sudo: line 41: /usr/bin/sudo: No such file or directory. MacOS succeeded.

@tbirdso
Copy link
Contributor Author

tbirdso commented Nov 30, 2022

After a minor update, now even 3.7 fails with /opt/rh/gcc-toolset-12/root/usr/bin/sudo: line 41: /usr/bin/sudo: No such file or directory. MacOS succeeded.

@dzenanz The issue you've described happening in your branch is tracked in #235 . It looks like your branch is fixed at c98c5e3 which does not revert the manylinux image tag. I am now seeing a successful Python 3.7 build in ITKSplitComponents after reverting the manylinux tag. I am convinced that the newer manylinux image is the source of the error you're describing.

I understand that simply reverting the tag will not allow ITKIOOMEZarrNGFF CI to complete as it requires NASM. The resolution for the sudo issue needs to happen in dockcross rather than in ITKPythonPackage. The workaround for other modules that do not require NASM for now is to revert to an earlier image.

Let's continue discussion in #235. I will verify that other ITKSplitComponents builds also pass with these changes and then get this PR cleaned up for easier review.

@tbirdso tbirdso force-pushed the archive-update-step branch from 4ecbb4f to 329438a Compare November 30, 2022 18:28
@tbirdso tbirdso marked this pull request as ready for review November 30, 2022 18:59
@tbirdso
Copy link
Contributor Author

tbirdso commented Nov 30, 2022

@thewtex @dzenanz PR is ready for review at your convenience. Tested by ITKSplitComponents: https://github.com/InsightSoftwareConsortium/ITKSplitComponents/actions/runs/3586129456

This was referenced Nov 30, 2022
Merged
Merged
dzenanz
dzenanz reviewed Nov 30, 2022
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Mostly looks good.

@tbirdso
ENH: Add build script update step to download script
a00ac4c 
Addresses shortfall where ITKPythonBuilds archives are packaged with
ITKPythonPackage build scripts from the time of archive generation, but
no clear mechanism was available for updates to apply build script
patches.

Adds mechanism in download scripts where ITKPythonBuilds archives are
fetched, decompressed, and optionally patched prior to continuing with
module builds.
@tbirdso
ENH: Expose MANYLINUX_VERSION and IMAGE_TAG for Linux builds
583e7a6 
Replaces `TARBALL_SPECIALIZATION` parameter with distinct
`MANYLINUX_VERSION` and `IMAGE_TAG` parameters to be set by the user
prior to build. The resulting script fetches the correct dockcross image
for building specialized wheels and artifacts.

Resolves an issue where `manylinux2014` wheels were attempted to build
on `-manylinux_2_28` images.
@tbirdso
Revert "ENH: Update manylinux images to latest tagged version"
2d3a552 
This reverts commit ecdc6b2.
@tbirdso tbirdso force-pushed the archive-update-step branch from 329438a to 2d3a552 Compare November 30, 2022 20:22
@tbirdso
Copy link
Contributor Author

tbirdso commented Nov 30, 2022

Most recent push applies requested documentation fix.

thewtex
thewtex approved these changes Nov 30, 2022
View reviewed changes
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Thank you @tbirdso !

scripts/dockcross-manylinux-build-module-wheels.sh
MANYLINUX_VERSION=_2_28
IMAGE_TAG=20221128-2024e4b
MANYLINUX_VERSION=${MANYLINUX_VERSION:=_2_28}
IMAGE_TAG=${IMAGE_TAG:=20221108-102ebcc}
Copy link
Member

Choose a reason for hiding this comment

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

cool

@tbirdso tbirdso merged commit 03391ad into master Nov 30, 2022
@thewtex thewtex deleted the archive-update-step branch November 30, 2022 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thewtex thewtex thewtex approved these changes

@dzenanz dzenanz dzenanz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ITKPythonPackage update step in download scripts
3 participants
@tbirdso @dzenanz @thewtex