Skip to content

workflow and automation cleanups #309

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

Closed
sarnold wants to merge 2 commits into from
Closed

workflow and automation cleanups #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae50765
fix: dev: update action/python versions, packaging, and tox file
sarnold Feb 28, 2025
5503276
chg: address PR feedback, update ci matrix, add pypi upload job to re…
sarnold Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,43 @@ concurrency:
jobs:
build-test:
runs-on: ${{ matrix.os }}
defaults:
run:
shell: bash

@aaronliu0130 aaronliu0130 Mar 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see any bash-specific features used.

@sarnold sarnold Mar 6, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Better that than dash. It's generally better to set what you want and not let GH runner environment decide.

@aaronliu0130 aaronliu0130 Mar 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But we're only using sh features.

@jayvdb jayvdb Mar 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO this is a very good addition to any GHA workflow. Keeping to strictly sh is not normal behaviour for devs now - and there are some subtle gotchas when not using an explicit shell , c.f. rhysd/actionlint#374

env:
OS: ${{ matrix.os }}
PYTHON: ${{ matrix.python-version }}
PYTHONIOENCODING: utf-8
PIP_DOWNLOAD_CACHE: ${{ github.workspace }}/../.pip_download_cache
strategy:
matrix:
# Python 3.8 is the last non-EOL version. Also adapt tox.ini
python-version: ['3.8', 'pypy3.10', '3.12']
os: [ubuntu-latest, windows-latest]
fail-fast: false
matrix:
os: [macos-15, macos-latest, ubuntu-latest, ubuntu-22.04, ubuntu-24.04-arm, windows-latest, windows-2025]
python-version: [3.9, '3.10', '3.11', '3.12']

steps:
- name: Checkout
uses: actions/checkout@v4
with:
fetch-depth: 0

- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
architecture: x64
allow-prereleases: true
cache: 'pip'
cache-dependency-path: '**/test-requirements'
env:
SETUPTOOLS_USE_DISTUTILS: ${{ startsWith(matrix.os, 'macos') || matrix.os == 'ubuntu-24.04-arm' && matrix.python-version == '3.9' && 'stdlib' || 'local' }}

- name: Install dependencies
run: |
python -m pip install --upgrade pip
python -m pip install -e .[dev]
python -m pip install 'tox-gh-actions<4.0.0'
pip install tox tox-gh-actions

@cclauss cclauss Mar 2, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not really needed with tox -e py in GitHub Actions.

@sarnold sarnold Mar 7, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

tox -e py is implicitly selecting whatever python version is the environment default and we don't want that.

@aaronliu0130 aaronliu0130 Mar 7, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We specified the Python environment in the python action step.


- name: Test with tox
run: tox
run: |
tox
env:
PLATFORM: ${{ matrix.os }}
117 changes: 117 additions & 0 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
name: Release

on:
push:
# release on tag push
tags:
- '*'

jobs:
wheels:

runs-on: ${{ matrix.os }}
defaults:
run:
shell: bash
env:
PYTHONIOENCODING: utf-8
strategy:
fail-fast: false
matrix:
os: [macos-15, macos-latest, ubuntu-latest, ubuntu-22.04, ubuntu-24.04-arm, windows-latest, windows-2025]

@aaronliu0130 aaronliu0130 Mar 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought wheels were platform-independent.

@sarnold sarnold Mar 6, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

non-platform wheels are, but the point here is the platform environments are different.

@aaronliu0130 aaronliu0130 Mar 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, I don't quite get what you're saying. Could you elaborate on the possible differences?

@sarnold sarnold Mar 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't quite get what your objection and/or alternative is. Can you elaborate more on what you want? I'm not trying to be weird here, but the really simple/short workflows don't seem to "last" very long due to arbitrary changes in runner environments. I just had to fix coverage workflows in several different repos this week after GitHub suddenly decided to stop finding the right version ov llvm-cov (after the same workflows were working fine for literally years).

python-version: [3.9, '3.10', '3.11', '3.12']

steps:
- name: Set git crlf/eol
run: |
git config --global core.autocrlf false
git config --global core.eol lf

- uses: actions/checkout@v4
with:
fetch-depth: 0

- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
allow-prereleases: true
cache: 'pip'
cache-dependency-path: '**/test-requirements'
env:
SETUPTOOLS_USE_DISTUTILS: ${{ matrix.os == 'macos-latest' && matrix.python-version == '3.9' && 'stdlib' || 'local' }}

@cclauss cclauss Mar 2, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Distutils is history.

@sarnold sarnold Mar 3, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is about specifically what versions of bundled python modules are installed with the (now older) python on the Github CI runners. It has nothing to do with the project packaging.

@aaronliu0130 aaronliu0130 Mar 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, and we don't need distutils in any Python version.


- name: Install dependencies
run: |
python -m pip install --upgrade pip wheel
pip install tox

- name: Build dist pkgs
run: |
tox -e build,check

- name: Upload artifacts
if: matrix.python-version == 3.9 && runner.os == 'Linux'
uses: actions/upload-artifact@v4
with:
name: packages
path: dist

create_release:
name: Create Release
needs: [wheels]
runs-on: ubuntu-22.04

steps:
- name: Get version
id: get_version
run: |
echo "VERSION=${GITHUB_REF/refs\/tags\//}" >> $GITHUB_ENV
echo ${{ env.VERSION }}

- uses: actions/checkout@v4
with:
fetch-depth: 0

# download all artifacts to project dir
- uses: actions/download-artifact@v4

- name: Generate changes file
uses: sarnold/gitchangelog-action@915234f151ceffb7a8c4f76de77e4ae321087b8f # 1.1.1
with:
github_token: ${{ secrets.GITHUB_TOKEN}}

- name: Create release
id: create_release
uses: softprops/action-gh-release@v1
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
tag_name: ${{ env.VERSION }}
name: Release v${{ env.VERSION }}
body_path: CHANGES.md
draft: false
prerelease: false
files: |
packages/cpplint*

# When a GitHub release is made, upload the artifacts to PyPI
upload:
name: Upload to PyPI
runs-on: ubuntu-22.04
needs: [wheels]

steps:
- uses: actions/setup-python@v5

- uses: actions/download-artifact@v4

- name: check artifacts
run: find packages/ -maxdepth 2 -name cpplint-\* -type f

# - name: Publish bdist and sdist packages
# uses: pypa/gh-action-pypi-publish@release/v1
# with:
# password: ${{ secrets.PYPI_API_TOKEN }}
# packages_dir: packages/
# print_hash: true
9 changes: 4 additions & 5 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ def read_without_comments(filename):
with open(filename) as f:
return [line for line in f.read().splitlines() if not len(line) == 0 and not line.startswith('#')]

test_required = read_without_comments('test-requirements')

setup(name='cpplint',
version=cpplint.__VERSION__,
Expand All @@ -30,19 +29,19 @@ def read_without_comments(filename):
'License :: OSI Approved :: BSD License',
'Natural Language :: English',
'Programming Language :: Python :: 3 :: Only',
'Programming Language :: Python :: 3.8',
'Programming Language :: Python :: 3.9',
'Programming Language :: Python :: 3.10',
'Programming Language :: Python :: 3.11',
'Programming Language :: Python :: 3.12',
'Programming Language :: Python :: 3.13',
'Programming Language :: C++',
'Topic :: Software Development :: Quality Assurance'],
description='Automated checker to ensure C++ files follow Google\'s style guide',
long_description=open('README.rst').read(),
long_description_content_type='text/x-rst',
license='BSD-3-Clause',
tests_require=test_required,
# extras_require allow pip install .[dev]
extras_require={
'test': test_required,
'dev': read_without_comments('dev-requirements') + test_required
'test': read_without_comments('test-requirements'),
'dev': read_without_comments('dev-requirements'),
})
94 changes: 87 additions & 7 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,100 @@
[tox]
envlist = py38, py39, py3.10, py311, py312, pypy3
envlist = pypy3,py3{9,10,11,12,13}-{linux,macos,windows}

@aaronliu0130 aaronliu0130 Mar 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fairly sure a machine where you run tox can only have one OS loaded at a time. Is this needed?

@sarnold sarnold Mar 6, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That ^^ matrix is how tox constructs env commands, including PLATFORM matrix.

@aaronliu0130 aaronliu0130 Mar 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the PLATFORM matrix for?

skip_missing_interpreters = true
isolated_build = true
skipsdist = true

[gh-actions]
python =
3.8: py38
3.9: py39
3.10: py310
3.11: py311
3.12: py312
pypy3.10: pypy3
3.13: py313

[gh-actions:env]
PLATFORM =
ubuntu-latest: linux
ubuntu-22.04: linux
ubuntu-24.04-arm: linux
macos-latest: macos
macos-15: macos
windows-latest: windows
windows-2025: windows

[testenv]
extras = dev
skip_install = true

passenv =
DISPLAY
XAUTHORITY
HOME
USERNAME
USER
XDG_*
CI
OS
PYTHONIOENCODING
PIP_DOWNLOAD_CACHE

setenv =
PYTHONPATH = {toxinidir}

allowlist_externals =
bash
Comment on lines +28 to +44

@aaronliu0130 aaronliu0130 Mar 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are these really needed?


deps =
-e .[test,dev]

@aaronliu0130 aaronliu0130 Mar 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there an advantage for this over "extras"?

@sarnold sarnold Mar 6, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Visibility? otherwise it's just -e .

@aaronliu0130 aaronliu0130 Mar 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does that change anything? AFAIK tox sandboxes the environment anyways.


commands =
python -m pytest {posargs:}
python -m pylint cpplint.py
python -m flake8 cpplint.py

[testenv:build]
skip_install = true

passenv =
pythonLocation
CI
PYTHONIOENCODING
PIP_DOWNLOAD_CACHE

deps =
pip>=21.1
build
twine

commands =
python -m build .
twine check dist/*

[testenv:check]
skip_install = true
allowlist_externals =
bash

passenv =
CI
GITHUB*
PIP_DOWNLOAD_CACHE

deps =
pip>=21.1

changedir =
dist/

commands =
python -m pip install cpplint --force --pre --prefer-binary -f ./

[testenv:clean]
skip_install = true
allowlist_externals =
bash

deps =
pip>=21.1

commands =
{envpython} -m pytest {posargs:}
{envpython} -m pylint cpplint.py
{envpython} -m flake8 cpplint.py
bash -c 'rm -rf build/ dist/ __pycache__/ *.egg-info/ .coverage'

@aaronliu0130 aaronliu0130 Mar 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason to specifically use bash here? Why not

Suggested change
bash -c 'rm -rf build/ dist/ __pycache__/ *.egg-info/ .coverage'
rm -rf build/ dist/ __pycache__/ *.egg-info/ .coverage

@sarnold sarnold Mar 6, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In this case either bash or rm needs to be in the allow list. Bash is usually my default "allow" and that gets used to run any other shell commands. In this it's just "rm".

@aaronliu0130 aaronliu0130 Mar 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

GitHub workflows have an allowlist for commands?