Add libhwloc support for memkind example #12
Conversation
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @szadam)
-- commits line 2 at r1:
Provide proper author name.
docker/Dockerfile line 88 at r1 (raw file):
openssl-libs \ xz-libs \ zlib \
I suppose these are redundant, please verify.
Code quote:
daxctl-libs \
kmod-libs \
libgcc \
libuuid \
numactl-libs \
openssl-libs \
xz-libs \
zlib \docker/Dockerfile line 94 at r1 (raw file):
COPY hwloc.spec.mk / COPY docker_install_hwloc.sh / RUN /docker_install_hwloc.sh
Remove these files at the end of this dockerfile run.
Code quote:
COPY hwloc.spec.mk /
COPY docker_install_hwloc.sh /
RUN /docker_install_hwloc.shdocker/docker_install_hwloc.sh line 5 at r1 (raw file):
# Copyright (C) 2020 - 2021 Intel Corporation. # docker_install_hwloc.sh - is called inside a Docker container;
Comment not needed.
Code quote:
# docker_install_hwloc.sh - is called inside a Docker container;docker/docker_install_hwloc.sh line 9 at r1 (raw file):
# # Parameters: # -memkind utils/docker directory
Misleading comment, no parameters are passed to this script.
Code quote:
# Parameters:
# -memkind utils/docker directory
#docker/docker_install_hwloc.sh line 14 at r1 (raw file):
set -e UTILS_DOCKER_PATH=$1
Remove this parameter and its usages from the script.
docker/docker_install_hwloc.sh line 43 at r1 (raw file):
# rpmbuild -ba $SPEC # sudo rpm -i $RPMDIR/RPMS/$RPM_ARCH/*.rpm # else
Remove commented out code.
Code quote:
# if [[ $(cat /etc/os-release) = *"fedora"* ]]; then
# rpmdev-setuptree
# SPEC="$UTILS_DOCKER_PATH"/hwloc.spec.mk
# RPMDIR=$HOME/rpmbuild
# RPM_ARCH=$(uname -m)
# cp "$HWLOC_LOCAL_TAR_GZ" "$RPMDIR/SOURCES/${HWLOC_TAR_GZ}"
# rpmbuild -ba $SPEC
# sudo rpm -i $RPMDIR/RPMS/$RPM_ARCH/*.rpm
# else
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @szadam)
docker/Dockerfile line 92 at r1 (raw file):
&& dnf clean all COPY hwloc.spec.mk /
This file is not provided with this commit. It's not needed.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 8 unresolved discussions (waiting on @PatKamin)
Previously, PatKamin (Patryk Kaminski) wrote…
Provide proper author name.
Done.
docker/Dockerfile line 88 at r1 (raw file):
Previously, PatKamin (Patryk Kaminski) wrote…
I suppose these are redundant, please verify.
you're right
docker/Dockerfile line 92 at r1 (raw file):
Previously, PatKamin (Patryk Kaminski) wrote…
This file is not provided with this commit. It's not needed.
done
docker/Dockerfile line 94 at r1 (raw file):
Previously, PatKamin (Patryk Kaminski) wrote…
Remove these files at the end of this dockerfile run.
Done.
docker/docker_install_hwloc.sh line 5 at r1 (raw file):
Previously, PatKamin (Patryk Kaminski) wrote…
Comment not needed.
Done.
docker/docker_install_hwloc.sh line 9 at r1 (raw file):
Previously, PatKamin (Patryk Kaminski) wrote…
Misleading comment, no parameters are passed to this script.
Done.
docker/docker_install_hwloc.sh line 14 at r1 (raw file):
Previously, PatKamin (Patryk Kaminski) wrote…
Remove this parameter and its usages from the script.
Done.
docker/docker_install_hwloc.sh line 43 at r1 (raw file):
Previously, PatKamin (Patryk Kaminski) wrote…
Remove commented out code.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @szadam)
docker/Dockerfile line 85 at r2 (raw file):
COPY docker_install_hwloc.sh / RUN /docker_install_hwloc.sh
nit: Move this just before memkind installation
Code quote:
COPY docker_install_hwloc.sh /
RUN /docker_install_hwloc.shdocker/docker_install_hwloc.sh line 30 at r2 (raw file):
make -j "$(nproc)" sudo make -j "$(nproc)" install # fi
Remove this as well.
Code quote:
# fi
szadam
force-pushed
the
main
branch
4 times, most recently
from
f9e49bf to
93fdf1d
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @PatKamin)
docker/docker_install_hwloc.sh line 30 at r2 (raw file):
Previously, PatKamin (Patryk Kaminski) wrote…
Remove this as well.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @szadam)
docker/Dockerfile line 56 at r3 (raw file):
ncurses-devel\ ndctl-devel\ numactl\
Is it needed? Please, verify.
docker/Dockerfile line 124 at r3 (raw file):
RUN /tz.sh RUN rm /pmdk.sh /valgrind.sh /pmemobj-cpp.sh /pmemkv.sh /setup-maven-settings.sh /pmemkv-java.sh /pmemkv-python.sh /pmemkv-nodejs.sh /pmemkv-ruby.sh /memkind.sh /librpma.sh /tz.sh /docker_install_hwloc.sh
nit: Please, preserve the order of removed scripts to align with the order of executed scripts for better readability.
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @PatKamin)
docker/Dockerfile line 56 at r3 (raw file):
Previously, PatKamin (Patryk Kaminski) wrote…
Is it needed? Please, verify.
You're right. It isn't needed.
docker/Dockerfile line 124 at r3 (raw file):
Previously, PatKamin (Patryk Kaminski) wrote…
nit: Please, preserve the order of removed scripts to align with the order of executed scripts for better readability.
done
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @szadam)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @szadam)
a discussion (no related file):
and just to be sure... have you tested it? will your memkind example work with that? 😉
docker/Dockerfile line 112 at r4 (raw file):
COPY docker_install_hwloc.sh / RUN /docker_install_hwloc.sh
pls rename to be consistent with other script names, e.g., install-hwloc.sh
docker/docker_install_hwloc.sh line 3 at r4 (raw file):
#!/bin/bash # SPDX-License-Identifier: BSD-2-Clause # Copyright (C) 2020 - 2021 Intel Corporation.
-2022
docker/docker_install_hwloc.sh line 13 at r4 (raw file):
HWLOC_TAR_GZ=hwloc-"${HWLOC_VERSION}".tar.gz HWLOC_TARBALL_URL=https://download.open-mpi.org/release/hwloc/"$HWLOC_LIBRARY_VERSION"/"$HWLOC_TAR_GZ"
above, you allow for the HWLOC_LIBRARY_VERSION to be unset (hence the default "1"), but here you expect it to be set...
did you mean to use HWLOC_VERSION instead?
docker/docker_install_hwloc.sh line 15 at r4 (raw file):
HWLOC_TARBALL_URL=https://download.open-mpi.org/release/hwloc/"$HWLOC_LIBRARY_VERSION"/"$HWLOC_TAR_GZ" HWLOC_LOCAL_DIR="$HOME"/hwloc/"$HWLOC_LIBRARY_VERSION"
do you need these paths to be so complicated, why not just use /tmp for downloading this tarball?
docker/docker_install_hwloc.sh line 25 at r4 (raw file):
tar -xzf "$HWLOC_LOCAL_TAR_GZ" -C "$HWLOC_LOCAL_DIR" --strip-components=1 # go to hwloc directory, build and install library
why is this indented?
docker/docker_install_hwloc.sh line 26 at r4 (raw file):
# go to hwloc directory, build and install library cd "$HWLOC_LOCAL_DIR"
pls use pushd <dir> command (instead of cd)
and then, when done add a line with cd back = popd
docker/docker_install_hwloc.sh line 29 at r4 (raw file):
./configure --prefix=/usr make -j "$(nproc)" sudo make -j "$(nproc)" install
sudo is redundant...?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @lukaszstolarczuk)
a discussion (no related file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
and just to be sure... have you tested it? will your memkind example work with that? 😉
yes ;)
docker/docker_install_hwloc.sh line 13 at r4 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
above, you allow for the
HWLOC_LIBRARY_VERSIONto be unset (hence the default "1"), but here you expect it to be set...did you mean to use
HWLOC_VERSIONinstead?
This is Rafał Rudnicki example. I just checked that it works and added a small correction
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @lukaszstolarczuk)
docker/Dockerfile line 112 at r4 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
pls rename to be consistent with other script names, e.g.,
install-hwloc.sh
Done.
docker/docker_install_hwloc.sh line 3 at r4 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
-2022
Done.
docker/docker_install_hwloc.sh line 13 at r4 (raw file):
Previously, szadam (Adam Szopiński) wrote…
This is Rafał Rudnicki example. I just checked that it works and added a small correction
It is working fine.
docker/docker_install_hwloc.sh line 15 at r4 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
do you need these paths to be so complicated, why not just use
/tmpfor downloading this tarball?
It is working fine.
docker/docker_install_hwloc.sh line 25 at r4 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
why is this indented?
Done.
docker/docker_install_hwloc.sh line 26 at r4 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
pls use
pushd <dir>command (instead ofcd)and then, when done add a line with cd back =
popd
It is working fine.
docker/docker_install_hwloc.sh line 29 at r4 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
sudo is redundant...?
yes
There was a problem hiding this comment.
I guess, since it works, we shouldn't change much, at the moment
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status:
This change is