fix: use python3 or python2 for alpine #1331

ttshivers · 2020-09-19T21:44:37Z

In alpine3.12, they removed the python package in favor of either python2 or python3.
https://wiki.alpinelinux.org/wiki/Release_Notes_for_Alpine_3.12.0#python2_no_longer_provides_python_and_python-devel

Refs #1330

nschonni · 2020-09-19T22:43:41Z

Thanks, these changes will need to be made in https://github.com/nodejs/docker-node/blob/master/Dockerfile-alpine.template since these files are generated. Do you know if python 3 would be applicable to all the previous builds as well (3.9-3.12)?

ttshivers · 2020-09-19T22:51:07Z

The python3 package is available in alpine 3.9-3.12, so it should be fine to switch the template to use python3. I went ahead and made the change to the template and ran the update script.

nschonni · 2020-09-19T23:22:27Z

If you run the script with -s you won't get all the extra changes for the Yarn version

ttshivers · 2020-09-19T23:25:51Z

Redid it with the -s flag

PeterDaveHello · 2020-09-21T14:53:59Z

cc @tianon

SimenB · 2020-09-21T15:51:19Z

This shouldn't impact final image as we remove python after building node, no?

nschonni · 2020-09-21T16:17:53Z

@SimenB I think you're right, it mostly has an impact on the Alpine builds that the official images make on other architectures

tianon · 2020-09-21T18:55:30Z

Has this change been tested on an architecture other than amd64 where Node does have to build from source? (where it's currently failing)

nschonni · 2020-09-22T00:45:41Z

@ttshivers did you test this out on one of the other platforms? I'm guessing you might be running on one of those since you opened up the other issue

ttshivers · 2020-09-22T01:03:55Z

Let me try and build all the images on the various arches and see how it goes. Perhaps modifying the github actions to use buildx to build on multiple arches might be useful? I could do a PR for this.

ttshivers · 2020-09-22T01:41:32Z

It appears on nodejs <= 12, python2 is required for building on some architectures. (I ran into an issue building 10/alpine3.10 on linux/arm64)

One solution is to install both python2 and python3. Another is to install just python2.

SimenB · 2020-09-22T08:51:48Z

Perhaps modifying the github actions to use buildx to build on multiple arches might be useful? I could do a PR for this.

Yes, that would be awesome!

One solution is to install both python2 and python3. Another is to install just python2.

Seeing as python2 is EOL, I think we should install v3 for those builds that supports it, but keep 2 for the ones that don't?

ttshivers · 2020-09-22T23:01:24Z

Seeing as python2 is EOL, I think we should install v3 for those builds that supports it, but keep 2 for the ones that don't?

That sounds even more reasonable. I'll work on modifying update.sh to generate the Dockerfiles with the appropriate python version.

nschonni · 2020-09-22T23:05:44Z

Dockerfile-alpine.template

@@ -33,7 +33,7 @@ RUN addgroup -g 1000 node \
        libgcc \
        linux-headers \
        make \
-        python \
+        python3 \


Maybe ${PYTHON_VERSION} and do the replacement in the update. (Just kind of repeating what you said you were working on :))
You could fuzzy match in here for the replacement

docker-node/update.sh

Lines 163 to 169 in 3a5c226

if is_alpine "${variant}"; then

alpine_version="${variant#*alpine}"

checksum="\"$(

curl -sSL --compressed "https://unofficial-builds.nodejs.org/download/release/v${nodeVersion}/SHASUMS256.txt" | grep "node-v${nodeVersion}-linux-x64-musl.tar.xz" | cut -d' ' -f1

)\""

sed -Ei -e "s/(alpine:)0.0/\\1${alpine_version}/" "${dockerfile}-tmp"

sed -Ei -e "s/CHECKSUM=CHECKSUM_x64/CHECKSUM=${checksum}/" "${dockerfile}-tmp"

Ah yes that is a better way that how I initially did it. Let me do it that way.

How is it looking now?

Dockerfile-alpine.template

In alpine3.12, they removed the python package in favor of either python2 or python3. https://wiki.alpinelinux.org/wiki/Release_Notes_for_Alpine_3.12.0#python2_no_longer_provides_python_and_python-devel However, nodejs 12, 10 require python2 for building from source. I added a change in update.sh that swaps out python3 for python2 in those older versions for alpine Dockerfiles. Refs nodejs#1330

nschonni · 2020-09-22T23:50:24Z

update.sh

+
+      # Replace python3 with python2 for nodejs < 14 on alpine
+      if [ "$version" -lt 14 ]; then
+        pythonVersion="python2"


We could probably keep this as the old "python" and it would minimize the diff

I'll have to add some additional logic since "python" isn't available on alpine3.12, and there there is 12/alpine3.12

OK, fair enough, was just looking to see what the minimal change would be. If this is it, that should be good

I could add a check for alpine < 3.12 and nodejs < 14 and have it use "python" in that case. Would that be preferred?
Ex:

if [ "$version" -lt 14 ]; then if (( $(echo "${alpine_version} < 3.12" | bc -l) )); then pythonVersion="python" else pythonVersion="python2" fi else pythonVersion="python3" fi

I think it looks good now, thanks for the patients

nschonni · 2020-09-22T23:53:59Z

@tianon @nodejs/docker should we land this before doing the 14.11.0 PR?

tianon · 2020-09-23T16:43:30Z

LGTM, but #1312 (comment) should be resolved first (or the PR bot disabled) to ensure the downstream PR is good

I tested this on arm64 with docker build --pull https://github.com/nodejs/docker-node.git#refs/pull/1331/merge:10/alpine3.11 and docker build --pull https://github.com/nodejs/docker-node.git#refs/pull/1331/merge:14/alpine3.12 successfully (didn't test all versions, but that's enough to satisfy me that this is definitely better than where we're at now 😄).

nschonni · 2020-10-02T00:02:10Z

@tianon we're a little stalled pinging the admin folks to get the secret set on this repo to fix the PR. I'm going to land this one and the 4.12 while we work it out. Hopefully after you get a change to deploy those, we can get the new setup for the pending 4.13 release PR

nodejs-github-bot · 2020-10-02T00:03:14Z

Created PR to the official-images repo (docker-library/official-images#8820). See https://github.com/docker-library/faq#an-images-source-changed-in-git-now-what if you are wondering when it will be available on the Docker Hub.

ttshivers · 2020-10-02T00:03:56Z

Looks like it worked (or the secret has been added)

nschonni · 2020-10-02T00:04:24Z

@ttshivers no, this is still the old PR flow

ttshivers mentioned this pull request Sep 19, 2020

alpine3.12 images only have linux/amd64 #1330

Closed

ttshivers force-pushed the alpine3.12-fix branch 2 times, most recently from bb64594 to cc88bcf Compare September 19, 2020 22:49

ttshivers force-pushed the alpine3.12-fix branch from cc88bcf to 3672a0b Compare September 19, 2020 23:24

nschonni requested a review from a team September 19, 2020 23:26

nschonni changed the title ~~fix: switch python to python3 for alpine3.12~~ fix: switch python to python3 for alpine Sep 19, 2020

PeterDaveHello approved these changes Sep 21, 2020

View reviewed changes

PeterDaveHello requested a review from a team September 21, 2020 14:53

PeterDaveHello mentioned this pull request Sep 21, 2020

Adjust GitHub Action Job names #1332

Closed

SimenB approved these changes Sep 21, 2020

View reviewed changes

nschonni reviewed Sep 22, 2020

View reviewed changes

ttshivers force-pushed the alpine3.12-fix branch 3 times, most recently from 6586bc1 to 171439d Compare September 22, 2020 23:36

nschonni reviewed Sep 22, 2020

View reviewed changes

Dockerfile-alpine.template Outdated Show resolved Hide resolved

ttshivers force-pushed the alpine3.12-fix branch from 171439d to 550d1b4 Compare September 22, 2020 23:49

nschonni reviewed Sep 22, 2020

View reviewed changes

ttshivers force-pushed the alpine3.12-fix branch from 550d1b4 to 3d290f8 Compare September 22, 2020 23:50

ttshivers changed the title ~~fix: switch python to python3 for alpine~~ fix: use python3 or python2 for alpine Sep 22, 2020

nschonni merged commit d349533 into nodejs:master Oct 2, 2020

nodejs-github-bot mentioned this pull request Oct 2, 2020

Node: fix: use python3 or python2 for alpine docker-library/official-images#8820

Closed

ttshivers deleted the alpine3.12-fix branch October 13, 2020 19:00

	if is_alpine "${variant}"; then
	alpine_version="${variant#*alpine}"
	checksum="\"$(
	curl -sSL --compressed "https://unofficial-builds.nodejs.org/download/release/v${nodeVersion}/SHASUMS256.txt" \| grep "node-v${nodeVersion}-linux-x64-musl.tar.xz" \| cut -d' ' -f1
	)\""
	sed -Ei -e "s/(alpine:)0.0/\\1${alpine_version}/" "${dockerfile}-tmp"
	sed -Ei -e "s/CHECKSUM=CHECKSUM_x64/CHECKSUM=${checksum}/" "${dockerfile}-tmp"

fix: use python3 or python2 for alpine #1331

fix: use python3 or python2 for alpine #1331

Uh oh!

Conversation

ttshivers commented Sep 19, 2020

Uh oh!

nschonni commented Sep 19, 2020

Uh oh!

ttshivers commented Sep 19, 2020

Uh oh!

nschonni commented Sep 19, 2020

Uh oh!

ttshivers commented Sep 19, 2020

Uh oh!

PeterDaveHello commented Sep 21, 2020

Uh oh!

SimenB commented Sep 21, 2020

Uh oh!

nschonni commented Sep 21, 2020

Uh oh!

tianon commented Sep 21, 2020

Uh oh!

nschonni commented Sep 22, 2020

Uh oh!

ttshivers commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ttshivers commented Sep 22, 2020

Uh oh!

SimenB commented Sep 22, 2020

Uh oh!

ttshivers commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nschonni Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

ttshivers Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

ttshivers Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nschonni Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

ttshivers Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nschonni Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

ttshivers Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nschonni Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

nschonni commented Sep 22, 2020

Uh oh!

tianon commented Sep 23, 2020

Uh oh!

nschonni commented Oct 2, 2020

Uh oh!

nodejs-github-bot commented Oct 2, 2020

Uh oh!

ttshivers commented Oct 2, 2020

Uh oh!

nschonni commented Oct 2, 2020

Uh oh!

Uh oh!

ttshivers commented Sep 22, 2020 •

edited

Loading

ttshivers commented Sep 22, 2020 •

edited

Loading

ttshivers Sep 22, 2020 •

edited

Loading

ttshivers Sep 22, 2020 •

edited

Loading