-
Notifications
You must be signed in to change notification settings - Fork 34
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
Docker workflow #1105
base: main
Are you sure you want to change the base?
Docker workflow #1105
Conversation
changes: - created docker build and push workflow - created ubuntu 24.04 docker image - modified reuseable_fast and basic to work on docker image - modified pr/push to run build image when needed - added few installation to docker files
d1700d4
to
527baa9
Compare
# Install hwloc | ||
COPY .github/scripts/install_hwloc.sh /opt/umf/install_hwloc.sh | ||
RUN apt-get update \ | ||
&& apt-get install -y dos2unix libtool \ |
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 define deps as above using ARG and installing it as part of RUN apt ...
command above
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.
done
&& dos2unix /opt/umf/install_hwloc.sh \ | ||
&& bash -x /opt/umf/install_hwloc.sh \ | ||
&& ldconfig \ | ||
&& rm -f /opt/umf/install_hwloc.sh |
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.
no need for removing the script
&& apt-get install -y dos2unix libtool \ | ||
&& dos2unix /opt/umf/install_hwloc.sh \ | ||
&& bash -x /opt/umf/install_hwloc.sh \ | ||
&& ldconfig \ |
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.
are these 3 commands required?
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.
yes because without these commands there is a problem with windows line endings (\r)
|
||
# Install lcov | ||
RUN apt-get update && \ | ||
apt-get install lcov -y |
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.
lcov is a MISC dependency (required only for coverage checking)
|
||
# Install valgrind | ||
RUN apt-get update && \ | ||
apt-get install -y valgrind cmake hwloc libhwloc-dev libnuma-dev libtbb-dev |
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 move it up, to test deps
.github/workflows/reusable_fast.yml
Outdated
- name: Print metadata of our dll's | ||
if: matrix.os == 'windows-latest' | ||
run: | | ||
get-command ${{github.workspace}}/build/bin/Release/umf.dll | format-list | ||
get-command ${{github.workspace}}/build/src/proxy_lib/Release/umf_proxy.dll | format-list | ||
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.
something wrong here with EOF - there's should be just a single empty line at the end
@@ -122,10 +189,9 @@ jobs: | |||
working-directory: ${{env.BUILD_DIR}} | |||
run: ctest --output-on-failure --test-dir test -C Release | |||
|
|||
# TODO: We could add some script to verify metadata of dll's (selected fields, perhaps) |
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.
why remove this comment?
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 restored it
.github/workflows/reusable_fast.yml
Outdated
fetch-depth: 0 | ||
|
||
- name: Initialize vcpkg | ||
if: matrix.os == 'windows-latest' |
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 only execute tests here in this job on windows-latest
- these if
's are no longer needed
.github/workflows/reusable_fast.yml
Outdated
name: Fast builds (Linux) | ||
runs-on: ${{ matrix.os }} | ||
container: | ||
image: ${{ matrix.os == 'ubuntu-latest' && 'ghcr.io/rbanka1/umf2-ubuntu-22.04:latest' || matrix.os == 'ubuntu-20.04' && 'ghcr.io/rbanka1/umf2-ubuntu-20.04:latest' }} |
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.
hmm... TBH, I'm not a fan on this solution - if we have e.g. 10 dockers this will become unreadable... But, I guess, we can figure it out later and leave it as is for now...
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 changed it to a more suitable solution for more dockers
.github/workflows/reusable_fast.yml
Outdated
FastBuild: | ||
name: Fast builds | ||
FastBuild_Linux: | ||
name: Fast builds (Linux) |
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.
Linux
should be fine (we know it's a Fast builds job based on the workflow name 😉
Add Docker workflow
changes:
Checklist