seL4 / microkit

Microkit - A simple operating system framework for the seL4 microkernel
Other
70 stars 37 forks source link

tool: adhere to CI style checks #67

Closed Ivan-Velickovic closed 5 months ago

Ivan-Velickovic commented 9 months ago

Ran seL4_tools/misc/style-all.sh on the whole repository, so hopefully this is the last PR of this kind we have to make.

Ivan-Velickovic commented 9 months ago

The style script managed to break the SDK, interesting.

Ivan-Velickovic commented 5 months ago

The style checker is now crashing, I am not sure why this is happening. The only thing I can think of is that it looks like the style checker uses Python 3.10 and Microkit uses Python 3.9.

lsf37 commented 5 months ago

It should be working fine with python 3.9, but I have seen the style checker crashing before on some code (long time ago, I don't remember the details).

Ivan-Velickovic commented 5 months ago

The version of autopep8 that the CI uses is older so maybe it's a bug that has been fixed by autopep8 by now. I installed the latest version of autopep8 and that doesn't crash locally. I'll test the version in the container to see if I can at least reproduce the Ci failure locally.

Ivan-Velickovic commented 5 months ago

I was able to reproduce it:

ivanv@in-container:/host$ autopep8 -i --max-line-length 100 tool/microkit/docs.md tool/microkit/elf.py tool/microkit/__init__.py tool/microkit/loader.py tool/microkit/__main__.py tool/microkit/sel4.py tool/microkit/sysxml.py tool/microkit/util.py
Traceback (most recent call last):
  File "/usr/local/bin/autopep8", line 8, in <module>
    sys.exit(main())
  File "/usr/local/lib/python3.9/dist-packages/autopep8.py", line 4161, in main
    ret = fix_multiple_files(args.files, args, sys.stdout)
  File "/usr/local/lib/python3.9/dist-packages/autopep8.py", line 4063, in fix_multiple_files
    ret = _fix_file((name, options, output))
  File "/usr/local/lib/python3.9/dist-packages/autopep8.py", line 4043, in _fix_file
    return fix_file(*parameters)
  File "/usr/local/lib/python3.9/dist-packages/autopep8.py", line 3423, in fix_file
    fixed_source = fix_lines(fixed_source, options, filename=filename)
  File "/usr/local/lib/python3.9/dist-packages/autopep8.py", line 3403, in fix_lines
    fixed_source = fix.fix()
  File "/usr/local/lib/python3.9/dist-packages/autopep8.py", line 587, in fix
    self._fix_source(filter_results(source=''.join(self.source),
  File "/usr/local/lib/python3.9/dist-packages/autopep8.py", line 531, in _fix_source
    modified_lines = fix(result)
  File "/usr/local/lib/python3.9/dist-packages/autopep8.py", line 717, in fix_e225
    pycodestyle.missing_whitespace_around_operator(fixed, ts))
AttributeError: module 'pycodestyle' has no attribute 'missing_whitespace_around_operator'
Ivan-Velickovic commented 5 months ago

Okay I used a newer version of autopep8 locally and pushed the changes it produced. Now the CI style check does not crash anymore but we have another problem which is that it wants to apply this diff:

diff --git a/tool/microkit/sysxml.py b/tool/microkit/sysxml.py
index 7bcf4ca..b231878 100644
--- a/tool/microkit/sysxml.py
+++ b/tool/microkit/sysxml.py
@@ -3,6 +3,7 @@
 #
 # SPDX-License-Identifier: BSD-2-Clause
 #
+import xml.etree.ElementTree as ET
 from microkit.util import str_to_bool, UserError
 from typing import Dict, Iterable, Optional, Set, Tuple
 from dataclasses import dataclass
@@ -11,7 +12,6 @@ from pathlib import Path
 # Force use of Python elementtree to avoid overloading
 import sys
 sys.modules['_elementtree'] = None  # type: ignore
-import xml.etree.ElementTree as ET

which will cause the tool to not compile/interpret because import xml.etree.ElementTree as ET is needed after the sys.modules['_elementtree'] = None # type: ignore line.

lsf37 commented 5 months ago

Does the newer version also want this diff or only the CI version? I could look into upgrading the CI version.

Ivan-Velickovic commented 5 months ago

Does the newer version also want this diff or only the CI version? I could look into upgrading the CI version.

I think it's some Python PEP8 rule probably. So different versions are also enforcing it. Not sure what to do since I'm not that familiar with this stuff.

Ivan-Velickovic commented 5 months ago

Sorted it out finally.