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

Modernize CI build #614

Merged
merged 35 commits into from
Feb 24, 2025
Merged

Conversation

cvanelteren
Copy link
Contributor

This was rather frustrating to figure but I have gotten it to build using CI and not touching the other part of the codebase. The major issue was that some of the docs on CIBUILD were confusing with paths changing without really good documentation on it. At least the CIBUILD is transferred now and we can move on to other things. There are some warnings that needs addressing in another PR.

@cvanelteren
Copy link
Contributor Author

I need to add the pytest but otherwise it looks good

@molinav molinav self-assigned this Feb 18, 2025
@molinav molinav self-requested a review February 18, 2025 12:40
@cvanelteren cvanelteren marked this pull request as draft February 18, 2025 13:01
Copy link
Member

@molinav molinav left a comment

Choose a reason for hiding this comment

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

I added some comments to your changes. The only one that is important is the one regarding the missing auditwheel repair (targeting manylinux2014/manylinux_2_17), the rest is all minor.

Based on the `cibuildwheel` docs:

    https://cibuildwheel.pypa.io/en/stable/options/

If `CIBW_REPAIR_WHEEL_COMMAND` is not specified, then the default
behaviour for Linux is our desired behaviour:

    auditwheel repair -w {dest_dir} {wheel}

Because `CIBW_REPAIR_WHEEL_COMMAND` was set before to "", the
repair step was being skipped.
This is done by forcing the value of `LD_LIBRARY_PATH` temporarily
to the location of the GEOS shared library.
@cvanelteren
Copy link
Contributor Author

The wheels seems to be missing the data still:

Archive:  wheels-basemap-windows-latest.zip
  inflating: wheels/basemap-1.5.0.dev0-cp310-cp310-win_amd64.whl  
  inflating: wheels/basemap-1.5.0.dev0-cp311-cp311-win_amd64.whl  
  inflating: wheels/basemap-1.5.0.dev0-cp312-cp312-win_amd64.whl  
  inflating: wheels/basemap-1.5.0.dev0-cp39-cp39-win_amd64.whl  
 Archive:  wheels-basemap-ubuntu-latest.zip
  inflating: wheels/basemap-1.5.0.dev0-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl  
  inflating: wheels/basemap-1.5.0.dev0-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl  
  inflating: wheels/basemap-1.5.0.dev0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl  
  inflating: wheels/basemap-1.5.0.dev0-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl  

@molinav
Copy link
Member

molinav commented Feb 18, 2025

If you mean the additional data files (boundaries, rivers, etc.), they belong to basemap-data and basemap-data-hires, and these packages are generated (in sdist and wheel form) in two other artifacts by the workflow.

But now that you mentioned this, I realised that what we are missing is the sdist version of basemap itself. We only have precompiled wheels for basemap, but the source distribution also needs to be generated.

@cvanelteren cvanelteren marked this pull request as ready for review February 18, 2025 19:29
@cvanelteren
Copy link
Contributor Author

cvanelteren commented Feb 18, 2025

The sdist is added. Should the data artifacts be merged or this done in the process?

@molinav
Copy link
Member

molinav commented Feb 18, 2025

I think it is fine for now to have separate artifacts, as long as the upload job picks all of them later.

@molinav
Copy link
Member

molinav commented Feb 18, 2025

I want to pick one wheel for Windows and one for GNU/Linux to see that they install and import properly on my machine, and then it is time to merge!

@molinav molinav force-pushed the workflow-fix-cleaner branch 3 times, most recently from 89bbcd1 to 220cdc1 Compare February 19, 2025 13:07
@molinav molinav force-pushed the workflow-fix-cleaner branch from a22a722 to 35d848f Compare February 20, 2025 19:48
@molinav molinav force-pushed the workflow-fix-cleaner branch from cc9769b to eb91a56 Compare February 21, 2025 18:40
@molinav molinav force-pushed the workflow-fix-cleaner branch from eb91a56 to 21368dc Compare February 21, 2025 18:48
@molinav
Copy link
Member

molinav commented Feb 24, 2025

@cvanelteren I am merging your PR now. I want to play a bit more with a couple of things, but I can make it in a separate branch in my fork without delaying your PR more. In the future I would like to:

  • Improve the setup of GEOS_DIR (it is working but not as I would be expecting).
  • Make before_all platform-independent. For this, a Python script in .github/workflows can be added doing what the before_all snippets are doing right now.
  • At some point, linting (at least flake8) and testing (with pytest) need to be added back to the workflow.

Thanks a lot for your effort!

@molinav molinav merged commit 4974855 into matplotlib:develop Feb 24, 2025
@molinav molinav mentioned this pull request Feb 24, 2025
@cvanelteren
Copy link
Contributor Author

cvanelteren commented Feb 24, 2025

No worries @molinav. I can assist in this if you would like. To respond to your points

  • I had some issues too with the build platforms having platform specific issues and or not working as expected. Hence the roundabout way of doing it. Moving it to a platform agnostic (read python centric) would benefit in future maintainability
  • if the current implementation works, there is no need but I agree with the previous point I wrote here
  • linting could be performed with a precommit to a a PR, pytest could be added to the current build work flow at the end stage. See our package for inspiration

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.

2 participants