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

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

Merged
merged 9 commits into from
Mar 19, 2022
Merged

Conversation

webbertakken
Copy link
Member

@webbertakken webbertakken commented Mar 3, 2022

Decision

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

  • go back to python2 for all target platforms (in base image) for Ubuntu 18.04 for and release v1.0.0, introduce BC
  • when Ubuntu 20.04 or 22.04 becomes recommended by Unity, we move to python3 and release v2.0.0, introducing another BC (we'll need to consider which editor versions will support 20.04)

Changes in this PR

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

  • Removes python install from webgl and il2cpp as python3 python2 is already installed on the ubuntu image.
  • Add python-is-python3 to base image (this is available from ubuntu 20.04 on and not desired in ubuntu 18.04)
  • Move build-essential, clang from the editor image to the base image
  • Move python-setuptools from the editor image to the base image
  • Cleanup any code that is no longer needed
  • [BC] BREAKING CHANGE: python will no longer reference python3 (but python2) on iOS and Android target platform images. We believe this will not break existing projects that make use of Firebase SDK.

Fixes: #100

Rationale

Status quo
Discussion points
  • python symlink always used to be python2 so you could argue that developers will use /usr/bin/python3 directly, without the alias.
  • In 2012 PEP recommended to put python2 scripts in the shebang in
    order to stay compatible: "in preparation for an eventual change in the default version of Python, Python 2 only scripts should either be updated to be source compatible with Python 3 or else to use python2 in the shebang line."
  • The goals of Ubuntu were to set python3 as the default and only version in 18.04. They do not seem to have made that goal. Perhaps in 20.04?
Our options
  • install python2 in base image. Change all current python links to use python2 instead of python3. (downside: breaking change people will need to change their workflow if they were using python assuming python3)
  • install python3 in the base image. Change all current python links to use python3 instead of python2 (maybe except for WebGL, which broke on first sight) in a forward approach, without breaking changes. (downside: not every image will have the same link yet)
  • other?
Sources used

Checklist

  • Read the contribution guide and accept the code of conduct
  • Readme (updated or not needed)
  • I looked up the package sizes and this shouldn't add much at all, but still need to verify this
  • We'll need to verify that python works as expected still, on linux-il2cpp and webgl
  • Ideally we'd also check if this fixes the problem with firebase installs

@github-actions
Copy link

github-actions bot commented Mar 3, 2022

Cat Gif

@davidmfinol
Copy link
Member

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

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

@GabLeRoux
Copy link
Member

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

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

webbertakken commented Mar 7, 2022

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

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

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.

images/ubuntu/base/Dockerfile Outdated Show resolved Hide resolved
images/ubuntu/base/Dockerfile Show resolved Hide resolved
@webbertakken
Copy link
Member Author

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

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.

Copy link
Member

@davidmfinol davidmfinol left a comment

Choose a reason for hiding this comment

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

Recommend this change to install python (python 2).

images/ubuntu/base/Dockerfile Outdated Show resolved Hide resolved
images/ubuntu/base/Dockerfile Outdated Show resolved Hide resolved
@webbertakken
Copy link
Member Author

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

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

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

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

Yea I'm in favour of that too.

@mastef
Copy link

mastef commented Mar 13, 2022 via email

@webbertakken
Copy link
Member Author

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

AFAIK apt install python will install python 2, no?

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

@mastef
Copy link

mastef commented Mar 13, 2022

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

mastef commented Mar 13, 2022

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

mastef commented Mar 13, 2022

The current code of the PR looks great!

@webbertakken
Copy link
Member Author

webbertakken commented Mar 13, 2022

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

mastef commented Mar 13, 2022

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

webbertakken commented Mar 13, 2022

@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 #100 (comment) 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: #160 (comment). Please let us know if you have any relevant input or concerns about our proposed solution.

@dharmeshmp
Copy link
Contributor

dharmeshmp commented Mar 16, 2022

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

Screenshot 2022-03-16 at 9 46 49 AM

webbertakken and others added 6 commits March 19, 2022 17:24
Co-authored-by: David Finol <davidmfinol@gmail.com>
Co-authored-by: David Finol <davidmfinol@gmail.com>
Co-authored-by: David Finol <davidmfinol@gmail.com>
@webbertakken
Copy link
Member Author

webbertakken commented Mar 19, 2022

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

  • build-essential
  • clang
  • python
  • python-setuptools

image

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

@webbertakken
Copy link
Member Author

webbertakken commented Mar 19, 2022

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

@webbertakken webbertakken changed the title feat: python-is-python3 and move build essentials to base feat: [BC] Python 2 for all target platforms Mar 19, 2022
@webbertakken webbertakken merged commit 0613cf6 into main Mar 19, 2022
@webbertakken webbertakken deleted the python-is-python3 branch March 19, 2022 20:17
Copy link
Member

@GabLeRoux GabLeRoux left a comment

Choose a reason for hiding this comment

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

reviewed afterward, but it looks good to me. Nice work 😊

mob-sakai pushed a commit to mob-sakai/docker that referenced this pull request May 17, 2022
* feat: python-is-python3 and move build essentials to base

* feat: use manual symlink instead and include pip3

* fix: derp

* fix: python already exists

* feat: add python install

* Apply suggestions from code review

Co-authored-by: David Finol <davidmfinol@gmail.com>

* go back to installing python2

Co-authored-by: David Finol <davidmfinol@gmail.com>

* remove symlinks

Co-authored-by: David Finol <davidmfinol@gmail.com>

* redistribute installations based on their weight

Co-authored-by: David Finol <davidmfinol@gmail.com>
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.

Firebase builds and Android - (ios references and python missing from Android images)
5 participants