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
Issue #507 - improve gh actions #524
Conversation
…not default (fixes issue in centos7)
… yum external. Enforce the correct compiler
…er is too old. Try to enforce compiler version for centos:7
…kage manager is 7 and thats unfortunatly too old
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.
Whatever possible should be pushed into install.sh
.github/workflows/build.yml
Outdated
- name: Install AmazonLinux Deps | ||
if: startsWith(matrix.container, 'amazonlinux') | ||
run: yum -y install tar xz gzip | ||
- name: Install CentOS Deps | ||
if: startsWith(matrix.container, 'centos:8') | ||
run: | | ||
yum -y install epel-release dnf-plugins-core | ||
yum config-manager --set-enabled powertools |
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.
This must be in install.sh
. How people would otherwise be able to compile on these platforms?
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.
This is specific to centos 8 in order to allow the sources to install the compiler. Since installing the compiler is also not part of install.sh, this should also not be there.
.github/workflows/build.yml
Outdated
- name: Install AmazonLinux Deps | ||
if: startsWith(matrix.container, 'amazonlinux') | ||
run: yum -y install tar xz gzip | ||
- name: Install CentOS Deps | ||
if: startsWith(matrix.container, 'centos:8') | ||
run: | | ||
yum -y install epel-release dnf-plugins-core | ||
yum config-manager --set-enabled powertools |
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.
duplication, see 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.
see above
install.sh
Outdated
if ! command -v ctest &>/dev/null;then | ||
if command -v ctest3 &>/dev/null;then |
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.
Please merge into one if
if possible
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 possible, because the else case needs to happen only if inside the first if
Requires changes after #503 is approved, only merge/review afterwards |
@ilyash-b code was adapted from the changes done on the |
container: [ "fedora:34", "fedora:35", "amazonlinux:2", "centos:8" ] | ||
steps: | ||
- name: Install AmazonLinux Deps | ||
if: startsWith(matrix.container, 'amazonlinux') |
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.
Nice expression!
Fixes issue #507