Skip to content
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.

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

tool: adhere to CI style checks #67

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

Ivan-Velickovic
Copy link
Collaborator

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.

@@ -1258,7 +1258,7 @@ def build_system(

final_cap_slot = cap_slot

## Minting in the address space
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will have to revert these kinds of changes since these comments are intentionally formatted as so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems that this is enforced and the style checker does not allow freedom in how you format comments so will have to keep these changes.

@Ivan-Velickovic
Copy link
Collaborator Author

The style script managed to break the SDK, interesting.

@Ivan-Velickovic
Copy link
Collaborator Author

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
Copy link
Member

lsf37 commented Jan 11, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Ivan-Velickovic commented Jan 11, 2024

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
Copy link
Member

lsf37 commented Jan 11, 2024

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

@Ivan-Velickovic
Copy link
Collaborator Author

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 Ivan-Velickovic force-pushed the tool_style_check branch 2 times, most recently from 738919e to 5bd1b8f Compare January 12, 2024 03:10
Signed-off-by: Ivan Velickovic <i.velickovic@unsw.edu.au>
@Ivan-Velickovic
Copy link
Collaborator Author

Sorted it out finally.

@Ivan-Velickovic Ivan-Velickovic merged commit ad271b5 into seL4:main Jan 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants