game-ci / docker

Series of CI-specialised docker images for Unity.
https://hub.docker.com/u/unityci
MIT License
392 stars 122 forks source link

feat: [BC] Python 2 for all target platforms #160

Closed webbertakken closed 2 years ago

webbertakken commented 2 years ago

Decision

As core maintainers we've considered all options and we've decided to go with the following solution:

Changes in this PR

Allows using Firebase with all images, by enabling the python command, referring to python3.

Fixes: https://github.com/game-ci/docker/issues/100

Rationale

Status quo
Discussion points
Our options
Sources used

Checklist

github-actions[bot] commented 2 years ago

Cat Gif

davidmfinol commented 2 years ago

There is this error: Unable to locate package python-is-python3

I guess apt-get doesn't have python-is-python3?

GabLeRoux commented 2 years ago

This is also an interesting read:
https://askubuntu.com/questions/1296790/python-is-python3-package-in-ubuntu-20-04-what-is-it-and-what-does-it-actually

the package only does a symlink so we can do this by hand instead. I’m sharing this so we don’t forget about pip which I think should also default to pip3

webbertakken commented 2 years ago

Yes indeed. It looks like the package was never created for 18.04, as can be seen on the packages page.

I'll update with a symbolic link and do the same for pip/pip3.

webbertakken commented 2 years ago

I'm a bit confused after this though. Wasn't the whole point that python did not exist?

#6 [4/7] RUN ln -s /usr/bin/python3 /usr/bin/python  && ln -s /usr/bin/pip3 /usr/bin/pip
#6 0.220 ln: failed to create symbolic link '/usr/bin/python': File exists
#6 ERROR: executor failed running [/bin/sh -c ln -s /usr/bin/python3 /usr/bin/python  && ln -s /usr/bin/pip3 /usr/bin/pip]: exit code: 1
webbertakken commented 2 years ago

So should we just install python as python3 for all images? What do you think. Would it introduce breaking changes?

I don't use python often enough to know the conventions very well. Please advise.

davidmfinol commented 2 years ago

I feel like the transition from python 2 to python 3 has been slow and painful, but I think pointing python to python 3 should work fine in most if not all scenarios now.

webbertakken commented 2 years ago

I feel like the transition from python 2 to python 3 has been slow and painful, but I think pointing python to python 3 should work fine in most if not all scenarios now.

Maybe we should make this the 1.0.0 release then. Because all actions are currently rolling 0.x.x images automatically and this could be a BC for many people potentially.

davidmfinol commented 2 years ago

Looks like the WebGL builds still expect python 2, so those are failing. We could simply install python 2, and we may reconsider switching to python 3 if Unity updates the version of python that WebGL builds use.

Even so, I think moving to 1.0.0 after adding python to the base image is a good idea.

webbertakken commented 2 years ago

The problem is that we use both python2 (for webgl) and python3 (for android and iOS).

Those were all added in individual fixes. I think if python3 is indeed already used in some images it might be worth not installing python2 to more target platforms.

Does anyone have the right documentation for these Firebase SDKs that are used in games?

Perhaps we should go for a per-platform upgrade, or install python3 but not link it to python?

davidmfinol commented 2 years ago

I haven't been able to find confirmation, but it seems to me that Firebase (for iOS and Android) expects python, but that can be either Python 2 or Python 3.

@dharmeshmp @mastef Maybe one of you can confirm?

If so, I think we should be OK to merge this PR and close the corresponding issue.

webbertakken commented 2 years ago

I haven't been able to find confirmation, but it seems to me that Firebase (for iOS and Android) expects python, but that can be either Python 2 or Python 3.

@dharmeshmp @mastef Maybe one of you can confirm?

If so, I think we should be OK to merge this PR and close the corresponding issue.

That's also what I'm thinking. Except I don't think that we should go from python 3 to python 2. Which is what it would mean for iOS and Android if we merge it in the current state.

What about if we just install both (python2 and python3) and link python to python3 for all platforms, then mark it as a breaking change and call it v1.0.0? Or even remove python2 in the process? If we're moving to 1.0 it would be perfect to introduce that change now.

davidmfinol commented 2 years ago

link python to python3 for all platforms

WebGL builds expect that python is python 2, so this would break all WebGL builds. Maybe we could link python to python3 for all platforms except WebGL?

webbertakken commented 2 years ago

Yea I'm in favour of that too.

mastef commented 2 years ago

Not a necessity. The python = python 3 hack can be removed in favor of just having python 2 installed

AFAIK apt install python will install python 2, no?

I would agree with the code in this PR. Am I missing something?

webbertakken commented 2 years ago

Not a necessity. The python = python 3 hack can be removed in favor of just having python 2 installed

I'm not sure if I get your meaning.

webbertakken commented 2 years ago

AFAIK apt install python will install python 2, no?

Sure. But I believe you may have missed the past few comments.

mastef commented 2 years ago

We had the symlink python to python3 hack in there, just because python 3 was already installed on the system. Firebase is fine with having python as python 2. There is no necessity to have a "python is python 3" hack overall.

If apt install python is run on all systems, Firebase etc should be fine.

There does not seem to be any necessity for having python 3 symlinked to python.

mastef commented 2 years ago

The problem is that we use both python2 (for webgl) and python3 (for android and iOS).

@webbertakken Ok I found your comment. Firebase just needs any python to be available at python. So if python 2 is installed, everything seems to be ok.

mastef commented 2 years ago

The current code of the PR looks great!

webbertakken commented 2 years ago

The current code of the PR looks great!

It's not complete. It has breaking changes for people that were already relying on python 3. As discussed above, In case we introduce breaking changes we'd rather go up than down - maybe with the exception of WebGL which somehow needs python 2 specifically (I think?).

Installing python 3 and symbolic linking that would seem like a better forward solution.

mastef commented 2 years ago

It's not complete. It has breaking changes for people that were already relying on python 3. As discussed above, In case we introduce breaking changes we'd rather go up than down - maybe with the exception of WebGL which somehow needs python 2 specifically (I think?).

Installing python 3 and symbolic linking that would seem like a better forward solution.

Python 3 is already installed. If somebody needs to use Python 3, they can call python3.

The hack we used was to not add another dependency ( python2 ) - but since that is added now for all images, the hack is not needed any longer.

Do you have any use-case where Python 3 symlinked to python is needed? Because it's not needed for Firebase.

webbertakken commented 2 years ago

@mastef

Python 3 is already installed. If somebody needs to use Python 3, they can call python3.

This is misinformation. Python 3 is not installed on all images. That's exactly what we've been discussing above your first comment. See also this comment https://github.com/game-ci/docker/issues/100#issuecomment-1064919198 on the originating issue, from when this threw us off our game the first time. Please read the posts before your first comment on this thread.

We have concluded that ubuntu does not install python3 by default. Web target needs python 2, some other platforms include (and require) a python 3 installation instead. And so we have agreed on the following solution: https://github.com/game-ci/docker/pull/160#issuecomment-1065990987. Please let us know if you have any relevant input or concerns about our proposed solution.

dharmeshmp commented 2 years ago

Some how our base image contains python3. That's the reason for misinformation.

Screenshot 2022-03-16 at 9 46 49 AM

webbertakken commented 2 years ago

Tried moving all dependencies for WebGL, IL2CPP, iOS, Android together to the base image to see how much space it would take.

image

I think it's safe to say it's only worth moving python and python-setuptools.

webbertakken commented 2 years ago

Confusion about which version is installed

@dharmeshmp: Some how our base image contains python3. That's the reason for misinformation.

~This is likely because you're using Android or iOS as a target platform, which seem to indeed include a Python 3 installation.~ ~In the base image it is never present though~ or so I thought I had confirmed.

Actually it looks like indeed both python2 and python3 are installed after these changes.

To Python3 or not to Python3

Regarding our python2 vs python3 discussion: We've discussed the matter amongst the core contributors of GameCI (cc: @davidmfinol @GabLeRoux @frostebite) and I've updated the opening post with our rationale, conclusions and decision on how to move forward.

cc: @mastef