Skip to content

Detect node architecture to enable MacOS M* (ARM-based processors) builds#108

Merged
szschaler merged 1 commit intomdenet:mainfrom
LinusDietz:main
Mar 24, 2026
Merged

Detect node architecture to enable MacOS M* (ARM-based processors) builds#108
szschaler merged 1 commit intomdenet:mainfrom
LinusDietz:main

Conversation

@LinusDietz
Copy link
Contributor

This fixes mdenet/educationplatform-docker#65

Tested on a Apple M2 Pro (Sequoia 15.7.4)

@szschaler
Copy link
Contributor

Thanks, @LinusDietz. Quick question: is there a reason why you didn't go with the solution originally proposed in mdenet/educationplatform-docker#65?

@LinusDietz
Copy link
Contributor Author

LinusDietz commented Mar 19, 2026

I didn't understand what the solution was concretely?

mdenet-tool-xtext: 
    platform: linux/amd64

I did actually not try that.

If there is a more straightforward way to solve it, that's great.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the Xtext container build to install the correct Node.js binary for the build architecture, enabling successful builds on Apple Silicon (arm64) while retaining x64 compatibility.

Changes:

  • Replaces a fixed linux-x64 Node.js release selection with runtime architecture detection (dpkg --print-architecture).
  • Installs the appropriate Node.js tarball for linux-arm64 vs linux-x64 and standardizes the install path via a /usr/local/node symlink.
  • Updates PATH to use the symlinked Node install directory.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@szschaler
Copy link
Contributor

Thanks. I haven't got a Mac, so am unsure about the solution proposed in the original issue myself. It compiles on my Linux VM, so I'll give it a spin on Github CI...

@szschaler
Copy link
Contributor

In the meantime, what are your thoughts, @LinusDietz, on what CoPilot has suggested above?

@szschaler
Copy link
Contributor

So, this builds successfully on GitHub CI (see mdenet/educationplatform-docker#82). Once we have formed an opinion on the Copilot comments here, I'm happy to get this integrated.

@LinusDietz
Copy link
Contributor Author

The copilot comments feel a bit nitpicky in the context of the educational platform.

  1. checking the shasum might be relevant in highly critical domains, I would keep it simple in this case and avoid including more lines in the dockerfile. (not technically wrong, but the tradeoff is not there for me to include it)
  2. for the same reason, I would keep it simple and not delete the .tar.xz. I don't believe that any meaningful errors are hidden from the user if the curl fails (quite unlikely anyways).
  3. the last comment on the different architectures is unrealistic, as based on my cursory market analysis any docker user on a pc/mac/server will have either arm-64 or x64.

Copy link
Contributor

@szschaler szschaler left a comment

Choose a reason for hiding this comment

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

LGTM

@szschaler szschaler merged commit 3310420 into mdenet:main Mar 24, 2026
5 checks passed
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.

Apple M Compatibility

3 participants