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

Addition of Python 3 #22

Merged
merged 4 commits into from
Feb 27, 2022
Merged

Addition of Python 3 #22

merged 4 commits into from
Feb 27, 2022

Conversation

GazHank
Copy link
Contributor

@GazHank GazHank commented Feb 5, 2022

Inclusion of Python 3 for Centos and Alpine

FOR REVIEW / TESTING

I've tried to make as small a change as possible, and added Python 3 without removing Python 2, to avoid introducing a breaking change. I don't think the other images require the same updates since they are based upon dockcross images.

@vweevers
Copy link
Member

vweevers commented Feb 5, 2022

without removing Python 2, to avoid introducing a breaking change

I actually prefer removing Python 2, to get smaller images and avoid inconsistencies in which versions ends up being used.

@GazHank
Copy link
Contributor Author

GazHank commented Feb 5, 2022

without removing Python 2, to avoid introducing a breaking change

I actually prefer removing Python 2, to get smaller images and avoid inconsistencies in which versions ends up being used.

Cool, that's a small tweak for Alpine. I need to double check on centos as I think we would need to make the change in the upstream image, or simply uninstall python 2.

An upsteam change seems unlikely given there havent been any updates for 2 years (from what I can see)

ralphtheninja
ralphtheninja previously approved these changes Feb 5, 2022
remove python 2 per review feedback
@GazHank
Copy link
Contributor Author

GazHank commented Feb 7, 2022

Hi @vweevers I've dropped Python 2 from the Alpine image, per your suggestion. Removing it from centos looks a little more challenging / risky from what I can see

Given the end result is a little different between the 2 platforms, would you prefer I split this into 2 discrete PRs?

@vweevers
Copy link
Member

vweevers commented Feb 8, 2022

Removing it from centos looks a little more challenging / risky from what I can see

Perhaps we should then keep that. Removing it won't reduce the size (of the base layers) anyway.

Is there an easy way to disable python 2, or make it so that python resolves to Python 3? So that we won't need another semver-major release later on (if and when we do fully remove Python 2). In any case I'd like to test that latest node-gyp does select Python 3.

Can be done in the same PR, I don't mind.

@vweevers vweevers added the semver-major Changes that break backward compatibility label Feb 8, 2022
Copy link
Member

@vweevers vweevers left a comment

Choose a reason for hiding this comment

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

Merging as-is, to unblock us. If CentOS requires a fix (but I believe it will work) we can do so in a follow-up release. I'll test that on a native addon that I'm releasing today (which uses most of the images here, including CentOS).

Thanks!

@vweevers vweevers merged commit e532003 into prebuild:master Feb 27, 2022
@vweevers
Copy link
Member

Oops, when I did a git pull locally I saw I already did this. I completely forgot. Did I also test it? 🤔

diff --git a/centos7-devtoolset7/Dockerfile b/centos7-devtoolset7/Dockerfile
index 1c9b1de..7ce1264 100644
--- a/centos7-devtoolset7/Dockerfile
+++ b/centos7-devtoolset7/Dockerfile
@@ -5,8 +5,13 @@ USER 0
 RUN groupadd -g 1000 node && useradd -g 1000 -u 1000 -m node && \
   groupadd -g 2000 travis && useradd -g 2000 -u 2000 -m travis && \
   curl -fsSL https://rpm.nodesource.com/setup_lts.x | bash - && \
-  yum install -y make nodejs && \
-  npm -v
+  yum install -y make nodejs python3 && \
+  sed -i 's/#!\/usr\/bin\/python/#!\/usr\/bin\/python2/' /usr/bin/yum && \
+  ln -sf python3 /usr/bin/python && \
+  yum clean all && \
+  rm -rf /var/cache/yum && \
+  npm -v && \
+  python --version

@vweevers
Copy link
Member

I'm reverting the main branch and opening a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Changes that break backward compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants