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

[Dockerfile] Update base image from Ubuntu 20.04 to 22.04, cc #2877 #3078

Merged
merged 1 commit into from
Apr 10, 2023

Conversation

PeterDaveHello
Copy link
Collaborator

Did some basic tests, besides old versions like node.js v12 will need python 2 to build from source, the other part looks good.

@ljharb
Copy link
Member

ljharb commented Apr 5, 2023

Can we make sure python2 is included by default then? Ideally the dockerfile works with every version of node - down to 0.6.

@PeterDaveHello
Copy link
Collaborator Author

I don't think that will make any sense, only edge cases need to install very old packages on new systems, prefer not to make it too complicated, it's basically deprecated by every Linux distribution, and it's done since Ubuntu 20.04, so it has nothing to do with this change.

@ljharb
Copy link
Member

ljharb commented Apr 5, 2023

I'm not sure I agree - this is a dockerfile for nvm, and it should support whatever nvm supports. Are you saying ubuntu 20 already has this problem?

@PeterDaveHello
Copy link
Collaborator Author

Ubuntu 20.04 doesn't install Python2 by default. Though we can still install it, if that's you wanted, but it doesn't look like there is a complain about Python2 is missing at all, I doubt how many user is really using this Docker image to do the development work of nvm or testing it, and how long we'd like to additionally add Python2 to it? Eventually it'll be removed, right?

@ljharb
Copy link
Member

ljharb commented Apr 6, 2023

Why would it ever be removed, if it's always required for testing older nodes?

@PeterDaveHello
Copy link
Collaborator Author

It'll be removed from newer OS version eventually, just not completely deprecated yet?

@ljharb
Copy link
Member

ljharb commented Apr 6, 2023

What will be, python? I'm confused.

@PeterDaveHello
Copy link
Collaborator Author

Oh, sorry, I mean Python 2.

@ljharb
Copy link
Member

ljharb commented Apr 6, 2023

Whether it's there by default or not should be irrelevant; since it will eternally be needed for installing older nodes, I'd want nvm's dockerfile to eternally provide it.

@PeterDaveHello
Copy link
Collaborator Author

@ljharb mind to have a separated PR to do it?

@ljharb
Copy link
Member

ljharb commented Apr 6, 2023

Landing this PR now would mean that the dockerfile is broken for those use cases until the separate PR lands, right?

@PeterDaveHello
Copy link
Collaborator Author

The Python problem is not a result of this pull request, and the Dockerfile isn't even broken. To build Node.js from source code inside this Docker image, you just simply need to manually install Python. This likely won't be a common scenario, as binary releases for Ubuntu Linux are quite reliable.

In fact, regardless of whether Ubuntu doesn't install Python 2 by default or removes it from the repository, the Docker image doesn't appear to include Python 2 at all. Additionally, we haven't installed it since the beginning.

To verify that Ubuntu Docker image doesn't include Python / Python 2.

$ for ver in 16.04 18.04 20.04; do docker run --rm -it ubuntu:$ver bash -c 'cat /etc/issue; command -v python; command -v python2;'; done
Ubuntu 16.04.7 LTS \n \l

Ubuntu 18.04.6 LTS \n \l

Ubuntu 20.04.5 LTS \n \l

@ljharb
Copy link
Member

ljharb commented Apr 10, 2023

ahh ok sorry, that was the piece i was missing. If binaries are available for this image, then it's only an issue for node 0.4 and 0.6, and if it's always been the case, then it needn't be a blocker here. Thanks.

@ljharb ljharb merged commit 44e1d9c into nvm-sh:master Apr 10, 2023
@PeterDaveHello PeterDaveHello deleted the update-Dockerfile branch April 10, 2023 06:32
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.

2 participants