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

Add support for Python 3 in Android and iOS #152

Closed dharmeshmp closed 2 years ago

dharmeshmp commented 2 years ago

Changes

Checklist

github-actions[bot] commented 2 years ago

Cat Gif

webbertakken commented 2 years ago

When you say python2, do you mean python3?

dharmeshmp commented 2 years ago

I have implemented second suggestion. https://github.com/game-ci/docker/issues/100#issuecomment-1023476647

webbertakken commented 2 years ago

I have implemented second suggestion. #100 (comment)

Much appreciated!

Would you mind updating the PR title and comments in the code to reflect the correct version please?

dharmeshmp commented 2 years ago

of course you can do any thing.

webbertakken commented 2 years ago

of course you can do any thing.

Done.

Did you build the image and check if it solves your problem?

dharmeshmp commented 2 years ago

Did you build the image and check if it solves your problem?

No, But I checked by adding ln -s /usr/bin/python3 /usr/bin/python in before_script stage of gitlab.

webbertakken commented 2 years ago

Did you build the image and check if it solves your problem?

No, But I checked by adding ln -s /usr/bin/python3 /usr/bin/python in before_script stage of gitlab.

Ok good enough. Can you confirm that that worked?

dharmeshmp commented 2 years ago

Ok good enough. Can you confirm that that worked?

Yes, It's working fine for me.

mastef commented 2 years ago

Hi, @dharmeshmp @webbertakken

this PR was based on my comment. After further review I would have recommended to just run the previous step for all targets :

image
-RUN echo "$module" | grep -q -v '\(webgl\|linux-il2cpp\)' \
-  && exit 0 \
-  || : \
-  && apt-get -q update \
+RUN apt-get -q update \
  && apt-get -q install -y --no-install-recommends --allow-downgrades \
    python \
    python-setuptools \
    build-essential \
    clang \
  && apt-get clean \
  && rm -rf /var/lib/apt/lists/*

As that already installs python. It would also remove additional complexity.

mastef commented 2 years ago

Or something like

+RUN apt-get -q install -y python
 RUN echo "$module" | grep -q -v '\(webgl\|linux-il2cpp\)' \
   && exit 0 \
   || : \
   && apt-get -q update \
   && apt-get -q install -y --no-install-recommends --allow-downgrades \
-    python \
     python-setuptools \
     build-essential \
     clang \
   && apt-get clean \
   && rm -rf /var/lib/apt/lists/*
mastef commented 2 years ago

( There's nothing wrong with the current implementation, I just noticed that python is being installed in a previous step. And for some reason I need it now also for win/mac/unix installs as Firebase is not filtering them in the latest version. So having python in all containers may be the fastest fix )