Made https://reviews.bitcoinabc.org/D20767 for the abstract class changes, I don't think the diff part is needed at all anymore, so i'll close this diff.

use uppercase linter names in abstract class

remove leftover useless function

remove unnecessary rules in ruff check arcanist linter code

fix typo

In D20675#471765, @PiRK wrote:

This is good work, and the refactor into a base class makes sense. I would just recommend splitting this work into two or 3 smaller diffs so the non-controversial changes can get landed quickly and there is a separate rationale for every piece of the work:

• extract rules to .ruff.toml, minimzing changes to the the php (some are necesarry, D20693), while avoiding any potential change in behavior (same command line args)
• extract the common stuff to AbstractRuffExternalLinter, pure refactor with no change of behavior
• change command line args, --diff etc... (for this part the rationale is still not clear to me)

@Fabien @PiRK Should be good now, there's autofix and arcanist still shows the diff of the things it autofixed. And I also took the liberty to make an abstract class for ruff executable and output parsing to avoid duplicating code in both ruff format and ruff check linters.

Remove reference to pinterest repository

run arc liberate

Just a rebase on master

Restore diff output in arcanist (with autofix still enabled) and move ruff linters common code to an AbstractRuffExternalLinter

In D20675#470704, @Fabien wrote:

I ran the linter on all files, both on master and on this patch. On both I modified the .arclint file to only keep the generated, ruff-format and ruff-check linters. My governor is in eco mode to reduce the imapct of the CPU boost.
I noticed 3 files from electrum/scripts have autofixes with this patch (but not on master, so the rules might be different). I excluded the electrum/script/*.py files from the run with this patch so it doesn't interfere with the measurement.

On master:

$ time arc lint --everything
 OKAY  No lint messages.

real	0m1,096s
user	0m2,249s
sys	0m3,068s

On arcpatch-D20675:

$ time arc lint --everything
 OKAY  No lint messages.

real	0m8,509s
user	0m11,085s
sys	1m27,875s

So it's roughly 8,5x slower to exclude the files from ruff instead of arcanist on my machine, and much worst on machines with less CPU cores as you can see from the sys measurement.
Let's keep the exclusion in arcanist.

@PiRK @Fabien It should be good now, I have re-added autofix to both linters.

Both linters should fix all issue they find when using arc lint, there are no detailed message shown except that there were code-style violations on a file.
Here's an example with a random file:

λ arc lint
>>> Lint for electrum/electrum-abc:

Update ruff linters to fix issues whenever they can and return an error message to arc lint, when an issue has been fixed or found (since not all issues can be fixe automatically).

In D20675#470343, @PiRK wrote:

I think the current behavior of the tool is good, but that's mostly because I'm used to it. If you see a way to improve it, go ahead. I think the goal should be to maximize automation and minimize decision making by devs.

In D20675#470311, @PiRK wrote:

In D20675#470301, @Greesb wrote:

It seems like it works, but (with my version at least) there's no diff to be displayed, is that ok for you to just show that the file was reformatted without asking or showing what was modified ?

Because from my understanding with those linters they can either:

• Show the diff that the linter wants to modify to comply with the style requirements and let the dev chose to apply it or not
• Fix the lint issues and exit with a fail
• Fix the lint issues and proceed with the commit/diff automatically

I think I also got lost in all those possible options while also trying to understand how the arcanist linters works 😅

For some reason the diff is not displayed if you remove setLine(1)->setChar(1) so you can also just restore these. Then another issue I find is that if both ruff format and ruff check find an issue with an auto-fix, only one of them is actually applied, so you have to run arc lint twice. But that's already a problem on master. I don't think there is a workaround for this, it doesn't seem possible to properly chain 2 automatic reformattings in a single pass of arc lint. Switching from having multiple python linters (isort, flake8....) to just ruff check improved that situation quite a lot already.

Try something like this: https://reviews.bitcoinabc.org/D20693 (based on your work with some of the php changes reverted)

In D20675#470252, @PiRK wrote:

The main issue is that the files are not passed via their path but via stdin, so the excludes from toml file are not respected.
I think you should be able to fix this in RuffFormatLinter the same way I had to do for RuffCheckLinter in D18906, with --stdin-filename:

diff --git a/arcanist/linter/RuffFormatLinter.php b/arcanist/linter/RuffFormatLinter.php
index 7ca9fceb96..fe3aa62e6c 100644
--- a/arcanist/linter/RuffFormatLinter.php
+++ b/arcanist/linter/RuffFormatLinter.php
@@ -36,11 +36,15 @@ final class RuffFormatLinter extends ArcanistExternalLinter {
   protected function buildFutures(array $paths) {
     $executable = $this->getExecutableCommand();

-    $bin = csprintf('%C %Ls -', $executable, $this->getCommandFlags());
-
     $futures = array();
     foreach ($paths as $path) {
       $disk_path = $this->getEngine()->getFilePathOnDisk($path);
+      $bin = csprintf(
+        '%C %Ls  --stdin-filename %s -',
+         $executable,
+         $this->getCommandFlags(),
+         $disk_path
+      );
       $future = new ExecFuture('%C', $bin);
       /* Write the input file to stdin */
       $input = file_get_contents($disk_path);

I did a few quick tests, and it seems to work. I wasn't able to dedup the include, though (even by setting --force-exclude directly on the command line). It seems you are correct to keep them both in the toml and the .arclint file.

In D20675#470204, @PiRK wrote:

I'm also not a big fan of losing the autofix. I'm used to my arc lint command fixing (almost) all the code in a single pass.
Moving all the config to a standard .ruff.toml file is a big improvement on the other hand (compatibility with IDEs)

In D20675#470199, @Fabien wrote:

I'm not sure about all of these changes. What I like:

• ruff running on the whole repo and not only electrum abc

It was already the case given the previous rules in the arcanist linters, just the ruff rules are put in a common file for the whole codebase to share.

In D20669#469779, @PiRK wrote:

I'm OK with the idea of formatting python code blocks in md files (if not in third-party dirs). I didn't know ruff-format could do that.

In D20669#469764, @PiRK wrote:

Running Ruff manually is not the way to go. If you think some code should be formatted, make sure it is included in .arclint under "ruff-format". We explicitely exclude most of these files there.

ignore src/secp256k1/tools/tests_wycheproof_generate.py in ruff-format

This revision was landed with ongoing or failed builds.

Remove unnecessary change for upgrade

Need to do some more tests in local before.

In D20631#468994, @Fabien wrote:

The actual changes looks good but some pieces are missing:

• Don't skip the linter, especially in a linter change
• You missed the .arclint file which needs to be updated accordingly
• You should lint the files in the same diff. If you want to speed up the operation you can temporary remove all linters but generated and clang-format from .arclint then run arc lint --everything. Doing so will avoid having the repo in a broken state with files that don't comply with the linter.

Fix .arclint and run arc lint --everything for generated and clang-format linters

In D20631#468994, @Fabien wrote:

The actual changes looks good but some pieces are missing:

• Don't skip the linter, especially in a linter change
• You missed the .arclint file which needs to be updated accordingly
• You should lint the files in the same diff. If you want to speed up the operation you can temporary remove all linters but generated and clang-format from .arclint then run arc lint --everything. Doing so will avoid having the repo in a broken state with files that don't comply with the linter.

Enforcing clang-format-19 is fine though, it's available on most distros.

Last typo

Guess what, a typo.

Typo again

Typo

Restrict clang-format to 19 instead of >=16

rebased on master and resolved electrum/CONTRIBUTING.md conflicts

In D20358#461786, @PiRK wrote:

I'm fine with pyqt5>=5.15.11 as long as we don't run into issues when doing the next release (we release with python 3.11)

I can take care of bumping the pyqt5 packages accordingly in contrib/deterministic-build/requirements-binaries.txt, unless you want to do it. The test plan will involve building the release (contrib/build-wine/build.sh, contrib/build-linux/appimage/build.sh) and testing it.

@PiRK
Should it still require pyqt5>=5.15.11 to fix this issue with python 3.12+ though ?
Having pyqt5>=5.15.11 would still allow all python versions to work and would also allow to remove a lot of the compatibility code i saw for older pyqt5 versions

Having pyqt5>=5.15.11 would still allow all python versions to work and would probably allow to remove a lot of the compatibility code i saw for older pyqt5 versions

In D20358#461743, @PiRK wrote:

I think just removing the upper limit (<=5.15.9) would be enough, pip would automatically pick the newest version when installing. And it would reduce the risks of breaking older versions of python, because i'm not sure if PyQt5==5.15.11 still support python 3.9 or 3.10.

The upper limit was introduced when I backported https://github.com/Electron-Cash/Electron-Cash/pull/2558. No rationale was given for this limit. So it's probably safe to just remove.

removed icons.py changes

In D20358#461736, @PiRK wrote:

you accidentally commited a change in icon.py, please reset it.

I need to figure out why we previously pinned pyqt5 to < 5.15.9

Not sure if there is anything else I need to do for this dependency version bump, if there is then please do let me know.

Changed the python restriction to upgrading the required pyqt5 version

Even the latest available version of pyqt5 seems to be enough to fix the crash actually (5.15.11), will do more test to validate and will change my diff when done.

The issue doesn't appear with PyQt6, and works also in 3.9.25, 3.10.20, 3.11.15, 3.12.13, 3.13.14, 3.14.6 (only tested the unit tests and just launching electrum-abc binary for now).
Would switching to PyQt6 be an acceptable fix ? Or is PyQt5 mandatory ? @Fabien

In D20358#461442, @Fabien wrote:

If there is a bug that crash the tests with python >= 3.12, then the bug should be fixed. Disabling python >= 3.12 is just hiding the bug and this is not the right approach.

The diff summary explains how to access that stack trace but not what the content is and if you're already investigated the root cause, can you please elaborate?

I forgot to mention that the stack trace issue seems to be coming from PyQt. I'll see if I can find time to investigate the issue.

I had to skip linters because arc diff required that the version of clang was <17 even though no clang-related changes were made; and I still haven't managed to install clang<17 or clang-format<17