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

add Python3.9 and Django 2.2 support, drop Python2.7 and Django 1.11support #13

Merged
merged 35 commits into from
Mar 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
1c7ce2f
Python3.9対応の記述を追加
imaxyz Mar 4, 2022
d1fd47f
READMEにPython3.9を追加
imaxyz Mar 4, 2022
62b696c
Travis CIの設定を削除。Github Actionsの設定を追加。
imaxyz Mar 8, 2022
804876d
toxを使わない場合の動作確認
imaxyz Mar 8, 2022
fb40940
toxを使わない場合のgithub actions動作確認
imaxyz Mar 8, 2022
3838514
wip: github actions動作確認
imaxyz Mar 8, 2022
9897f35
wip: github actions動作確認
imaxyz Mar 8, 2022
6ce6c6b
wip: github actions動作確認
imaxyz Mar 8, 2022
f66940e
wip: toxで実行するように戻す
imaxyz Mar 8, 2022
8fcfc5f
bpmailerのユニットテストのpathの記述を修正。コメントを追加。
imaxyz Mar 9, 2022
1b33848
Readmeに記載されているパッケージをrequirements.txtに記述。各種設定ファイルにコメントを追記。
imaxyz Mar 9, 2022
ebc3d14
コメント文を日本語に変更
imaxyz Mar 9, 2022
3ea2674
コメントを修正
imaxyz Mar 10, 2022
96079fb
Github Actionsの python-version にREADMEで対応の表記がある、2.7を追加
imaxyz Mar 10, 2022
e699981
tox-gh-actionsのtox.ini設定に2.7を追加
imaxyz Mar 10, 2022
f537538
2.7でエラーになったのでコメントを一旦英語に変更
imaxyz Mar 10, 2022
7888314
python 2.7サポートを外しました
imaxyz Mar 10, 2022
a1ce88e
python 2.7サポートを外しました
imaxyz Mar 10, 2022
aa6ebde
Travis CIのビルド結果アイコンを、Github Actionsのワークフロー結果のアイコンに変更
imaxyz Mar 10, 2022
733479d
github actionsのワークフローに、これから実行するtoxの動きを確認するコマンドを追加
imaxyz Mar 10, 2022
e40af11
何のためにあるrequirements.txtファイルなのかを説明するコメントを記述しました。
imaxyz Mar 10, 2022
6979781
Githu Actionsのジョブを並列実行するように修正
imaxyz Mar 11, 2022
ed5c025
READMEにおけるgithub actionsのリンクを修正
imaxyz Mar 11, 2022
76d2427
READMEにおけるgithub actionsのリンクを修正
imaxyz Mar 11, 2022
fecf1ae
READMEにおけるgithub actionsのリンクを修正
imaxyz Mar 11, 2022
76b6b17
リンク表現の向上のためREADMEの書式をmarkdownからreStructuredTextに変更
imaxyz Mar 11, 2022
fbe6534
Django 1.11を非対応にする
imaxyz Mar 11, 2022
46d2807
Django 1.11のサポートをドロップする旨を反映
imaxyz Mar 11, 2022
848e7f6
不要と思われるジョブの制御を削除
imaxyz Mar 11, 2022
22a2dd6
ファイル名に意図を込めました
imaxyz Mar 11, 2022
411ef07
classifiersの記述を最新の内容に変更
imaxyz Mar 11, 2022
a1a425a
パッケージを使用する際に要求するPythonバージョンを指定
imaxyz Mar 11, 2022
6b9b31c
`tox -l` はmasterにマージするワークフローには入れないようにする
imaxyz Mar 11, 2022
ba0fbfe
tox.iniの書き方を推敲
imaxyz Mar 11, 2022
2063bf7
依存パッケージの記述を追記。
imaxyz Mar 11, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 38 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
name: Tests

# push と pull request イベント毎にGtihubのWorkflowを起動する
on:
- push
- pull_request

jobs:
# ジョブの名称
test_with_tox:
runs-on: ubuntu-latest

# 並列して実行する各ジョブのPythonバージョン
strategy:
matrix:
Copy link
Member

Choose a reason for hiding this comment

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

ASK: djangoはmatrixにしないんですか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ASKの回答

tox-gh-actionsパッケージの仕様と、bpmailerのtox.iniの記述内容から、bpmailerのworkflow/tests.pyには、djangoのmatrixを記述する必要はないと判断しました。

理由

作業の際、Workflowが実行したジョブのログを確認したところ、自動でtox.iniに記述した各djangoバージョンのバリエーションテストを実行してくれていたので、matrixには記述しませんでした。

この辺の挙動をよく理解できていなかったため、再調査しました。

再調査でわかったこと

tox-gh-actionsパッケージの仕様で、tox.ini の gh-actions の項目にPythonバージョンを記述すると、Github Actionのジョブから実行するtoxでは、gh-actions に記述した各Pythonバージョンに絞った状態で実行ができる仕様があるようです。

次のドキュメントの例だと、

On Python 3.9 job, tox runs py39 environment
https://github.com/ymyzk/tox-gh-actions/blob/master/README.md#basic-example

Python3.9のジョブを実行する際には、py39環境にてtoxを実行してくれるようです。

考察

試しに、workflows/tests.ymlに次のように記述して見たところ

   - name: Test with tox
      run: |
        tox -l  ← 追記
        tox

Github Actionsは、tox-gh-actionsのドキュメントの通り、そのジョブで指定されたPythonバージョンに関するテストのバリエーションを実行していることを確認しました。

Run tox -l 
py36-django  
py36-django22
GLOB sdist-make: /home/runner/work/bpmailer/bpmailer/setup.py
py36-django111 create: /home/runner/work/bpmailer/bpmailer/.tox/py36-django111
tox: py36-django111
py36-django22 create: /home/runner/work/bpmailer/bpmailer/.tox/py36-django22
tox: py36-django22
___________________________________ summary ____________________________________
  py36-django111: commands succeeded
  py36-django22: commands succeeded
  congratulations :)

https://github.com/beproud/bpmailer/runs/5492233415?check_suite_focus=true

備考

workflowに追加した、tox -lのコマンドは今後コードを確認する人が動作を把握するのに役立つと思ったので、一旦そのまま残しました。

Copy link
Member

Choose a reason for hiding this comment

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

@imaxyz matrix化することによって順次実行ではなく並列実行になります。また Python x Django の組合せ名でテスト実行記録が残るためエラー発生時に原因が追いやすくなります。この機会にそのように構成したほうが良いとおもいますが、どうでしょうか?

具体的には以下の様になります。

現在のこのPRのテスト

image

別プロジェクトでの例

https://github.com/jazzband/django-redshift-backend/runs/5347475991?check_suite_focus=true
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

並列化しました。

Python 2.7とDjango 1.11をdropしたため、このPRでは並列化の効果が高くありませんが、今後のPRで恩恵を受けられるので、workflowを並列化する記述に修正しました。

python-version: ['3.6', '3.9']

Choose a reason for hiding this comment

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

3.6はすでにEOLです。3.7に修正して良さそう

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kemu3007 このbpmailerを使いたいプロジェクトがまだPython3.6を使っているため、現時点ではPython3.6対応は残したい感じです。

django-version: ['2.2']

steps:
# ソースコードをチェックアウト
- uses: actions/checkout@v2

# ジョブのPython環境を設定
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python-version }}

# Github Actionsからtoxを実行するために必要なパッケージをインストール
- name: Install dependencies
run: |
python -m pip install --upgrade pip
python -m pip install tox tox-gh-actions

# Github Actionsからtoxを実行
- name: Test with tox
run: |
tox
12 changes: 0 additions & 12 deletions .travis.yml

This file was deleted.

7 changes: 0 additions & 7 deletions README.md

This file was deleted.

10 changes: 10 additions & 0 deletions README.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
.. image:: https://github.com/beproud/bpmailer/actions/workflows/tests.yml/badge.svg
:target: https://github.com/beproud/bpmailer/actions
:alt: GitHub Actions

Requirements
============

* Python (3.6, 3.9)
* Celery (4.1)
* Django (2.2)
5 changes: 5 additions & 0 deletions requirements-test.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# toxを使用せず、直接 python tests.py で単体テスト実行する際に pip installするパッケージ一覧です。
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO: この後Django3.2対応もあると思うので、Django2.2固定のこのファイルは削除することになると思います。なのでこのPRでは一旦よいかもしれませんが、後々は削除したほうが第三者が混乱しなくてよさそうです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なるほどです。ryです。今回は残しておきます。

celery==4.1.0
Django==2.2.27
mock==4.0.3
six==1.15.0
13 changes: 9 additions & 4 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,19 +48,24 @@ def read_file(filename):
author='BeProud Inc.',
author_email='project@beproud.jp',
url='https://github.com/beproud/bpmailer/',
python_requires='>=3.6',
classifiers=[
'Development Status :: 3 - Alpha',
'Environment :: Plugins',
'Framework :: Django',
'Intended Audience :: Developers',
'License :: OSI Approved :: BSD License',
'Programming Language :: Python',
'Programming Language :: Python :: 3',
'Programming Language :: Python :: 3.6',
'Programming Language :: Python :: 3.9',
'Framework:: Django',
'Framework:: Django :: 2.2',
'Intended Audience :: Developers',
'Environment :: Plugins',
'Topic :: Software Development :: Libraries :: Python Modules',
],
include_package_data=True,
packages=find_packages(),
namespace_packages=['beproud', 'beproud.django'],
install_requires=['Django>=1.11', 'six'],
install_requires=['Django>=2.2', 'six', 'Celery'],
test_suite='tests.main',
zip_safe=False,
keywords=['django', 'mail']
Expand Down
12 changes: 8 additions & 4 deletions tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,15 @@ def main():
django.setup()

from django.test.utils import get_runner
test_runner = get_runner(global_settings)

test_runner = test_runner()
tests = ['beproud.django.mailer']
failures = test_runner.run_tests(tests)
# Djangoのtest runnerクラスを取得
TestRunner = get_runner(global_settings)

# test runnerオブジェクトを生成
test_runner = TestRunner()

# test runnerにbpmailerの単体テストのPathを渡して、bpmailerの単体テストを実行する
failures = test_runner.run_tests(['beproud.django.mailer.tests'])
Copy link
Contributor Author

@imaxyz imaxyz Mar 9, 2022

Choose a reason for hiding this comment

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

(Djangoのバージョンが変わったためか?) toxが通らなくなっていたので、Djangoのrun_tests()メソッドのドキュメントを参考に、テストモジュールへのPathを修正しました。

Test labels should be dotted Python paths to test modules, test classes, or test methods.

https://github.com/django/django/blob/e541f2d05b88e58c18b82b622aacc38d670eb5f6/django/test/runner.py#L618

Copy link
Contributor

Choose a reason for hiding this comment

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

原因はsetuptoolsの更新のためのようです。 変更自体はまずはこれでよさそう。

このPRで対応するかは任せますが、 python setup.py test コマンド自体が非推奨になっているのでpytestに置き換えた方がよさそうです。 https://setuptools.pypa.io/en/latest/userguide/commands.html?highlight=test_suite#test-build-package-and-run-a-unittest-suite

image

Copy link
Member

Choose a reason for hiding this comment

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

MUST: レビューのために、この修正の妥当性の説明が欲しいです。

(Djangoのバージョンが変わったためか?)

確信はないけどこうしたら動いた、ということでしょうか。

Copy link
Contributor Author

@imaxyz imaxyz Mar 11, 2022

Choose a reason for hiding this comment

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

現在 bpmailerのtoxがfailする原因について

@kashewnuts

  • 頂いたコメントに関して、py36, py39共に、bpmailerのtox.iniの testenv - deps の項目に、次の行を追加することでtoxから実行する単体テストがpassすることを確認しました。
setuptools<=51.1.2

この改修がsetuptoolsの51.1.2の次のバージョンである51.2.0でマージされたことも確認しました

このことから、今回の作業でbpmailerのtoxがfailしている原因は、setuptoolsが、bpmailerをpy36対応した時の作業の時と比べてバージョンアップしていて、動作が変わっていたことが原因と思いました。

python setup.py test コマンドが非推奨の件

@kashewnuts

#14 でissueにしました。別PRで対応できればと思います。

今回の改修方法の妥当性について

@shimizukawa

(Djangoのバージョンが変わったためか?)

最初上記のようにコメントしたのですが、上述のsetuptoolsの調査結果から、Djnagoのバージョンは関係なさそうです。

setuptoolsの改修で使われているメソッドの差分について調べられたらベストなのですが、まだ調べられていません。

改修方法の妥当性について、確信は無いのですが、bpmailerが使用するバージョンのDjnagoのrun_tests()のコメント

Test labels should be dotted Python paths to test modules, test classes, or test methods.
意訳: ラベルは「テストモジュールか、テストクラスか、テストメソッド」にすべき

と書かれているのに対して、テストモジュールの上位のパッケージ名(mailer)を指定していることから、Djangoが想定するパラメータ仕様に近づける意味で、適切ではと考えました。

# before
test_runner.run_tests(['beproud.django.mailer'])

# after
test_runner.run_tests(['beproud.django.mailer.tests'])

あと、tox.iniのdeps項目に、setuptoolsのバージョンを指定すべきか?を考えました。

現在の改修方法だと、setuptoolsのバージョンが51.2.0未満でも51.2.0以上でもtoxがpassしました。
なので、tox.iniにsetuptoolsのバージョン指定を記述する必要ないと考えました。

Copy link
Member

Choose a reason for hiding this comment

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

OKです。別PRでtest.pyを削除するのであれば、問題ないです。


sys.exit(failures)

Expand Down
20 changes: 6 additions & 14 deletions tox.ini
Original file line number Diff line number Diff line change
@@ -1,29 +1,21 @@
# Requires tox > 1.8

[tox]
envlist = py27-django111, py36-django{111,22}
envlist = py{36,39}-django22

[testenv]
basepython =
py27: python2.7
py36: python3.6
py39: python3.9
deps =
six
django111: Django>=1.11,<2.0
django111: celery>=4.0,<4.2
django22: Django>=2.2,<3.0
django22: celery>=4.0,<4.2
mock>=0.7.2
commands=python setup.py test

[travis]
os =
linux: py27-django111, py36-django{111,22}
# tox-gh-actionsパッケージの設定
[gh-actions]
python =
2.7: py27
3.6: py36

[travis:env]
DJANGO =
1.11: django111
Copy link
Member

Choose a reason for hiding this comment

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

MUST: 1.11をドロップしたのであれば、setup.pyも更新してください。

install_requires=['Django>=1.11', 'six'],

以下、ついでに依頼です。
classifiers も更新お願いします。

bpmailer/setup.py

Lines 51 to 59 in e44f6b4

classifiers=[
'Development Status :: 3 - Alpha',
'Environment :: Plugins',
'Framework :: Django',
'Intended Audience :: Developers',
'License :: OSI Approved :: BSD License',
'Programming Language :: Python',
'Topic :: Software Development :: Libraries :: Python Modules',
],

参考記述

https://github.com/jazzband/django-redshift-backend/blob/6b862eca2e1c6d5f66fe0dd4e1646be573256f59/setup.cfg#L15-L25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setup.pyの記述内容を修正しました。

2.2: django22
3.6: py36
3.9: py39