Skip to content

fix: exit Alpine build on unsupported architectures - #2602

Open
nschonni wants to merge 1 commit into
nodejs:mainfrom
nschonni:alpine-unsupported
Open

fix: exit Alpine build on unsupported architectures#2602
nschonni wants to merge 1 commit into
nodejs:mainfrom
nschonni:alpine-unsupported

Conversation

@nschonni

@nschonni nschonni commented Aug 8, 2026

Copy link
Copy Markdown
Member

Description

If one of the allowlisted architectures from the version.json parsed list isn't found, exit the build like on the Debian images.

Motivation and Context

Noticed that the default case on the Alpine behaved differently from the Debian version. Took the same message from the Debian template.

Testing Details

Example Output(if appropriate)

Types of changes

  • Documentation
  • Version change (Update, remove or add more Node.js versions)
  • Variant change (Update, remove or add more variants, or versions of variants)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (none of the above)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • All new and existing tests passed.

@MikeMcC399 MikeMcC399 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

Hopefully it should never hit this line of code!

@nschonni

Copy link
Copy Markdown
Member Author

@tianon @LaurentGoderre @yosifkit I did see a few different patterns like https://github.com/docker-library/openjdk/blob/b05c83d6bd9ca39869729ac90a403edcd8d9efd2/27/bookworm/Dockerfile#L54 on some of the other official images. Is this change OK, or should it be following an upstream pattern (and the Debian images updated too)

@MikeMcC399

Copy link
Copy Markdown
Contributor

I would feel comfortable landing this PR, since it extends the same method to Alpine variants that has been in use for quite a while successfully on Debian variants (although I don't think it has ever triggered).

The other example only looked marginally different, and in both cases they're using exit 1 which seems like it would be the right response for this error condition.

@yosifkit

Copy link
Copy Markdown
Contributor

Is this change OK, or should it be following an upstream pattern (and the Debian images updated too)

It seems fine to me. I think it was previously this fall through so that it could build on an architecture not in the case, but I think you no longer put architectures in the DOI library/node file that aren't in these case statements. You could leave it as-is if you want to allow someone to build for themselves on an unsupported architecture, but I'm unsure if that would be valuable. 🤷

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