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.

Sign up for GitHub

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

Jump to bottom

Compile musl lib, support for alpine image #1267

Closed
wants to merge 2 commits into from
Closed

Compile musl lib, support for alpine image #1267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35b93db
added support for `musl` dependency
ofirsnb Apr 10, 2024
0941f8b
Node: added `musl` compiling action
ofirsnb Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/build-node-wrapper/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@ inputs:
- amazon-linux
- macos-latest
- ubuntu-latest
- ubuntu-latest-musl
named_os:
description: "The name of the current operating system"
required: false
default: "linux"
type: string
options:
- linux
- linux-musl
- darwin
arch:
description: "The current architecture"
Expand Down
15 changes: 14 additions & 1 deletion .github/workflows/install-shared-dependencies/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ inputs:
options:
- x86_64-unknown-linux-gnu
- aarch64-unknown-linux-gnu
- aarch64-unknown-linux-musl
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
- x86_64-apple-darwin
- aarch64-apple-darwin
github-token:
Expand All @@ -35,7 +36,7 @@ runs:
brew upgrade || true
brew install git gcc pkgconfig openssl redis coreutils

- name: Install software dependencies for Ubuntu
- name: Install software dependencies for Ubuntu GNU
shell: bash
if: "${{ inputs.os == 'ubuntu-latest' }}"
run: |
Expand All @@ -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' }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cosider updating DELEVOPER.md

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

keep in mind that below we install specific version of protobuf

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't you need to install openssl-dev?

Copy link
Author

Choose a reason for hiding this comment

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

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



- name: Install Rust toolchain
if: "${{ inputs.os != 'ubuntu-latest-musl' }}"
uses: dtolnay/rust-toolchain@stable
with:
targets: ${{ inputs.target }}

- name: Install protoc (protobuf)
if: "${{ inputs.os != 'ubuntu-latest-musl' }}"
uses: arduino/setup-protoc@v3
with:
version: "25.1"
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/node-create-package-file/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@ inputs:
- amazon-linux
- macos-latest
- ubuntu-latest
- ubuntu-latest-musl
named_os:
description: "The name of the current operating system"
required: false
default: "linux"
type: string
options:
- linux
- linux-musl
Copy link
Contributor

Choose a reason for hiding this comment

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

remove (see comments below)

- darwin
arch:
description: "The current architecture"
Expand Down
46 changes: 41 additions & 5 deletions .github/workflows/node.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ jobs:
- name: Install Redis Modules
uses: ./.github/workflows/install-redis-modules
with:
redis-version: ${{ matrix.redis }}
redis-version: ${{ matrix.redis }}

- name: test
run: npm test
Expand All @@ -73,17 +73,17 @@ jobs:
- name: test hybrid node modules - commonjs
run: |
npm install --package-lock-only
npm ci
npm ci
npm run build-and-test
working-directory: ./node/hybrid-node-tests/commonjs-test

- name: test hybrid node modules - ecma
run: |
npm install --package-lock-only
npm ci
npm run build-and-test
working-directory: ./node/hybrid-node-tests/ecmascript-test

- name: test redis modules
run: npm run test-modules -- --load-module=$GITHUB_WORKSPACE/redisjson.so
working-directory: ./node
Expand Down Expand Up @@ -147,7 +147,7 @@ jobs:
yum -y remove git
yum -y remove git-*
yum -y install https://packages.endpointdev.com/rhel/7/os/x86_64/endpoint-repo.x86_64.rpm
yum install -y git
yum install -y git
git --version

- uses: actions/checkout@v4
Expand Down Expand Up @@ -177,3 +177,39 @@ jobs:
- name: Test compatibility
run: npm test -- -t "set and get flow works"
working-directory: ./node


build-linux-musl-latest:
runs-on: ubuntu-latest
container:
image: node:18.19.0-alpine
timeout-minutes: 15
steps:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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:)

- name: Install git
run: apk add git

- uses: actions/checkout@v4

- name: Checkout submodules
run: |
git config --global --add safe.directory "$GITHUB_WORKSPACE"
git submodule update --init --recursive

- name: Build Node wrapper
uses: ./.github/workflows/build-node-wrapper
with:
os: "ubuntu-latest-musl"
target: "aarch64-unknown-linux-musl"
github-token: ${{ secrets.GITHUB_TOKEN }}
arch: "arm64"
named_os: "linux-musl"
Copy link
Contributor

@barshaul barshaul Apr 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.


- name: Create a symbolic Link for redis6 binaries
working-directory: ./node
run: |
ln -s /usr/bin/redis6-server /usr/bin/redis-server
ln -s /usr/bin/redis6-cli /usr/bin/redis-cli

- name: Test compatibility
run: npm test -- -t "set and get flow works"
working-directory: ./node
15 changes: 13 additions & 2 deletions .github/workflows/npm-cd.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ jobs:
if: github.repository_owner == 'aws'
name: Publish packages to NPM
runs-on: ${{ matrix.build.RUNNER }}
container:
image: ${{ matrix.build.IMAGE }}
strategy:
fail-fast: false
matrix:
Expand All @@ -54,6 +56,14 @@ jobs:
TARGET: aarch64-unknown-linux-gnu,
CONTAINER: "2_28",
}
- {
OS: ubuntu-latest-musl,
NAMED_OS: linux-musl,
RUNNER: ubuntu-latest,
ARCH: arm64,
TARGET: aarch64-unknown-linux-musl,
IMAGE: "node:18.19.0-alpine",
}
- {
OS: macos-latest,
NAMED_OS: darwin,
Expand Down Expand Up @@ -85,6 +95,7 @@ jobs:
echo "RELEASE_VERSION=${version}" >> $GITHUB_ENV

- name: Setup node
if: "${{ matrix.build.OS != 'ubuntu-latest-musl' }}"
uses: actions/setup-node@v3
with:
node-version: "16"
Expand All @@ -93,13 +104,13 @@ jobs:
scope: "${{ vars.NPM_SCOPE }}"
always-auth: true
token: ${{ secrets.NPM_AUTH_TOKEN }}

- name: Update package version in config.toml
uses: ./.github/workflows/update-glide-version
with:
folder_path: "${{ github.workspace }}/node/rust-client/.cargo"
named_os: ${{ matrix.build.NAMED_OS }}

- name: Build Node wrapper
uses: ./.github/workflows/build-node-wrapper
with:
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/update-glide-version/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ inputs:
type: string
options:
- linux
- linux-musl
- darwin
runs:
using: "composite"
Expand Down
5 changes: 4 additions & 1 deletion node/npm/glide/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import { arch, platform } from "process";
import { existsSync } from "fs";

let globalObject = global as unknown;

Expand All @@ -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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@barshaul barshaul Apr 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why using existsSync?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@barshaul barshaul Apr 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

@ofirsnb ofirsnb Apr 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

? require("@scope/glide-for-redis-linux-musl-arm64")
: require("@scope/glide-for-redis-linux-arm64")
break;
default:
throw new Error(
Expand Down
3 changes: 2 additions & 1 deletion node/npm/glide/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@
"${scope}glide-for-redis-darwin-arm64": "${package_version}",
"${scope}glide-for-redis-darwin-x64": "${package_version}",
"${scope}glide-for-redis-linux-arm64": "${package_version}",
"${scope}glide-for-redis-linux-x64": "${package_version}"
"${scope}glide-for-redis-linux-x64": "${package_version}",
"${scope}glide-for-redis-linux-musl-arm64": "${package_version}"
},
"eslintConfig": {
"extends": [
Expand Down
Loading