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

build: use python3 to lint #33627

Merged
merged 4 commits into from
Apr 11, 2022
Merged

build: use python3 to lint #33627

merged 4 commits into from
Apr 11, 2022

Conversation

nornagon
Copy link
Member

@nornagon nornagon commented Apr 5, 2022

Description of Change

python no longer exists on macOS Monterey.

Notes: none

@miniak
Copy link
Contributor

miniak commented Apr 6, 2022

python3 ./script/check-relative-doc-links.py

Traceback (most recent call last):
  File "./script/check-relative-doc-links.py", line 130, in <module>
    sys.exit(main())
  File "./script/check-relative-doc-links.py", line 30, in main
    totalBrokenLinks += getBrokenLinks(path)
  File "./script/check-relative-doc-links.py", line 81, in getBrokenLinks
    newLines = newFile.readlines()
  File "/usr/lib/python3.6/encodings/ascii.py", line 26, in decode
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 1927: ordinal not in range(128)

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

I know my stamp has no effect here, but throwing a request changes on this.

I think if we should be more deliberate if we're going to make this change and choose to cut over to Python 3 entirely, rather than doing a change like this. Especially since a change like this affects pre-commit hooks and the likes.

  • Our documentation lists Python 2 as a prerequisite
  • Windows CI scripts currently install Python 2
  • build-tools uses Python 2 for Windows
  • Not all OSs we support for building have a python3 executable
    • As far as I can tell, macOS Mojave (10.14) and earlier don't come with Python 3, and the build instructions list support for >= 10.11.6
    • If you use the official Python 3 installer on Windows, you won't get a python3 executable, that's not a naming convention they implement there. However, python3 is a stub on Windows 10 which will take you to the Windows Store to install it via the store - yea, it's a headache

I'm not opposed to switching to officially switching over to Python 3 if we determine we can do so. However, this change feels like it will potentially cause problems on other OSs just to fix macOS Monterey.

@nornagon
Copy link
Member Author

nornagon commented Apr 6, 2022

Python 2 has been officially unsupported since Jan 1 2020. I'm not interested in spending any effort supporting it.

However, we should (a) make sure our scripts actually work in Python 3 and (b) continue to function on Windows, those are important.

@nornagon
Copy link
Member Author

nornagon commented Apr 6, 2022

I've removed some outdated Python 2 information from our docs, and also dropped some no-longer-current information about required macOS and SDK versions, as those now shift regularly as we track Chromium.

@jkleinsc
Copy link
Contributor

jkleinsc commented Apr 8, 2022

FWIW... in working on the latest chromium roll, I came across this change to depot_tools:
3525960: Explicitly run everything with python3 | https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3525960.

@nornagon
Copy link
Member Author

Alright, given that change upstream, I'm going to merge this. If depot_tools expects a python3 to exist everywhere, then so can we.

@nornagon nornagon merged commit c9fd255 into main Apr 11, 2022
@nornagon nornagon deleted the py3 branch April 11, 2022 23:05
@release-clerk
Copy link

release-clerk bot commented Apr 11, 2022

No Release Notes

@trop
Copy link
Contributor

trop bot commented Apr 11, 2022

I was unable to backport this PR to "15-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Apr 11, 2022

I have automatically backported this PR to "16-x-y", please check out #33716

@trop
Copy link
Contributor

trop bot commented Apr 11, 2022

I have automatically backported this PR to "17-x-y", please check out #33717

@trop
Copy link
Contributor

trop bot commented Apr 11, 2022

I have automatically backported this PR to "19-x-y", please check out #33718

@trop
Copy link
Contributor

trop bot commented Apr 11, 2022

I have automatically backported this PR to "18-x-y", please check out #33719

@nornagon
Copy link
Member Author

Followup: #33720

bavulapati pushed a commit to bavulapati/electron that referenced this pull request Apr 29, 2022
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants