astropy / extension-helpers

Helpers to assist with building Python packages with compiled C/Cython extensions
https://extension-helpers.readthedocs.io
BSD 3-Clause "New" or "Revised" License
16 stars 12 forks source link

Switch from Azure to GitHub Actions #37

Closed astrofrog closed 2 years ago

astrofrog commented 2 years ago

I am trying out the new re-usable OpenAstronomy workflows. For now, I am using @ConorMacBride's fork for the tox testing. This also now adds a publish workflow.

Fixes https://github.com/astropy/extension-helpers/issues/8

ConorMacBride commented 2 years ago

The tox workflow is now merged so you should be able to use the OpenAstronomy main branch.

codecov[bot] commented 2 years ago

Codecov Report

Merging #37 (4cfc229) into main (cd990d7) will decrease coverage by 4.31%. The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #37      +/-   ##
==========================================
- Coverage   79.13%   74.82%   -4.32%     
==========================================
  Files           4        4              
  Lines         278      278              
==========================================
- Hits          220      208      -12     
- Misses         58       70      +12     
Impacted Files Coverage Δ
extension_helpers/_utils.py 74.00% <0.00%> (-18.00%) :arrow_down:
extension_helpers/_setup_helpers.py 64.28% <0.00%> (-2.05%) :arrow_down:
extension_helpers/_openmp_helpers.py 87.17% <0.00%> (-0.86%) :arrow_down:

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

astrofrog commented 2 years ago

@ConorMacBride - just curious, do you have any idea what could be happening in the Windows build here? (it's complaining about not finding the Visual C++ compiler which is needed to build a package with a C extension in the test - I'm curious why this wasn't an issue on Azure though)

ConorMacBride commented 2 years ago

It's saying "Microsoft Visual C++ 14.0 or greater is required" however that version should be installed in the windows-latest image: https://github.com/actions/virtual-environments/blob/main/images/win/Windows2022-Readme.md#microsoft-visual-c

Unless an environment variable is missing (or the test is run isolated from the default environment variables)?

astrofrog commented 2 years ago

Yeah I was wondering if it was a tox isolation thing - it's just curious because this didn't occur on Azure. I'll try and dig deeper.

ConorMacBride commented 2 years ago

Yeah, maybe disabling the tox isolation will make it work? Azure Pipelines is using the windows-2019 image (https://github.com/actions/virtual-environments/blob/win19/20220131.1/images/win/Windows2019-Readme.md) while GitHub Actions is using windows-2022. The windows job seems to expect OpenMP to be installed, although nether of these images lists that as being installed so how did it work before?

saimn commented 2 years ago

I think there was an issue with the recent Windows upgrade on Actions and the Visual C++ compiler. @pllim probably knows more ?

pllim commented 2 years ago

Yes, @lpsinger fixed it by downgrading to windows-2019

ConorMacBride commented 2 years ago

Should we temporarily downgrade Windows in the reusable workflows from windows-latest to windows-2019? Would it be worth adding a way to specify a custom image?

lpsinger commented 2 years ago

Yes, @lpsinger fixed it by downgrading to windows-2019

See https://github.com/actions/virtual-environments/issues/5141

pllim commented 2 years ago

You only have to downgrade if you are compiling C, so 🤷

astrofrog commented 2 years ago

@ConorMacBride - I think it's probably best to downgrade to windows-2019 for now, it's not like we really need the more recent version?

astrofrog commented 2 years ago

Ah I just saw https://github.com/OpenAstronomy/github-actions-workflows/pull/15

ConorMacBride commented 2 years ago

The pinned jinja2==2.10.3 in tox.ini is trying to import a name in a dependency which has now been removed. Is this pin needed? jinja2 is now used by pytest-mpl to produce HTML reports and sunpy is using pytest-mpl, although just running pytest --pyargs sunpy doesn't enable pytest-mpl, it still gets imported. Maybe the jinja import in pytest-mpl should be made made lazy?

astrofrog commented 2 years ago

@Cadair - just FYI I've set PYPI_TOKEN in the repo secrets

Cadair commented 2 years ago

We waiting for the first release to merge this?

astrofrog commented 2 years ago

I guess we might as well at this point as we should be able to tag the workflows so it will be a good test?

astrofrog commented 2 years ago

@Cadair - I've updated the ref for the workflows to v1, feel free to merge if the CI passes!