Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
148 changes: 148 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
name: CI

on:
push:
branches: [ master ]
tags: [ "**" ]
pull_request:
branches: [ "**" ]

defaults:
run:
shell: bash

jobs:
build:
name: ${{ matrix.task.name}} - ${{ matrix.os.name }} ${{ matrix.python.name }}
runs-on: ${{ matrix.os.runs-on }}
strategy:
fail-fast: false
matrix:
os:
- name: Linux
runs-on: ubuntu-latest
python:
- name: CPython 3.8
tox: py38
action: 3.8
task:
- name: Build
tox: build

steps:
- uses: actions/checkout@v2

- name: Set up ${{ matrix.python.name }}
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python.action }}

- name: Install dependencies
run: |
python -m pip install --upgrade pip setuptools wheel
python -m pip install build check-manifest twine

- uses: twisted/python-info-action@v1

- name: Build
run: |
check-manifest --verbose .

python -m build --sdist --outdir dist/ .

mkdir empty/
cd empty

tar -xvf ../dist/*
cd *

# build the wheel from the sdist
python -m build --wheel --outdir ../../dist/ .
cd ../../

twine check dist/*

- name: Publish
uses: actions/upload-artifact@v2
with:
name: dist
path: dist/

test:
name: ${{ matrix.task.name}} - ${{ matrix.os.name }} ${{ matrix.python.name }}
runs-on: ${{ matrix.os.runs-on }}
needs:
- build
strategy:
fail-fast: false
matrix:
os:
- name: Linux
runs-on: ubuntu-latest
python:
- name: CPython 3.5
tox: py35
action: 3.5
- name: CPython 3.6
tox: py36
action: 3.6
- name: CPython 3.7
tox: py37
action: 3.7
- name: CPython 3.8
tox: py38
action: 3.8
- name: CPython 3.9
tox: py39
action: 3.9
- name: PyPy 3.6
tox: pypy36
action: pypy-3.6
- name: PyPy 3.7
tox: pypy37
action: pypy-3.7
task:
- name: Test
tox: tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this also run precommit.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could setup whatever to run, sure. I personally haven't gotten up to working on precommit stuff, just hasn't bubbled up the list... but yeah. Maybe make a ticket and list out all the bits you want. As mentioned, I think it's worth getting the basics in and then filling out the rest.


steps:
- uses: actions/checkout@v2

- name: Download package files
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please correct me if I'm wrong -- I'm not familiar with testing Python packages this way.

Wouldn't it be simpler to just install dependencies and run the pre-commit script:

- name: Install dependencies
  run: pip3 install .[dev]

- name: Run checks
  run: python precommit.py

? This simplification would also allow you to dispense of MANIFEST.in?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's see how to fix this in a separate pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just installing from the source doesn't check to see if the built sdist and wheel work. Usually it doesn't matter. Sometimes you get into situations where you end up missing files in the sdist. The purpose of this overall structure is to 1) create the artifacts you are going to publish (sdist and wheel) then 2) test that single wheel file in all places it might be used (that you cover in CI) and 3) publish those same exact artifacts that were built in the first step. Then the thing to be published is the thing that gets tested. Rather than doing several different installations from several different source checkouts and testing that followed by creating an sdist and wheel to upload that never get tested at all.

But, I'm going to unresolve this conversation and take a look at the precommit before moving on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am used to tox being the central location for activity descriptions. It looks like you are using precommit for that role. If using precommit.py, I think it may make sense to add some more configurability (arguments) for it to be used here. I'm used to linting activities (isort, mypy, yapf, etc) being run on the vcs copy and tests with coverage being run on the installed package.

MANIFEST.in is just about describing what should and shouldn't be in your sdist. I almost certainly don't have the MANIFEST.in correct, but I don't think precommit is an alternative.

I'm going to phone a friend on this one since I don't have experience with precommit and what sort of setup works out well for all the use cases. @graingert, if you have a couple minutes to help get me up to speed... thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the clarification re tests! I haven't spent too much thought on it -- it actually does make sense to include the tests so that users can test for the "dependency hell" and that everything works with their virtual environment which might have certain dependencies with different versions already installed.

We used precommit.py just because it was simple and convenient. If you are familiar with tox, please feel free to move the code into tox. In some packages I wanted to add more complex logic into precommits, so tox falls short in that regard. For this particular package, though, tox should be just fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should I merge the pull request as it is or you'd like to integrate first the pre-commit (tox or otherwise)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess go ahead and merge it. As is, it is better than the present state of not having CI. Maybe some of the other bits will be able to be tackled in focused PRs. I'm certainly game for adding more tox, but I don't want to remove the existing precommit functionality that I presume you like. I need to figure out how they play together without too much repetition either of code or of env management.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, merged! You can just split up precommit.py into tox.ini as you see fit.

uses: actions/download-artifact@v2
with:
name: dist
path: dist/

- name: Set up ${{ matrix.python.name }}
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python.action }}

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

- name: Install project
run: |
mkdir empty/
cd empty/
pip install ../dist/*.whl

- uses: twisted/python-info-action@v1

- name: Test
run: |
cd empty/
cp --recursive ../tests .
python -m unittest discover tests/

all:
name: All
runs-on: ubuntu-latest
needs:
- build
- test
steps:
- name: This
shell: python
run: import this
12 changes: 0 additions & 12 deletions .travis.yml

This file was deleted.

12 changes: 12 additions & 0 deletions MANIFEST.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
include *.py
include *.rc
include *.rst
include *.txt
include *.yapf
include .isort.cfg
include mypy.ini
include tox.ini
recursive-include docs *.py
recursive-include docs *.rst
recursive-include docs Makefile
recursive-include tests *.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I noticed this only here. Why do you want to include tests in the distribution? Would this mean that the tests are also shipped to pypi.org?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally put my tests inside the actual package and distribute them everywhere the code goes. But sure, lots of people don't. I'll look into this because perhaps the tests are getting installed even when you just pip install ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

packages=find_packages(exclude=['tests']),

Ok, so you do exclude the tests from installation. Do you feel it is bad to include the tests in the sdist? It seems fairly reasonable to me to have them available for people that get grabbing the source.

To directly respond to your questions, it is useful to have tests available wherever you have the code so that no matter how you have the code you can run the tests. Sure, most people won't leverage this. They pay the price of 36K or whatever of disk space used. A bare venv with nothing extra installed is 15MB so... Again, sure, lots of packages don't do this. I don't know where I would even go for an authoritative should or shouldn't on this but the age old distutils says it defaults to including them.

https://docs.python.org/3/distutils/sourcedist.html#specifying-the-files-to-distribute

anything that looks like a test script: test/test*.py (currently, the Distutils don’t do anything with test scripts except include them in source distributions, but in the future there will be a standard for testing Python module distributions)

As to them being shipped to PyPI. Yes, if they are either in the sdist or the wheel then the expectation would be that they end up on PyPI.

4 changes: 2 additions & 2 deletions README.rst
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
pylddwrap
=========
.. image:: https://travis-ci.com/Parquery/pylddwrap.svg?branch=master
:target: https://travis-ci.com/Parquery/pylddwrap.svg?branch=master
.. image:: https://github.com/Parquery/pylddwrap/actions/workflows/ci.yml/badge.svg?branch=master
:target: https://github.com/Parquery/pylddwrap/actions/workflows/ci.yml?query=branch%3Amaster
:alt: Build Status

.. image:: https://coveralls.io/repos/github/Parquery/pylddwrap/badge.svg?branch=master
Expand Down
5 changes: 4 additions & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,13 @@
'Development Status :: 5 - Production/Stable',
'Intended Audience :: Developers',
'License :: OSI Approved :: MIT License',
'Programming Language :: Python :: Implementation :: CPython',
'Programming Language :: Python :: Implementation :: PyPy',
'Programming Language :: Python :: 3.5',
'Programming Language :: Python :: 3.6',
'Programming Language :: Python :: 3.7',
'Programming Language :: Python :: 3.8'
'Programming Language :: Python :: 3.8',
'Programming Language :: Python :: 3.9',
],
# yapf: enable
license='License :: OSI Approved :: MIT License',
Expand Down