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

python style check fails with internal error #329

Open
axel-h opened this issue Feb 1, 2024 · 5 comments
Open

python style check fails with internal error #329

axel-h opened this issue Feb 1, 2024 · 5 comments
Labels
dependencies Pull requests that update a dependency file

Comments

@axel-h
Copy link
Member

axel-h commented Feb 1, 2024

When seL4/ci-actions/style@master runs, it fails with this internal error. Which may indicate there is a style issue in my code, but it seem the checker itself runs into a version problem:

`b'Traceback (most recent call last):\n  File "/home/runner/.local/bin/autopep8", line 8, in <module>\n    sys.exit(main())\n  File "/home/runner/.local/lib/python3.10/site-packages/autopep8.py", line 4161, in main\n    ret = fix_multiple_files(args.files, args, sys.stdout)\n  File "/home/runner/.local/lib/python3.10/site-packages/autopep8.py", line 4063, in fix_multiple_files\n    ret = _fix_file((name, options, output))\n  File "/home/runner/.local/lib/python3.10/site-packages/autopep8.py", line 4043, in _fix_file\n    return fix_file(*parameters)\n  File "/home/runner/.local/lib/python3.10/site-packages/autopep8.py", line 3423, in fix_file\n    fixed_source = fix_lines(fixed_source, options, filename=filename)\n  File "/home/runner/.local/lib/python3.10/site-packages/autopep8.py", line 3403, in fix_lines\n    fixed_source = fix.fix()\n  File "/home/runner/.local/lib/python3.10/site-packages/autopep8.py", line 587, in fix\n    self._fix_source(filter_results(source=\'\'.join(self.source),\n  File "/home/runner/.local/lib/python3.10/site-packages/autopep8.py", line 531, in _fix_source\n    modified_lines = fix(result)\n  File "/home/runner/.local/lib/python3.10/site-packages/autopep8.py", line 717, in fix_e225\n    pycodestyle.missing_whitespace_around_operator(fixed, ts))\nAttributeError: module \'pycodestyle\' has no attribute \'missing_whitespace_around_operator\'. Did you mean: \'whitespace_around_operator\'?\n'

It seem this issue is known, see hhatto/autopep8#689. But now I am a bit lost how to solve this. Can we fix this in scripts/install-python.sh and force a specific version or do we need python 3.11?

@axel-h axel-h added the dependencies Pull requests that update a dependency file label Feb 1, 2024
@lsf37
Copy link
Member

lsf37 commented Feb 1, 2024

It looks like what we need is autopep8 v2.0.4 (which is currently the latest). The package sel4-deps in the seL4 repo sets autopep8==1.4.3.

Not sure if there will be flow-on effects from that, but we could upgrade that.

@lsf37
Copy link
Member

lsf37 commented Feb 1, 2024

The same package also pins cmake-format, which from time to time was showing stupid formatting. Could try to upgrade that too, but all of this may lead to style failures. Hopefully only at the points that previously were stupid, but we should probably at least try to check if those versions do something fundamentally different.

@lsf37
Copy link
Member

lsf37 commented Feb 1, 2024

So, playing around with it a bit locally, the latest version of autopep8 does upgrade one dependency which doesn't seem to be a problem, and it does have impact on a small number of our python files, but everything I have seen are actual fixes that it should have done that way in the first place.

So I'd be up for bumping that version.

Still need to investigate cmake-format.

@Ivan-Velickovic I remember you having trouble with autopep8 crashing -- would that potentially be alleviated by bumping the style checks to the latest version? Or do you expect more trouble from that?

@Ivan-Velickovic
Copy link
Contributor

I can't remember exactly but based on this seL4/microkit#67 (comment) comment I think it was fine. Using the newer version didn't (at least for that code) do a bunch of incompatible/extra styling so at least for my purposes upgrading the version shouldn't cause trouble.

@lsf37
Copy link
Member

lsf37 commented Feb 4, 2024

Result of playing with cmake-format versions: their output differs fairly strongly. If we decide to upgrade, that should definitely be a separate discussion and needs to be weighed carefully against the noise it is going to produce in commits. The latest version is 0.6.13. Development seems to have slowed down, at least there hasn't been a new version since 2021 (and plenty before). That version has slightly different config options with more customisability, so it might be possible to get something nicer for those cases that are currently annoying, but at least in < 15min I wasn't able to find a combination that produces only small diffs to what we have now.

So: separate discussion for cmake-format, and if nobody objects, I would upgrade autopep8 to the latest version which fixes some crashes and seems mostly layout-stable. Pinging @seL4/tsc in case people want to weigh in on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

No branches or pull requests

3 participants