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

Remove tox from CI #462

Merged
merged 4 commits into from
Dec 19, 2022
Merged

Remove tox from CI #462

merged 4 commits into from
Dec 19, 2022

Conversation

dvarrazzo
Copy link
Member

I have realised that tox gets in the way of writing an efficient and succinct ci.

Dropping it would allow to have a single test step. Using a simple bash wrapper to retry the tests would be much simpler than using the corresponding tox idiom. The features offered by tox (env isolation) is of no use for local development (virtualenv is fine) and ci execution (runners are isolated).

Copy link
Contributor

@dlax dlax left a comment

Choose a reason for hiding this comment

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

I'm a bit sad about this because I, as a casual contributor (in general, not so casual for this project) I find it very comfortable to find tox as an entry point for setting up a development environment. I also like the pattern of keeping the CI configuration simple by relying on tox.

Just to be clear, I don't run tox either on projects I work on regularly (I just run pytest or mypy as you do, and other linters are in $EDITOR); this applies to psycopg for me.

But if I put myself in me-as-a-casual-contributor's place, I would be annoyed to have to decipher those large YAML files to know how to get a working dev env.

Again, for me, in this project, this does not matter much so I can live with this change. Just wanted to get another perspective.

@@ -19,32 +19,37 @@ jobs:

linux: # {{{
runs-on: ubuntu-latest
if: true
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this (and other similar below) for?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is because, when I want to run tests only a subset of the platforms (for instance debugging a windows problem), I wish to disable the other platform running. It can be done adding if: false to the other platforms... but only after I make the regular mistake of adding when: false and being scolded by github 😄

So it's a reminder about the syntax to use to disable a branch.

@@ -0,0 +1,6 @@
[flake8]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file really needed? It seems redundant with the one at root level.

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 have to double-check if they are still both needed. Until recently (where this config was in the tox.ini files), the top-level flake8 file was taken into account when running flake8 from the root of the project, to check all the code, but, when the vim plugin checks the syntax (using flake8 /absolute/path/to/file.py I assume), I understand that flake8 stops at the first setup.py or similar file it finds, which in most projects identify the root of the project, but not in this "monorepo".

I hate the duplication too. I'll check if I can drop it.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the top file is dropped, flake8 gets lost in the .venv directories. It doesn't have the smarts to check in the .gitignore.

$ flake8  # finishes quickly
$ rm .flake8 
$ flake8  # several seconds of inactivity
^C... stopped while processing files

$ flake8 -v
flake8.checker            MainProcess     46 INFO     Making checkers
^Cflake8.checker            MainProcess  28457 WARNING  Flake8 was interrupted by the user
... stopped while processing files

so that's kinda needed. Trying to drop the directory ones, vim doesn't pick up on the config:

image

I use vim with ALE. Using :ALEInfo it shows that the command line executed is:

['/bin/bash', '-c', 'cd ''/home/piro/dev/psycopg3/psycopg'' && ''/home/piro/dev/psycopg3/.venv/bin/flake8''
--format=default --stdin-display-name ''/home/piro/dev/psycopg3/psycopg/psycopg/errors.py'' - < ''/tmp/vOedQyL/4248/errors.py''']

the problem with this is the cd to the psycopg sub-directory of the project instead of the root:

(.venv) piro@sherekhan:~/dev/psycopg3$ flake8 /home/piro/dev/psycopg3/psycopg/psycopg/errors.py
# finds no problem

(.venv) piro@sherekhan:~/dev/psycopg3$ cd psycopg
/home/piro/dev/psycopg3/psycopg
(.venv) piro@sherekhan:~/dev/psycopg3/psycopg$ flake8 /home/piro/dev/psycopg3/psycopg/psycopg/errors.py 
# finds the problem

I'll check if I can configure ALE better...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ale, as I understood, takes the psycopg directory as the root as it looks one of these files. There seem to be a way to customize it using ale_root but I'm not so much an expert of vim so it will take some time to figure out how to write a function e.g. to use .git as the project root.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed I also have trouble within vim when the inner file is dropped. That's for the investigation.
But maybe the file could be a symlink?

.github/workflows/lint.yml Outdated Show resolved Hide resolved
No idea why ctypes doesn't work, and I really don't care.
They were needed to deal with problem caused by tox. Not needed anymore.
@dvarrazzo
Copy link
Member Author

Thank you for your feedback, @dlax! Of course you are much more than an occasional contributor. Trying to step in the shoes of one such contributor, I think the best favour we may make them is to provide a simple README to start from.

I don't expect people to dig into the .github files, I don't think that's the right entry point. However, tox wasn't particularly representative about how to run tests. The test stanza was:

    -python -bb -m pytest {posargs}
    -python -bb -m pytest --lf --lfnf=none --no-collect-ok --randomly-seed=last {posargs}
    python -bb -m pytest --lf --lfnf=none --no-collect-ok --randomly-seed=last {posargs}

which has a lot to explain, whereas, on desktop, all you need to do for testing is more similar to

pip install ./psycopg[test]  # once
pytest

the isolated tox environment was causing problems I could never solve, such as why multiprocess tests were failing using the C extension, and it's fundamentally redundant given the throwaway nature of current ci runners.

@dlax
Copy link
Contributor

dlax commented Dec 19, 2022 via email

@dvarrazzo dvarrazzo merged commit aff1b3c into master Dec 19, 2022
@dvarrazzo dvarrazzo deleted the detox branch December 19, 2022 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants