Compile `musl` lib, support for `alpine` image #1267

ofirsnb · 2024-04-10T20:21:26Z

Issue #, if available:
fixes #1205

Description of changes:
Re issue above, this PR adds compiling action for musl environment.
Demo of the compiled binary - https://github.com/ofirsnb/node-glide-demo

This PR was tested in a sterile environment, but I need your help testing it under your repo using your workflows.
I expect it to create a new sub-dependency, called glide-for-redis-linux-musl-arm64, which will be pushed to the registry, and of course once done - npm+napi should successfully download and use it under musl env.

Let me know how can I help forward.
Thanks

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Yury-Fridlyand · 2024-04-10T21:31:28Z

.github/workflows/install-shared-dependencies/action.yml

+              apk update
+              wget -O - https://sh.rustup.rs | sh -s -- -y
+              source "$HOME/.cargo/env"
+              apk add protobuf-dev musl-dev make gcc


keep in mind that below we install specific version of protobuf

You're right.
Tried to fix it, but unfortunately the version 25.1 doesn't exists in the alpine repo,
latest version is 24.4-r1.
The official protobuf repo doesn't seem to have a compiled musl package, so I guess we need to rely on 24.4-r1. (or compile it ourself).
Since the binary is being compiled successfully with 24.4-r1, do you still think it's crucial to use 25.1?

Yury-Fridlyand · 2024-04-10T21:31:52Z

.github/workflows/install-shared-dependencies/action.yml

+              apk update
+              wget -O - https://sh.rustup.rs | sh -s -- -y
+              source "$HOME/.cargo/env"
+              apk add protobuf-dev musl-dev make gcc


Don't you need to install openssl-dev?

openssl-dev is installed as part of the musl-dev .
Do you suggest to explicitly specify it?

Yury-Fridlyand · 2024-04-10T21:33:13Z

.github/workflows/install-shared-dependencies/action.yml

@@ -48,12 +49,24 @@ runs:
          run: |
              yum install -y gcc pkgconfig openssl openssl-devel which curl redis6 gettext --allowerasing

+        - name: Install software dependencies for Ubuntu MUSL
+          shell: sh
+          if: "${{ inputs.os == 'ubuntu-latest-musl' }}"


is it supposed for node only or for all clients?
if for node only, move this to node CI and revert this file

this is a good question - if we want to support alpine, we should support it on all languages.

We should support for all languages.
We should take this a step forward and test the package on all languages alpine versions.
Theres no dependency between them tho, we can provide support first to one language alpine, and in a later point to provide support to other languages.
But since we already know that we will provide supports to all in the future I think its ok to lay the foundation for the others.
Its need to be well documented so the developer who will build the support for other languages will have an easy life understanding the foundations and how he can use it.

Cosider updating DELEVOPER.md

It's not nodejs specific, it installs shared glide dependencies for musl, it should be located here

This part is shared and needed for all musl / alpine, as @barshaul said. Not Node specific.
I agree in general it should support all languages, and I strongly agree with @avifenesh to support at this stage only one lang (Node) and expand later.

.github/workflows/install-shared-dependencies/action.yml

Yury-Fridlyand · 2024-04-10T21:38:06Z

CI is red, please fix

acarbonetto · 2024-04-10T21:55:29Z

.github/workflows/install-shared-dependencies/action.yml

@@ -48,12 +49,24 @@ runs:
          run: |
              yum install -y gcc pkgconfig openssl openssl-devel which curl redis6 gettext --allowerasing

+        - name: Install software dependencies for Ubuntu MUSL
+          shell: sh
+          if: "${{ inputs.os == 'ubuntu-latest-musl' }}"


this is a good question - if we want to support alpine, we should support it on all languages.

acarbonetto · 2024-04-10T22:31:08Z

node/npm/glide/index.ts

@@ -17,7 +18,9 @@ function loadNativeBinding() {
                    nativeBinding = require("@scope/glide-for-redis-linux-x64");
                    break;
                case "arm64":
-                    nativeBinding = require("@scope/glide-for-redis-linux-arm64");
+                    nativeBinding = existsSync("./node_modules/@scope/glide-for-redis-linux-musl-arm64")


I suppose this could've been done as the check for any platform

We should have a different check to verify if we're running on musl, to avoid scenarios where we run on musl but glide-for-redis-linux-musl-X wasn't properly installed and therefore we're trying to install glide-for-redis-linux-arm64 instead of exiting with the proper error.
Have you seen this thread for different methods to check if we're running on musl?
nodejs/node#48204 or this library: https://www.npmjs.com/package/detect-libc?activeTab=readme, or: https://github.com/ZingerLittleBee/system-status/blob/main/index.js#L10

We don't need it to be very performant as this is being called only once when the library is being installed.

avifenesh · 2024-04-11T06:43:22Z

node/npm/glide/index.ts

@@ -17,7 +18,9 @@ function loadNativeBinding() {
                    nativeBinding = require("@scope/glide-for-redis-linux-x64");
                    break;
                case "arm64":
-                    nativeBinding = require("@scope/glide-for-redis-linux-arm64");
+                    nativeBinding = existsSync("./node_modules/@scope/glide-for-redis-linux-musl-arm64")


Can you explain why using existsSync?

Pretty sure there's nothing obvious in process that distinguishes between linux and linux-musl

When this code will be called, it will be running on the target host that tries to install glide. If the host is running on musl, only the glide-for-redis-linux-musl-X dependency will be installed on the target host, and therefore he's using the existsSync check to find out which OS are we running on. but, as I said above - I think this isn't the right way to do it

oh, sorry, both distributions will be installed. @ofirsnb Based on os:

The host operating system is determined by process.platform

If process.platform is 'linux' both for musl and not-musl platforms, it means that we install both musl and the glibc linux dependencies on every linux host. That means that with your current implementation (checking if the glide-for-redis-linux-musl-X dependency exists), we'll always get the musl dependency, also on glibc hosts.

You've also changed in the node-create-package file that ${node_os} will be override with 'linux_musl', however npm only accepts the process.platform output, which should be linux.

You're correct @barshaul , missed the os.
This method should be changed.
I suggest using this:
https://github.com/ZingerLittleBee/system-status/blob/main/index.js#L10
as it's also the exact way napi uses to determine musl.
But it not enough either, I'll explain further below.

barshaul

To summarize:
We should check if we're running on musl using one of the ways described here:
https://github.com/ZingerLittleBee/system-status/blob/main/index.js#L10
nodejs/node#48204 https://www.npmjs.com/package/detect-libc?activeTab=readme
instead of checking existsSync("./node_modules/@scope/glide-for-redis-linux-musl-arm64").
We should keep node_os name as 'linux', as the package.json 'os' field is based on process.platform.
and, lastly - what about musl X86 support?

barshaul · 2024-04-14T15:15:09Z

.github/workflows/node.yml

+                  target: "aarch64-unknown-linux-musl"
+                  github-token: ${{ secrets.GITHUB_TOKEN }}
+                  arch: "arm64"
+                  named_os: "linux-musl"


We should keep node_os in node-create-package file as 'linux', so you might want to create a new optional 'libc' argument that will hold the 'musl' suffix

That'll probably work.
As I see it, it the only current solution, if we're getting rid of the existsSync (and we should).
There is another direction we can take, I'll explain further below.

barshaul · 2024-04-14T15:16:20Z

.github/workflows/node-create-package-file/action.yml

    named_os:
        description: "The name of the current operating system"
        required: false
        default: "linux"
        type: string
        options:
            - linux
+            - linux-musl


remove (see comments below)

ofirsnb · 2024-04-14T18:49:45Z

Thank you for your replies, all.

I agree we should support all languages and all architectures. But I also think we can start with one and expand later. This PR focuses on Node+aarch64(arm64), since this is what we need in order to go into production with glide-for-redis, it's a blocker at the moment.
Most problematic issue in this PR, is how to detect musl during runtime (thread1 thread2 - both are needed for it to work.) There is another direction we can do - we can only add the musl compiled binary into the current generated package (i.e glide-for-redis-linux-arm64), it will work out-of-the-box, since napi already has this piece of code in the installed package. I think it would be more elegant, dynamic and simple. I would love to have a quick meet to talk about the architecture, if possible. @barshaul
I don't have much time to work on this on my own at the moment, perhaps more after Passover. If we can coordinate the work on this to get it live as fast as possible, it would be best.

Thanks again have a great week all, keep safe.

alex-arzola-imp · 2024-04-15T17:06:26Z

.github/workflows/node.yml

+        container:
+            image: node:18.19.0-alpine
+        timeout-minutes: 15
+        steps:


This also needs to have bash and gettext tools
Please add:
- name: Install bash run: | apk add bash apk add gettext

Same as noted here ,
both bash and gettext are installed as part of musl-dev.
I tested it few times (can be tested on your own too :) ), no additional packages are needed.
Of course we can explicitly specifying other packages as well like you and Yuri suggested, but I really don't see the point.
WDYT? Whatever you guys decide:)

avifenesh · 2024-05-09T10:40:38Z

Closing - work in #1379 in progress

ofirsnb added 2 commits April 10, 2024 23:11

added support for musl dependency

35b93db

Node: added musl compiling action

0941f8b

ofirsnb requested a review from a team as a code owner April 10, 2024 20:21

ofirsnb mentioned this pull request Apr 10, 2024

Node-Alpine: Initialization Failed #1205

Closed

Yury-Fridlyand reviewed Apr 10, 2024

View reviewed changes

Yury-Fridlyand added node Node.js wrapper CI/CD CI/CD related labels Apr 10, 2024

acarbonetto reviewed Apr 10, 2024

View reviewed changes

avifenesh reviewed Apr 11, 2024

View reviewed changes

avifenesh requested review from barshaul and a team April 11, 2024 06:43

barshaul requested changes Apr 14, 2024

View reviewed changes

barshaul reviewed Apr 14, 2024

View reviewed changes

alex-arzola-imp reviewed Apr 15, 2024

View reviewed changes

barshaul mentioned this pull request May 2, 2024

Node/dev aarzola alpine fix #1287

Closed

barshaul marked this pull request as draft May 5, 2024 17:18

avifenesh closed this May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile `musl` lib, support for `alpine` image #1267

Compile `musl` lib, support for `alpine` image #1267

ofirsnb commented Apr 10, 2024

Yury-Fridlyand Apr 10, 2024

ofirsnb Apr 14, 2024

Yury-Fridlyand Apr 10, 2024

ofirsnb Apr 14, 2024

Yury-Fridlyand Apr 10, 2024

acarbonetto Apr 10, 2024

avifenesh Apr 11, 2024

Yury-Fridlyand Apr 11, 2024

barshaul Apr 14, 2024

ofirsnb Apr 14, 2024

Yury-Fridlyand commented Apr 10, 2024

acarbonetto Apr 10, 2024

acarbonetto Apr 10, 2024

barshaul Apr 14, 2024 •

edited

Loading

avifenesh Apr 11, 2024

acarbonetto Apr 11, 2024

barshaul Apr 14, 2024

barshaul Apr 14, 2024 •

edited

Loading

barshaul Apr 14, 2024

ofirsnb Apr 14, 2024 •

edited

Loading

barshaul left a comment •

edited

Loading

barshaul Apr 14, 2024 •

edited

Loading

ofirsnb Apr 14, 2024

barshaul Apr 14, 2024

ofirsnb commented Apr 14, 2024 •

edited

Loading

alex-arzola-imp Apr 15, 2024

ofirsnb Apr 15, 2024

avifenesh commented May 9, 2024

Compile musl lib, support for alpine image #1267

Compile musl lib, support for alpine image #1267

Conversation

ofirsnb commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul Apr 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul Apr 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofirsnb Apr 14, 2024 • edited Loading

Choose a reason for hiding this comment

barshaul left a comment • edited Loading

Choose a reason for hiding this comment

barshaul Apr 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofirsnb commented Apr 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avifenesh commented May 9, 2024

Compile `musl` lib, support for `alpine` image #1267

Compile `musl` lib, support for `alpine` image #1267

barshaul Apr 14, 2024 •

edited

Loading

barshaul Apr 14, 2024 •

edited

Loading

ofirsnb Apr 14, 2024 •

edited

Loading

barshaul left a comment •

edited

Loading

barshaul Apr 14, 2024 •

edited

Loading

ofirsnb commented Apr 14, 2024 •

edited

Loading