Skip to content

CP-47555 Porting usb_scan.py to python3 #5424

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Mar 13, 2024
Merged

CP-47555 Porting usb_scan.py to python3 #5424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5e29ccb
formate with black
acefei Jan 30, 2024
c16578d
decode value produced by python3-pyudev which return bytes
acefei Feb 4, 2024
780065b
remove usb_scan from expected_to_fail list in pyproject.toml
acefei Feb 4, 2024
6aef7b2
update some neccesary pylint issues
acefei Feb 4, 2024
d302501
fix pytype error: unsupported operand type(s) for +: str and UsbInter…
acefei Feb 4, 2024
57fffaa
Disable false positive in Pytype error reporting
acefei Feb 4, 2024
83b8e24
update for comments
acefei Feb 22, 2024
f645d83
fix ut errors as python-pyudev return bytes instead of string
acefei Feb 27, 2024
6e53d0a
increase code coverage
acefei Feb 27, 2024
3450536
format with black for test_usb_scan.py
acefei Mar 1, 2024
3aae700
move usb_scan.py with ut code into python3 folder
acefei Mar 1, 2024
86a772f
Disable the code coverage of scripts folder
acefei Mar 1, 2024
20064e1
false positive for code coverage
acefei Mar 1, 2024
1a34e20
solve the pylint warning
acefei Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ jobs:
if: ${{ matrix.python-version == '2.7' }}
run: >
pytest
--cov=scripts --cov=ocaml/xcp-rrdd
--cov=ocaml/xcp-rrdd
scripts/ ocaml/xcp-rrdd -vv -rA
--junitxml=.git/pytest${{matrix.python-version}}.xml
--cov-report term-missing
Expand Down
18 changes: 14 additions & 4 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,20 @@ profile = "black"
combine_as_imports = true
ensure_newline_before_comments = false

[tool.pylint.messages_control]
disable = [
"missing-function-docstring",
"missing-module-docstring",
"consider-using-f-string",
"too-many-branches",
"broad-exception-caught",
"no-else-break",
"no-else-return",
"invalid-name",
"import-error",
"unnecessary-pass",
"unspecified-encoding",
Copy link
Member

@psafont psafont Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@bernhardkaindl are these safe to disable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bernhardkaindl are these safe to disable?

May I have your further feedback to push this review orward?

Copy link
Contributor Author

@acefei acefei Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @psafont , as @bernhardkaindl 's previous perspective (#5296 (comment)), he thought those warnings are harmless and even far less of a fixed "problem" that would belong to a Python3 PR.
If enabling these rule, we have to add dozens more lines of changes, and even refactor the logic for some rules, all of which are risky.
Overall, it is fine to disable rules to keep code stable.

Copy link
Collaborator

@bernhardkaindl bernhardkaindl Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

"unspecified-encoding" is not safe to ignore blindly for all files.

The reason is that files can contain UTF-8 characters or even non UTF-8 bytes > ord(127), that are not valid UTF-8 and this caused crashing the auto-cert-kit Xapi plugin last year:

LC_ALL=C python3.6 -c 'open("/usr/share/hwdata/pci.ids").read()'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib64/python3.6/encodings/ascii.py", line 26, in decode
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 90372: ordinal not in range(128)

See my review comment #5424 (review) for the further follow-up on this discussion.

Copy link
Member

Choose a reason for hiding this comment

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

If enabling these rule, we have to add dozens more lines of changes, and even refactor the logic for some rules, all of which are risky.

Hi @acefei Would you please create a new ticket to track this kind of changes? It would be a good chance to make these potential issues more clear in feature branch.

]

[tool.mypy]
# Note mypy has no config setting for PYTHONPATH, so you need to call it with:
Expand Down Expand Up @@ -85,10 +99,6 @@ expected_to_fail = [
"scripts/examples/python/shell.py",
"scripts/examples/smapiv2.py",
"scripts/static-vdis",
# add_interface: unsupported operand type(s) for +: str and UsbInterface
"scripts/usb_scan.py",
# TestUsbScan.assertIn() is called with wrong arguments(code not iterable)
"scripts/test_usb_scan.py",
"scripts/plugins/extauth-hook-AD.py",
]

Expand Down
2 changes: 2 additions & 0 deletions python3/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ SITE3_DIR=$(shell python3 -c "from distutils.sysconfig import get_python_lib; pr
install:
mkdir -p $(DESTDIR)$(OPTDIR)/bin
mkdir -p $(DESTDIR)$(SITE3_DIR)
mkdir -p $(DESTDIR)$(LIBEXECDIR)

$(IPROG) bin/hfx_filename $(DESTDIR)$(OPTDIR)/bin
$(IDATA) packages/observer.py $(DESTDIR)$(SITE3_DIR)/
$(IPROG) libexec/usb_scan.py $(DESTDIR)$(LIBEXECDIR)
Loading