Skip to content

python3-pytest: add new package#8562

Closed
ja-pa wants to merge 10 commits into
openwrt:masterfrom
ja-pa:pytest
Closed

python3-pytest: add new package#8562
ja-pa wants to merge 10 commits into
openwrt:masterfrom
ja-pa:pytest

Conversation

@ja-pa
Copy link
Copy Markdown
Contributor

@ja-pa ja-pa commented Apr 1, 2019

Maintainer: me @ja-pa
Compile tested: Turris Omnia (TOS4), OpenWrt 18.06.2
Run tested: Turris Omnia (TOS4), OpenWrt 18.06.2

Description:
Pytest is a popular testing framework for python. It allows to write scalable and complex test set. It also supports multiple useful plugins like xdist pytest-django and more (see http://plugincompat.herokuapp.com)

I runtested package with examples provided in docs

This PR also adds python dependency required by pytest

  • pluggy
  • atomicwrites
  • more-itertools
  • py
  • python3-importlib-metadata
  • python3-wcwidth
  • python3-zipp
  • python3-packaging
  • python3-pyparsing

Why add this package:

  • Pytest is a dependency for Deckard (https://gitlab.labs.nic.cz/knot/deckard/ ) utility, which tests DNS resolvers on the network to let users know when their ISP is mishandling DNS. I would like to submit Deckard in some future PR.
  • It's nice to have pytest on a router for testing new python packages and also updates.

Comment thread lang/python/atomicwrites/Makefile Outdated
@ja-pa ja-pa marked this pull request as ready for review April 3, 2019 12:22
@ja-pa
Copy link
Copy Markdown
Contributor Author

ja-pa commented Apr 3, 2019

cc @jefferyto, @commodo

Copy link
Copy Markdown
Member

@jefferyto jefferyto left a comment

Choose a reason for hiding this comment

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

Some cross-package comments:

  • atomicwrites, more-itertools, pluggy, py: Would prefer to set URL to the homepage link as declared on their PyPI project pages (in the Project links sidebar section).

    For py, I know URL is already set to their readthedocs.io link, but would prefer to have it exactly as on PyPI (https is okay though), so that the visitor's browser will redirect to the most appropriate language version.

  • pluggy, py, pytest-xdist, pytest: These packages use setuptools-scm during setup (you can see this in the setup_requires list in their setup.py). IIRC without this, the installed packages will not the have correct version number in their .egg-info (I haven't compiled these packages to verify). Can you check to see if this is the case?

    The way I've dealt with this before, is to pass the Makefile version number to setup.py by adding a variable to PYTHON3_PKG_SETUP_VARS, then patching setup.py to use the Makefile version instead of setuptools-scm. (See python-automat for an example.) You may also need to "patch in" the file that setuptools-scm writes to.

Comment thread lang/python/pytest-xdist/Makefile Outdated
Comment thread lang/python/pytest/Makefile Outdated
@ja-pa ja-pa changed the title python3-pytest: add new package WIP python3-pytest: add new package Apr 17, 2019
Comment thread lang/python/pluggy/Makefile
@commodo
Copy link
Copy Markdown
Contributor

commodo commented Apr 18, 2019

From my side, this looks good (generally).
After resolving comments on this and fixing the build, it should be ok.

Looks like some os imports need handling

Traceback (most recent call last):
  File "./setup.py", line 45, in <module>
    main()
  File "./setup.py", line 31, in main
    version=os.getenv('PKG_VERSION'),
NameError: name 'os' is not defined

@jefferyto
Copy link
Copy Markdown
Member

I see you have updated the URL fields - thanks!


Regarding setuptools-scm, I see you have patches to supply setuptools with version numbers, but this is only half the task. Taking pluggy as an example, we can see its use_scm_version directive is:

use_scm_version={"write_to": "pluggy/_version.py"},

which tells setuptools-scm to both pass the version number to setuptools, and write the version number to the file "pluggy/_version.py".

The template setuptools-scm uses for this file looks like this:

# coding: utf-8
# file generated by setuptools_scm
# don't change, don't track in version control
version = {version!r}

Here {version!r} is basically replaced with the version number (technically a string). pluggy tries to import this value in its __init__.py:

try:
    from ._version import version as __version__
except ImportError:
    # broken installation, we don't even try
    # unknown only works because we do poor mans version compare
__version__ = "unknown"

(__version__ is a "quasi-standard" variable for Python packages to store/expose version numbers.) So in order to replicate this, we can adding something like this to the pluggy Makefile (somewhere after include ../python3-package.mk; note I haven't tested this):

define Py3Build/Compile
	echo "version = \"$(PKG_VERSION)\"" > $(PKG_BUILD_DIR)/pluggy/_version.py
	$(call Py3Build/Compile/Default)
endef

Would appreciate it if you can go through the affected packages (pluggy, py, pytest-xdist, pytest) and add this treatment (in addition to the "do not use setuptools-scm" patches). (I know the example I gave before (python-automat) didn't have this; that package didn't try to write the version number to a file.)

Thanks 🙏 😂

@ja-pa ja-pa changed the title WIP python3-pytest: add new package python3-pytest: add new package Jun 11, 2019
@ja-pa
Copy link
Copy Markdown
Contributor Author

ja-pa commented Jun 11, 2019

@jefferyto @commodo Hi, I believe that I fixed PR according to your suggestion. I removed pytest-xdist plugin package, updated all packages to the latest version and cherry-picked new one from #9096 .
I run tested pytest with examples provided in docs and It worked fine.
Will be glad for feedback, thx.

Comment thread lang/python/python3-importlib-metadata/Makefile Outdated
@BKPepe
Copy link
Copy Markdown
Member

BKPepe commented Jun 11, 2019

@ja-pa I have just one request. Would you please add in the commit message for python3-zipp and python3-importlib-metdata above your signed-off-by that you added patches for those two? We would have the same commit message format as OpenWrt in its main repository.

As an example what I mean is openwrt/openwrt@64493d4

It's up to you. It's not required, but it would be good to have it.

Comment thread lang/python/python3-pyparsing/Makefile Outdated
@BKPepe
Copy link
Copy Markdown
Member

BKPepe commented Jun 11, 2019

Otherwise, it looks good to me.

@ja-pa
Copy link
Copy Markdown
Contributor Author

ja-pa commented Jun 11, 2019

@BKPepe changed as requested. I added explanatory comments to python3-zipp and python3-importlib-metdata commit messages

@aparcar
Copy link
Copy Markdown
Member

aparcar commented Jun 11, 2019

@ja-pa You might like this tool here to automate pip package maintaining #9220

@commodo
Copy link
Copy Markdown
Contributor

commodo commented Jun 12, 2019

One general note: maybe sending some of the PKG_VERSION patches upstream would be interesting.
It should be interesting for them as well as for us here.

@aparcar
Copy link
Copy Markdown
Member

aparcar commented Jun 12, 2019

@ja-pa There is no Copyright over the cherry-picked Makefiles, can this be a problem? If so please feel free to add your own Copyright.

@ja-pa
Copy link
Copy Markdown
Contributor Author

ja-pa commented Jun 12, 2019

@aparcar For me it's no problem. If you what to give there your copyright info I have no problem with that.

@aparcar
Copy link
Copy Markdown
Member

aparcar commented Jun 13, 2019

@ja-pa are you trying to get you patches upstream? I'd be happy to get rid of these patches at some point. Apart from that I'd be happy to merge this (as I need pytest to work) - objections?

Copy link
Copy Markdown
Member

@jefferyto jefferyto left a comment

Choose a reason for hiding this comment

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

Thanks for following up on the version files 😂

I noticed a few more odds and ends that need to be addressed, but nothing major.

Comment thread lang/python/pytest/Makefile Outdated
+python3-importlib-metadata \
+python3-packaging \
+python3-wcwidth \
+python3-zipp
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.

pytest doesn't depend on zipp (see below for what does).

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.

SUBMENU:=Python
TITLE:=Like importlib_resources only for metadata
URL:=https://gitlab.com/python-devs/importlib_metadata
DEPENDS=+python3-light
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.

Comment thread lang/python/pluggy/Makefile Outdated
CATEGORY:=Languages
TITLE:=pluggy
URL:=https://github.com/pytest-dev/pluggy
DEPENDS:=+python3-light
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.

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.

pluggy still depends on importlib-metadata.

Comment thread lang/python/python3-zipp/Makefile Outdated
PKG_HASH:=a9f185022cfa69e9ca5f7eabfd5a58b689894cb78a11e3c8c89398a8ccbb8e7f

PKG_MAINTAINER:=Paul Spooren <mail@aparcar.org>
PKG_LICENSE:=MIT
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.

The license is Apache-2.0.

Comment thread lang/python/python3-zipp/Makefile Outdated
Comment thread lang/python/python3-packaging/Makefile Outdated
Comment thread lang/python/python3-packaging/Makefile Outdated

PKG_MAINTAINER:=Jan Pavlinec <jan.pavlinec@nic.cz>
PKG_LICENSE:=BSD
PKG_LICENSE_FILES:=LICENSE
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.

Please add LICENSE.APACHE and LICENSE.BSD.

Comment thread lang/python/python3-wcwidth/Makefile Outdated
@jefferyto
Copy link
Copy Markdown
Member

I don't think the PKG_VERSION patches make sense upstream. I would assume they deliberately chose to use setuptools-scm.

Actually... now that I think about it, it should be possible to set:

HOST_PYTHON3_PACKAGE_BUILD_DEPENDS:=setuptools-scm

and have it installed host-side. I believe (although not 100% sure) all archives downloaded from PyPI have a PKG-INFO with the version number, and if it doesn't, we can set SETUPTOOLS_SCM_PRETEND_VERSION in the Makefile.

Apologies @ja-pa for making you jump through all those hoops - while I think the current version number handling is good enough to be merged, this might be the easier/better way of managing this.

Comment thread lang/python/python3-importlib-metadata/Makefile Outdated
Comment thread lang/python/more-itertools/Makefile Outdated
Comment thread lang/python/more-itertools/Makefile Outdated
Comment thread lang/python/more-itertools/Makefile Outdated
Comment thread lang/python/python-wcwidth/Makefile
Comment thread lang/python/python-zipp/Makefile
Copy link
Copy Markdown
Member

@BKPepe BKPepe left a comment

Choose a reason for hiding this comment

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

Update importlib-metadata to version 0.19.

Comment thread lang/python/python-importlib-metadata/Makefile Outdated
Comment thread lang/python/python-importlib-metadata/Makefile Outdated
@commodo
Copy link
Copy Markdown
Contributor

commodo commented Sep 2, 2019

this PR is kinda old now; since April

i don't know if we had a comment on this PR [but it was on one related to pytest], but at this point in time, it really looks like splitting this PR into smaller PRs is easier;
i know/remember the reply was "but we're almost there, so let's not split it just now"

at this rate, we will be stuck with this re-re-re-re-reviewing for another 4-5 months

also, this PR has 103 comments already;
let's take this as a learning opportunity and cut this PR here, and split [well, that's my 2 cents here]

also, i really-really-really-really-really don't want to offend anyone here; i am pretty relaxed now [since coming back from a vacation], but this reminds me of an old internet pic [which i find amusing in this case]:

reviews

@ja-pa
Copy link
Copy Markdown
Contributor Author

ja-pa commented Oct 29, 2019

@commodo Yes, I think that it is best way how to handle this.

ja-pa and others added 10 commits October 29, 2019 16:09
Signed-off-by: Jan Pavlinec <jan.pavlinec@nic.cz>
Signed-off-by: Jan Pavlinec <jan.pavlinec@nic.cz>
Signed-off-by: Jan Pavlinec <jan.pavlinec@nic.cz>
Signed-off-by: Jan Pavlinec <jan.pavlinec@nic.cz>
Signed-off-by: Jan Pavlinec <jan.pavlinec@nic.cz>
Signed-off-by: Paul Spooren <mail@aparcar.org>
Signed-off-by: Jan Pavlinec <jan.pavlinec@nic.cz>
[added patch for scm version and Makefile polishing]
Signed-off-by: Jan Pavlinec <jan.pavlinec@nic.cz>
Signed-off-by: Paul Spooren <mail@aparcar.org>
Signed-off-by: Jan Pavlinec <jan.pavlinec@nic.cz>
[added patch for scm version and Makefile polishing]
Signed-off-by: Jan Pavlinec <jan.pavlinec@nic.cz>
Signed-off-by: Jan Pavlinec <jan.pavlinec@nic.cz>
@aparcar
Copy link
Copy Markdown
Member

aparcar commented Nov 1, 2019

Maybe we should migrate this also to pypi.mk?

@ja-pa
Copy link
Copy Markdown
Contributor Author

ja-pa commented Nov 1, 2019

@aparcar Yes I agree (see MR #10405, #10407, #10408, #10410, #10411)

@BKPepe
Copy link
Copy Markdown
Member

BKPepe commented Jan 1, 2020

ping @ja-pa

@ja-pa
Copy link
Copy Markdown
Contributor Author

ja-pa commented Apr 11, 2020

Merged in this PR #8562 Thx all for the feedback!

@ja-pa ja-pa closed this Apr 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants