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

Switch from Azure to GitHub Actions #37

Merged
merged 17 commits into from
Mar 16, 2022
Merged

Conversation

astrofrog
Copy link
Member

@astrofrog astrofrog commented Mar 10, 2022

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 #8

@ConorMacBride
Copy link
Member

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

@codecov
Copy link

codecov bot commented Mar 10, 2022

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%) ⬇️
extension_helpers/_setup_helpers.py 64.28% <0.00%> (-2.05%) ⬇️
extension_helpers/_openmp_helpers.py 87.17% <0.00%> (-0.86%) ⬇️

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

@astrofrog
Copy link
Member Author

@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
Copy link
Member

ConorMacBride commented Mar 10, 2022

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Contributor

saimn commented Mar 10, 2022

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

@pllim
Copy link
Member

pllim commented Mar 10, 2022

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

@ConorMacBride
Copy link
Member

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
Copy link
Contributor

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

See actions/runner-images#5141

@pllim
Copy link
Member

pllim commented Mar 10, 2022

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

@astrofrog
Copy link
Member Author

@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
Copy link
Member Author

Ah I just saw OpenAstronomy/github-actions-workflows#15

# Test downstream packages
- linux: py39-downstream
env:
OA_WINDOWS_RUNNER: windows-2019
Copy link
Member Author

Choose a reason for hiding this comment

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

@ConorMacBride - I can't figure out what the correct level of indentation needs to be, nothing seems to work. Do you have any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the web editor tell you if you use GitHub web UI to edit the file? You indent/unindent until the red swiggles go away... 😆

Copy link
Member

Choose a reason for hiding this comment

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

Apparently you can't pass custom environment variables to a reusable workflow: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations

I'm working on a new runner input.

Copy link
Member

Choose a reason for hiding this comment

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

You can now remove the OA_WINDOWS_RUNNER and do:

- windows: py36-test
  runs-on: windows-2019
- windows: py38-test-dev
  runs-on: windows-2019

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I've added that here.

@ConorMacBride
Copy link
Member

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 astrofrog marked this pull request as ready for review March 13, 2022 22:03
@astrofrog astrofrog requested a review from Cadair March 13, 2022 22:04
@astrofrog
Copy link
Member Author

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

@@ -0,0 +1,12 @@
on:
Copy link
Member

Choose a reason for hiding this comment

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

This workflow should only run if the test one passes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok done!

@astrofrog astrofrog requested a review from Cadair March 14, 2022 22:49
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

I like it, the templates look good.

@@ -36,6 +36,7 @@ setuptools.finalize_distribution_options =

[options.extras_require]
test =
wheel
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

The test C extension package failed otherwise for some reason.

Copy link
Member

Choose a reason for hiding this comment

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

curious

@Cadair
Copy link
Member

Cadair commented Mar 15, 2022

We waiting for the first release to merge this?

@astrofrog
Copy link
Member Author

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
Copy link
Member Author

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

@astrofrog astrofrog merged commit e3655b9 into astropy:main Mar 16, 2022
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.

Automate releases on PyPI
6 participants