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

Replace tox.ini and Makefile for noxfile.py #731

Merged
merged 9 commits into from
Dec 11, 2024

Conversation

slimreaper35
Copy link
Member

https://nox.thea.codes/en/stable

Notes

  • Each session is defined as a Python function with the @nox.session decorator
  • reuse_venv=True is used to speed up the execution of sessions by reusing the existing venv
  • Docstrings below each session function are used as session descriptions in the help output
  • The module docstring is used in the help output when running nox -l, --list too
  • The *session.posargs is used to pass additional arguments to the command
  • The silent=True argument is used to suppress the output of the command only if the command was successful
  • To use external commands, set the external=True argument in the session.run() method
    (it should work without the external=True argument, but it's better to be explicit and silence the warning)
  • The only command from Makefile that is not included here is make venv
    (it probably doesn't make sense to have a session for that, it would require contributors to install nox first)
  • I made some minor adjustments to pytest commands to make them more readable and clean

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • Docs updated (if applicable)
  • Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

Copy link
Member

@eskultety eskultety left a comment

Choose a reason for hiding this comment

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

@slimreaper35 this is a very shallow top-level review I've done without trying it locally myself, but from the first look it looks fine, definitely doesn't hinder readability when compared to the declarative tox.ini config, so from that POV seems like a solid alternative to what we have today.

noxfile.py Outdated Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
Copy link
Contributor

@a-ovchinnikov a-ovchinnikov left a comment

Choose a reason for hiding this comment

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

This looks so neat!

noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
@slimreaper35 slimreaper35 force-pushed the nox branch 3 times, most recently from 6c0616a to aad2d24 Compare November 14, 2024 09:16
Copy link
Contributor

@a-ovchinnikov a-ovchinnikov left a comment

Choose a reason for hiding this comment

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

A couple small questions, but LGTM overall.

pyproject.toml Show resolved Hide resolved
tox.ini Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
@slimreaper35 slimreaper35 changed the title Proposal: Replace nox for testing automation instead of tox and/or Makefile Replace nox for testing automation instead of tox and Makefile Nov 18, 2024
noxfile.py Outdated Show resolved Hide resolved
"""Run mypy on cachi2 and tests directories"""
install_requirements(session)
cmd = "mypy --install-types --non-interactive cachi2 tests"
session.run(*cmd.split(), *session.posargs, silent=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Obligatory [non-blocking] tag.

Would this be too much of generalization?

@nox.session()
def mypy(session: Session) -> None:
    """Run mypy on cachi2 and tests directories"""
    run_cmd_for(session, "mypy --install-types --non-interactive cachi2 tests")

def run_cmd_for(session, cmd, loud=False):
    install_requirements(session)
    session.run(*cmd.split(), *session.posargs, silent=not loud)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, to me it feels like an unnecessary wrapper around each command.

Copy link
Contributor

@ben-alkov ben-alkov Nov 25, 2024

Choose a reason for hiding this comment

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

Given that all of the commands are exactly the same, maintainability would be increased by

def run_cmd(cmd, session):
	install_requirements(session)
    session.run(*cmd.split(), *session.posargs, silent=True)

called from the individual sessions as run_cmd(cmd, session).

Why? I realize that it seems excessive, but:

  • It actually reduces the amount of repetition. If we need to change the options to session.run, we can do it in ONE place instead of ten
  • If we do have one specific session which needs to have a slightly different session.run, we can call session.run directly from that specific session, i.e. ignoring our "wrapper". As it stands, if one out the many sessions has different options, it's really hard to see.

@slimreaper35 slimreaper35 force-pushed the nox branch 3 times, most recently from 761d02b to ad91366 Compare November 19, 2024 12:59
@ben-alkov
Copy link
Contributor

clean:
	rm -rf dist venv .tox *.egg-info *.log*

mock-unittest-data:
	hack/mock-unittest-data/gomod.sh

I'm not advocating for keeping these, but they must be documented somewhere. I can't even remember the complete list of what "clean" takes care of, much less how to refresh the mock data 😆 ...

@slimreaper35 slimreaper35 changed the title Replace nox for testing automation instead of tox and Makefile Replace tox.ini and Makefile for noxfile.py Nov 25, 2024
@slimreaper35
Copy link
Member Author

I'm not advocating for keeping these, but they must be documented somewhere. I can't even remember the complete list of what "clean" takes care of, much less how to refresh the mock data 😆 ...

I've never used hack/mock-unittest-data/gomod.sh or hack/mock-unittest-data/yarn.py

@ben-alkov
Copy link
Contributor

I'm not advocating for keeping these, but they must be documented somewhere. I can't even remember the complete list of what "clean" takes care of, much less how to refresh the mock data 😆 ...

I've never used hack/mock-unittest-data/gomod.sh or hack/mock-unittest-data/yarn.py

All the more reason to document them, before the last person who actually did run them gets hit by a bus 😁

Copy link
Member

@eskultety eskultety left a comment

Choose a reason for hiding this comment

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

What I'm missing in this PR:

  • adding .nox to .gitignore
  • making the final switch to nox from tox/Makefile, i.e. dropping latter
  • docs changes

tox.ini Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
@brunoapimentel
Copy link
Contributor

This LGTM, it only needs the changes that Erik pointed out.

With these changes, the only reason to create a venv outside of .nox is to run the cachi2 command itself? I'm wondering if we need to keep the make venv command at all. It seems to me that the best approach is to install nox as a system-wide package, so we can probably move the creation of the cachi2 venv to nox.

We already set a precedense to have all configuration
options for linters,formatters etc. inside one file - pyproject.toml.

The pytest.ini options are still inside tox.ini.
Since tox.ini might be replaced in the future, let's move the options
to pyproject.toml for consistency and to avoid test failures
later on.

`testpaths` option was dropped completely - not relevant for us

Signed-off-by: Michal Šoltis <[email protected]>
The commit follows the same reasoning as the previous one with moving
configuration options to one location.

`exclude` option is not relevant for us - we run flake8 on specific
targets

flake8 on its own does not support pyproject.toml as a configuration
file [1] -> install flak8-pyproject package and regenarete requirements
files

---
[1]: https://flake8.pycqa.org/en/latest/user/configuration.html#configuration-locations

Signed-off-by: Michal Šoltis <[email protected]>
gitpython is already defined as regular dependency (no need to
regenerate requirements files)

Signed-off-by: Michal Šoltis <[email protected]>
@slimreaper35 slimreaper35 force-pushed the nox branch 6 times, most recently from 1ab601b to c7cb752 Compare December 9, 2024 10:08
@slimreaper35 slimreaper35 force-pushed the nox branch 2 times, most recently from 24c07b4 to d1b770f Compare December 9, 2024 10:59
@eskultety
Copy link
Member

This LGTM, it only needs the changes that Erik pointed out.

With these changes, the only reason to create a venv outside of .nox is to run the cachi2 command itself? I'm wondering if we need to keep the make venv command at all. It seems to me that the best approach is to install nox as a system-wide package, so we can probably move the creation of the cachi2 venv to nox.

How about just documenting it? ;) Having a shortcut command for python3 -m venv venv; venv/bin/pip install -r requirements.txt -r requirements-extras.txt seems suspicious, I was never a fan of that, but went with the flow since we had the Makefile and some Makefile targets depended on having a venv, otherwise I'd have deleted it already a while ago, but now we have a chance to get rid of any cruft we don't need and given nox/tox default to creating venvs for all jobs I say it's time to say goodbye to holding hands when it comes to venv creation, it's a standard mechanism anyway :) .


[project]
name = "cachi2"
license = {text = "GPLv3+"}
license = { text = "GPLv3+" }
Copy link
Member

Choose a reason for hiding this comment

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

unrelated change

"setuptools",
"setuptools-scm",
]
requires = ["setuptools", "setuptools-scm"]
Copy link
Member

Choose a reason for hiding this comment

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

unrelated change

dynamic = [
"version",
]
dynamic = ["version"]
Copy link
Member

Choose a reason for hiding this comment

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

unrelated change

@slimreaper35 slimreaper35 force-pushed the nox branch 5 times, most recently from 1a9abd9 to 26e883a Compare December 10, 2024 16:40
Nox is a command-line tool that automates testing in multiple Python
environments, similar to tox.

Unlike tox, nox uses a standard Python file for configuration
(noxfile.py). The file can be easily used as a replacement for tox.ini
and Makefile.

The patch adds configuration options for mypy (because nox will check
type annotations in noxfile.py as well). `nox.*` notation is a wildcard
pattern that matches all modules and submodules under the nox namespace
and `noxfile` notation matches only noxfile.py.

Signed-off-by: Michal Šoltis <[email protected]>
Since noxfile.py is fully integrated to GH actions, we can safely drop
both files.

Signed-off-by: Michal Šoltis <[email protected]>
The hints, how to create custom cachi2 image, are already well described
inside CONTRIBUTING.md document [1].

---
[1]: https://github.com/containerbuildsystem/cachi2/blob/main/CONTRIBUTING.md#running-integration-tests

Signed-off-by: Michal Šoltis <[email protected]>
Remove all mentions of `make` or `tox` commands that are no longer
relevant after removal of Makefile and tox.ini.

Signed-off-by: Michal Šoltis <[email protected]>
@slimreaper35 slimreaper35 added this pull request to the merge queue Dec 11, 2024
Merged via the queue into hermetoproject:main with commit 2967fdb Dec 11, 2024
15 checks passed
@slimreaper35 slimreaper35 deleted the nox branch December 11, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants