-
Notifications
You must be signed in to change notification settings - Fork 35
Fix UMF build on Alpine OS #1294
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.
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
base: main
Are you sure you want to change the base?
Conversation
#if (_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && !_GNU_SOURCE | ||
char err[1024]; | ||
int err_ret = strerror_r(saveno, err, sizeof(err)); | ||
if (err_ret == ERANGE) { | ||
postfix = "[truncated...]"; | ||
} | ||
#else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it different from the apple version? can you just update apple ifdef?
# | ||
|
||
# Pull base Alpine image version 3.20 | ||
FROM alpine:3.20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls use the full link to the image, pls the pinned, hash version, as e.g. here:
https://github.com/oneapi-src/unified-memory-framework/blob/main/.github/docker/ubuntu-22.04.Dockerfile#L11
alpine: | ||
name: Alpine | ||
strategy: | ||
fail-fast: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed
|
||
steps: | ||
- name: Checkout repository | ||
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already moved on to new version
@@ -13,6 +13,8 @@ permissions: | |||
env: | |||
BUILD_DIR : "${{github.workspace}}/build" | |||
INSTALL_DIR: "${{github.workspace}}/build/install" | |||
HOST_WORKDIR: ${{github.workspace}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can specify these envs only in the job that actually uses it
|
||
cd unified-memory-framework | ||
|
||
cmake -B build -DCMAKE_BUILD_TYPE=$1 -DUMF_BUILD_TESTS=ON -DUMF_BUILD_EXAMPLES=ON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls assign $1
(unnamed param) into a variable with meaningful name
|
||
- name: Run UMF build on Alpine image | ||
run: | | ||
docker run --rm --name alpine_container -i -v $HOST_WORKDIR:$WORKDIR alpine_image $WORKDIR/.github/scripts/alpine_build.sh ${{matrix.build_type}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, you can break the line, the command is quite long
run: | | ||
docker build . -f .github/docker/alpine-3.20.Dockerfile -t alpine_image | ||
|
||
- name: Run UMF build on Alpine image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could also run tests on the build (not sure if as part of build.sh
or as a separate step here)
&& apk add \ | ||
${BASE_DEPS} \ | ||
${TEST_DEPS} \ | ||
${UMF_DEPS} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls add a non-root user (see other dockers as an example)
|
||
set -e | ||
|
||
cd unified-memory-framework |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I somehow dislike this random cd
... but I'm not sure how to better handle this... At the moment I can only think of updating HOST_WORKDIR
and WORKDIR
to already include it... but not sure if this is better...?
|
||
- name: Build Alpine image | ||
run: | | ||
docker build . -f .github/docker/alpine-3.20.Dockerfile -t alpine_image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tag could be probably something more customized, e.g. umf-alpine-3.20
Description
Checklist