Opened 16 months ago

Last modified 13 months ago

#30404 closed enhancement

Add pyright and pycodestyle checks as Github actions — at Version 19

Reported by: gh-tobiasdiez Owned by:
Priority: major Milestone: sage-9.3
Component: build Keywords: code style, lint
Cc: saraedum, mkoeppe, chapoton, gh-mjungmath, embray, slelievre, dimpase Merged in:
Authors: Tobias Diez Reviewers:
Report Upstream: N/A Work issues: merge dependencies
Branch: public/build/lintGithub (Commits, GitHub, GitLab) Commit: 51828da28a18b1c0d161f09a91417abd9f652088
Dependencies: #30408, #30361 Stopgaps:

Status badges

Description (last modified by gh-tobiasdiez)

Currently, the linters (pycodestyle, pyflakes, ...) run as plugins of patchbot. This has a few disadvantages: a) the lint results depend on the version of the tools installed by the owner of the patchbot b) it takes quite some time until the results are available and c) you need people to run the patchbot.

This ticket adds pyright, relint and pycodestyle checks as Github actions. They run really fast (about 2min), have reliable results as the version of the linters can be controlled and uses Github server instead of people's machines.

Although only those rules are activated that are also run by the patchbot, the pycodestyle and relint checks currently fail with a lot of errors. See https://github.com/tobiasdiez/sage/actions/runs/352364668. Thus, making these tests pass requires more work by the sage community.

What is left to do is to add the integration of the github action output to trac. That can be done as follows:

  1. Sync trac branches to sagetrac-mirror (by post-commit hook)
  2. Activate github actions in the sagetrac-mirror github repo
  3. Add the following code to either the patchbot output or to the trac ticket header (next to the patchbot button)
    <a href="https://github.com/sagemath/sagetrac-mirror/actions?query=workflow%3ALint+branch%3BRANCH"><img src="https://github.com/sagemath/sagetrac-mirror/workflows/Lint/badge.svg?branch=BRANCH&event=push"/></a>
    

Here BRANCH needs to be replaced by the branch. Giving something similar to

https://github.com/tobiasdiez/sage/workflows/Lint/badge.svg

As this requires rights to the github repo and trac config, I cannot do this and have to leave it to somebody else.

Change History (19)

comment:1 Changed 16 months ago by saraedum

Option A: I think this is politically impossible. I know that some of the contributors here have (or at least had) strong feelings about github. That likely did not improve with Microsoft buying them.

Option B*: Every branch here is already mirrored on GitLab. I believe what's missing here is better docs: https://gitlab.com/sagemath/dev/trac

Option C*: You could add the linter to GitLab CI. If I remember embray's comments correctly it's really easy to write a trac plugin that shows the gitlab CI status here on trac.

comment:2 Changed 16 months ago by mkoeppe

Note there's also https://github.com/sagemath/sagetrac-mirror - but I don't know at what intervals it is mirrored.

comment:3 Changed 16 months ago by gh-tobiasdiez

Perfect @mkoeppe! In this case, the integration should be fairly straightforward and amounts to adding the adding a link to the badge, by

<a href="https://github.com/sagemath/sagetrac-mirror/actions?query=workflow%3ALint+branch%3BRANCH"><img src="https://github.com/sagemath/sagetrac-mirror/workflows/Lint/badge.svg?branch=BRANCH&event=push"/></a>

Giving something similar to

https://github.com/tobiasdiez/sage/workflows/Lint/badge.svg

with a link to https://github.com/tobiasdiez/sage/actions?query=workflow%3ALint+branch%3Apatch-2 (its sadly not possible to directly link to the workflow output)

This leaves the following todo list:

  • Restrict current github actions to master and develop branch
  • Activate gitub actions in sagetrac-mirror
  • Add the link to the badge to trac

I can do the first task. Could somebody else then do point 2 and 3?

comment:4 Changed 16 months ago by gh-tobiasdiez

  • Cc embray added

comment:5 Changed 16 months ago by mkoeppe

My suggestion is to first focus on local operation. For example, finding a place for the pycodestyle configuration in the Sage source tree (this should not be configured in .github/workflows/, nor in the patchbot sources!) -- perhaps this should go to src/tox.ini, and then provide corresponding documentation on how to run it and why. Next, change the patchbot so that it uses the configuration from that file. This will make sure that things don't fall out of sync.

comment:6 Changed 16 months ago by slelievre

  • Cc slelievre added
  • Keywords code style lint added
  • Type changed from PLEASE CHANGE to enhancement

comment:7 follow-up: Changed 16 months ago by gh-tobiasdiez

Moving the config to src/tox.ini is done in #30408.

Should I continue with the above plan and restrict the current github workflows to the master & develop branch?

comment:8 Changed 16 months ago by mkoeppe

Take a look at the other workflows from my portability tests. They are configured to run on every push to a tag and on every pull request. That's I think also the right granularity for the new workflow

comment:9 in reply to: ↑ 7 Changed 16 months ago by mkoeppe

Replying to gh-tobiasdiez:

Moving the config to src/tox.ini is done in #30408.

You could make #30408 a dependency of this ticket & merge in the branch

comment:10 Changed 16 months ago by gh-tobiasdiez

  • Branch set to public/build/lintGithub
  • Commit set to 2e88f614ec248e40ef76d76b61231f3c5f76af74
  • Dependencies set to #30408, #30361
  • Status changed from new to needs_review

I've now changed the workflow to use tox, which is enabled by #30408. Moreover, I realized based on the above comment by mkoeppe that the other Github actions are already restricted to only run on pushes with tags. Thus, they don't run on normal pushes to sagetrac-mirror, so no further action needed there.

They are configured to run on every push to a tag and on every pull request. That's I think also the right granularity for the new workflow

I think the lint workflow should run on every push and pull request. In this way, the developer gets immediate feedback, and this is similar to the patchbot that also runs on every "push". Moreover, they run very quickly so there is no problem in running them on every push.

So this leaves the following todo's.

  • Peer-review this ticket
  • Activate gitub actions in sagetrac-mirror
  • Merge the dependencies #30408, #30361
  • Double check that the workflow runs successful on sagetrac-mirror
  • Merge
  • Add the link to the badge to trac as described above

Afterwards, the workflow can be extended to include further linters. It might also be good to later add a basic build & doctest workflow that runs on every push.


New commits:

2e88f61Add lint workflow

comment:11 Changed 15 months ago by mkoeppe

  • Authors set to Tobias Diez
  • Status changed from needs_review to needs_work
  • Work issues set to merge dependencies

comment:12 Changed 15 months ago by gh-tobiasdiez

@mkoeppe, what should I do here? The dependencies are rather indirect (it works without #30361, it just tests more code). Moreover, this github action is currently nowhere invoked and hence not checked / tested....

comment:13 Changed 15 months ago by mkoeppe

Ticket description: "The pyright check currently fails, as there is no configuration file"

comment:14 Changed 15 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

Yes, but even with the configuration files added the github workflow fails as there is no typing information yet(and the one added in #29775 is not complete) and pycodestyle reports over 600 errors as well.

Fixing these points requires more effort from the whole sage community.

The aim of this ticket is to add the github actions so that one has a consistent check, which at some point in the future hopefully turns green.

Last edited 15 months ago by gh-tobiasdiez (previous) (diff)

comment:15 Changed 15 months ago by gh-tobiasdiez

  • Status changed from needs_review to needs_work

Should invoke relint as well (added in #30467).

comment:16 Changed 15 months ago by mkoeppe

Could just run all tox environments...

comment:17 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:18 Changed 13 months ago by git

  • Commit changed from 2e88f614ec248e40ef76d76b61231f3c5f76af74 to 51828da28a18b1c0d161f09a91417abd9f652088

Branch pushed to git repo; I updated commit sha1. New commits:

e3751deMerge branch 'develop' of git://github.com/sagemath/sage into public/build/lintGithub
51828daAdd relint

comment:19 Changed 13 months ago by gh-tobiasdiez

  • Description modified (diff)
  • Status changed from needs_work to needs_review

I've now added relint. This is now ready for review.

Note: See TracTickets for help on using tickets.