Page MenuHomePhabricator

[lint] relax the PythonShebangLinter and apply it on electrum
ClosedPublic

Authored by PiRK on Feb 14 2024, 10:29.

Details

Summary

We currently have inconsistencies regarding the linting of python shebangs and file permissions:

  • the linter wants all .py files to have a shebang, but is not applied on electrum code
  • all of the functional test files have a shebang but the file permissions vary (some 644, some 755)

The plan is to introduce a new linter that will force all executable files to have a shebang, and all non executable files to not have a shebang.
We also want to make most python file non-executable (so without a shebang), except for the actual scripts.

This diff is an intermediate step that relaxes the current linter to enforce shebangs only on executable files. It also incidentally adds a check for consistency within the executable flags, as the future linter will only allow 644 and 755 permissions.

In a following scripted diff I will remove the shebang from non script python files and set their permission to 644, and enforce it with the existing linter. Then we can transition to the new linter.

Test Plan

arc lint --everything

Mess up some files and check that arc lint catches them (chmod 744 file.py, remove a shebang from an executable file)

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Feb 14 2024, 10:29
electrum/electrumabc/tests/test_cashaddrenc.py
1 ↗(On Diff #45221)

that now causes a linter error because it is not the right shebang (/usr/bin/env python3)

Fabien requested changes to this revision.Feb 14 2024, 13:34
Fabien added a subscriber: Fabien.
Fabien added inline comments.
arcanist/linter/PythonShebangLinter.php
58 ↗(On Diff #45221)

shouldn't be python(3)? ? so that it works with python as well (python2 is obsolete for quite some time now and most distro default to python 3)

58–59 ↗(On Diff #45221)

revert the test order, so you skip the expensive regex is the flag is false

This revision now requires changes to proceed.Feb 14 2024, 13:34

revert the test order to skip the expensive operation on most files

arcanist/linter/PythonShebangLinter.php
58 ↗(On Diff #45221)

For now python is always available as python3 on all distros, AFAIK. It is sometimes also available as python, but this requires extra steps on some distros (e.g. apt install python-is-python3 on some Ubuntu versions).
I think sticking with python3 is a bit safer, but I'm happy to relax this rule as well if you like.

This revision is now accepted and ready to land.Feb 14 2024, 15:01
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
This revision was landed with ongoing or failed builds.Feb 14 2024, 16:18
This revision was automatically updated to reflect the committed changes.