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

git-sync-deps seems to no longer require or support Python 2 #245

Merged
merged 3 commits into from
Apr 30, 2024

Conversation

pavpanchekha
Copy link
Contributor

This PR removes the mention of Python 2 from the macOS build instructions.

I just ran through the installation instructions on macOS and tools/git-sync-deps in the Skia build instructions no longer need or allow Python 2. I am guessing the Windows and Linux build instructions also need to be updated; that said, I'm a bit worried to propose those because I haven't tested on those operating systems.

@HinTak
Copy link
Collaborator

HinTak commented Apr 29, 2024

You are correct - skia's git-sync-deps these days requires python 3
and we updated the build-script 9 months ago to explicitly use python 3:
0da5cfd

I think "python" on CI mac runner is still python 2, (and python 2 is the default on older macs) so your update is not quite safe... it is probably best to remove those, and refer the users to scripts/build_* for build instructions, since that's how the pypi wheels are built.

@HinTak
Copy link
Collaborator

HinTak commented Apr 29, 2024

At a glance, it is quite outdated... scripts/build*.sh is current. Besides python 2 vs python3, skia v116+ requires c++ 17 instead of c++ 14 now. Ie. Needs a recent c++ compiler.

@pavpanchekha
Copy link
Contributor Author

Feel free to close if the actual plan is to get rid of that page instead.

@pavpanchekha
Copy link
Contributor Author

Or recommend the build scripts. I do believe I tried those and it didn't work, but that might have been a red herring.

@HinTak
Copy link
Collaborator

HinTak commented Apr 29, 2024

It is probably a bit more complicated than that - I assume you have an up-to-date mac os, for which python is python 3 (while python2 exists?). In somewhat older mac, python2 and python3 both exists, and python is python2. Apple is behind Linux and window's WSL by a few years on switching default python to python3.

I'd remove/update the build instructions and put more emphasis on the looking at the build scripts - which we use to build on somewhat old but still currently supported systems for max backwards compatibility. So they are the standard, but if you have a very up to date system, you might need to adapt.

@HinTak
Copy link
Collaborator

HinTak commented Apr 29, 2024

I think your pull is fine other than the python2 to python line - perhaps python3? Is "python3" available on your system? It sounds as if it isn't?

@pavpanchekha
Copy link
Contributor Author

python3 is available on my system. I just re-built using the scripts and they seem to work; it was probably something unrelated.

Use python 3 for Windows and Linux, and requires c++17.
@HinTak
Copy link
Collaborator

HinTak commented Apr 30, 2024

I have added some other python 2 to 3 changes, and a c++ 17 change, to the doc, and also cherry-pick a commit from #236 which fixes intermittent ci failures on windows, to let this finish ci, before merging.

@HinTak HinTak merged commit 59f98a8 into kyamagu:main Apr 30, 2024
14 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