Skip to content

Docker Fixes - #3657

Open
0cjs wants to merge 4 commits into
nvm-sh:masterfrom
0cjs:dev/cjs/25i15/docker-fixes
Open

Docker Fixes#3657
0cjs wants to merge 4 commits into
nvm-sh:masterfrom
0cjs:dev/cjs/25i15/docker-fixes

Conversation

@0cjs

@0cjs 0cjs commented Sep 15, 2025

Copy link
Copy Markdown
Contributor

The Docker image build was broken due to using a bad mirror; remove the mirror to fix this in a way that it's unlikely to recur. Fix a few bugs in the documentation for the Docker image/container and then do a general cleanup on it.

See the commit messages for full details.

@PeterDaveHello PeterDaveHello left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This PR mixes in many things, and some of them are unnecessary or related to different preferences. I suggest refocusing on the key parts that actually need adjustment.

Comment thread README.md
Comment thread README.md Outdated
Comment thread Dockerfile

# Pick a Ubuntu apt mirror site for better speed
# ref: https://launchpad.net/ubuntu/+archivemirrors
ENV UBUNTU_APT_SITE ubuntu.cs.utah.edu

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The mirror looks functional. Please provide broken proof of the "bad mirror".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The proof is in the error messages I gave in the commit message. Did you read those? How did you replicate this to have it working for you? Did you use docker build --no-cache to ensure you were actually rebuilding those layers? If that worked for you, I suppose there is a possibility that this is somehow related to geolocation (I'm in Tokyo), though I can't really see how that would be.

Nonetheless, setting a proxy that is likely to be worse for most people (I expect that Canonical has far better network connectivity than a mid-level university) in a place where users are unlikely to come across it is a bad idea anyway.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I checked its certificate, and it's correct, but I should test it another way to make sure we are on the same page, yes. Will do it later.

@0cjs 0cjs Sep 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You probably already know this, but just in case, remember that you need to try to validate the remote cert using apt in the container itself. Just because it works in the web browser on your laptop doesn't mean that that cert can be validated using the cert store built into the image. (Which is, after all, more than three years old now.)

Just doing a docker build --no-cache -t temp0 . at the head of master should demonstrate the problem. If it doesn't, that's very mysterious to me. I just checked this again right now and still see the issue.

@0cjs

0cjs commented Sep 16, 2025

Copy link
Copy Markdown
Contributor Author

This PR mixes in many things, and some of them are unnecessary or related to different preferences.

I believe the commits here separate the changes into properly separate groups of related changes. Which commits are you proposing should be split? Or did you want each commit to go into a separate PR?

Oh, and one more note: if you're reviewing the full set of diffs for the entire PR, it's likely to be somewhat messy and confusing, and mixing concerns. Try reading the diff for each individual commit; these along with the commit message should tell a very clear story about exactly what I'm doing, step by step.

@PeterDaveHello

Copy link
Copy Markdown
Collaborator

This PR mixes in many things, and some of them are unnecessary or related to different preferences.

I believe the commits here separate the changes into properly separate groups of related changes. Which commits are you proposing should be split? Or did you want each commit to go into a separate PR?

Oh, and one more note: if you're reviewing the full set of diffs for the entire PR, it's likely to be somewhat messy and confusing, and mixing concerns. Try reading the diff for each individual commit; these along with the commit message should tell a very clear story about exactly what I'm doing, step by step.

Yes, it has indeed been split into several different commits. In my view, style- or preference-related adjustments are somewhat different in nature from actual fixes.

@0cjs

0cjs commented Sep 16, 2025

Copy link
Copy Markdown
Contributor Author

Yes, it has indeed been split into several different commits. In my view, style- or preference-related adjustments are somewhat different in nature from actual fixes.

I agree, which is why I put them into separate commits.

@slow6r

This comment was marked as spam.

0cjs added 4 commits July 5, 2026 22:08
The ubuntu.cs.utah.edu mirror appears to be obsolete; trying to use it
produces error messages along the lines of:

    Ign:2 https://ubuntu.cs.utah.edu/ubuntu jammy-updates InRelease
    Err:2 https://ubuntu.cs.utah.edu/ubuntu jammy-updates InRelease
      Certificate verification failed: The certificate is NOT trusted. The certificate issuer is unknown.  Could not handshake: Error in the certificate verification. [IP: 155.98.64.89 443]
    W: http://ubuntu.cs.utah.edu/ubuntu/dists/jammy-updates/InRelease: No system certificates available. Try installing ca-certificates.

These messages don't make it completely obvious what the problem is, and
it's also a rather obscure setting; you have to carefully read the
Dockerfile to find it.

Due to issues like this, and changing mirrors over time, it's best
just to drop attempts to use a specific mirror and use whatever the
default you get for archive.ubuntu.com etc.

The package downloads generally happen only once per host, anyway,
since Docker will keep not only the final image, but also the layers
used to generate it: if the final image is deleted or anything in the
Dockerfile after the OS package updates is changed, these cached
layers will be used.
On my system, even without using local mirrors, the build is only two
minutes, even though I have only a 1 Gbps Internet connection and I'm
half-way across the world in Tokyo.

There probably are still users with much slower Internet connections
where the build could take eight minutes or more, but "several" still
covers that.
* Add `--name` to `docker run` command to make the container easier to
  manipulate. (This will produce collisions if two developers both try
  this on the same system, but it's not worth a lot of extra
  documentation to try to sort out the few developers who can't figure
  out what's going on from the error messages.)

* Explain better that the current repo and working copy are copied to
  ~/.nvm/ inside the container.

* Explain that the container is not deleted when you exit, and how to
  re-enter and delete it.

* General improvements to the language.
@0cjs
0cjs force-pushed the dev/cjs/25i15/docker-fixes branch from 19efb47 to 6980aad Compare July 5, 2026 13:10
@0cjs

0cjs commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

@slow6r I am unclear as to what your comment means; it looks as if you cut and pasted something done by a robot on another PR?

But at any rate, thanks for poking this; it has been sitting around for a while. I've rebased the branch on to the latest tip of master and confirmed that build --no-cache -t nvm-dev . (as the README says, but with --no-cache added) still works.

@PeterDaveHello I'm sorry that my PR seems so difficult to review that even after nine months it's still not done. Is there anything I can do to help out with this? I get the sense that you might find it easier if I split this into four PRs, one per commit; I'm happy to do that if it helps you out.

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.

3 participants