-
Notifications
You must be signed in to change notification settings - Fork 1
Add Basic CI to the project #3
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
Conversation
Signed-off-by: AdityaPandeyCN <[email protected]>
Signed-off-by: AdityaPandeyCN <[email protected]>
Signed-off-by: AdityaPandeyCN <[email protected]>
test/samexample.sam
Outdated
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 is this file uploaded. Seems huge.
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 was in the original repository as an example file so added it for testing I can compress it
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.
Can’t we generate that file. It should be some random genetic sequence…
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 reduced the file content to some 100 line as most of them were repetitive. Will this work? The file size is now 10 kb
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.
That should work but we should do it as a separate pr.
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.
ping.
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.
Should I revert to the original file or like generate one ?
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.
Removed the example sam file
.github/workflows/ci.yml
Outdated
jobs: | ||
build-and-test: | ||
name: Build and Test | ||
runs-on: ubuntu-22.04 |
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.
Maybe use the newer Ubuntu runner (ubuntu-24.04)
.github/workflows/ci.yml
Outdated
runs-on: ubuntu-22.04 | ||
|
||
steps: | ||
- uses: actions/checkout@v4 |
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.
checkout@v5 came out recently, so this can be updated to that.
.github/workflows/ci.yml
Outdated
- name: Install system dependencies | ||
run: | | ||
sudo apt-get update | ||
sudo apt-get install -y cmake build-essential wget gnupg 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.
Although you can put it, sudo is redundant on Github runners I believe, as the account you get has admin privileges.
cmake build-essential wget gnupg are already on Github runners (see
https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md)
I cannot see where you are using libtbb-dev
.github/workflows/ci.yml
Outdated
run: | | ||
ROOT_URL="https://root.cern/download/root_v6.34.00.Linux-ubuntu22.04-x86_64-gcc11.4.tar.gz" | ||
wget -O root.tar.gz $ROOT_URL | ||
sudo tar -xzf root.tar.gz -C /opt/ |
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.
redundant sudo
.github/workflows/ci.yml
Outdated
- name: Configure CMake | ||
run: | | ||
source /opt/root/bin/thisroot.sh | ||
mkdir -p build |
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.
Your starting from scratch with each github workflow run, so -p is not needed. build is guaranteed not to exist
.github/workflows/ci.yml
Outdated
|
||
- name: Install ROOT | ||
run: | | ||
ROOT_URL="https://root.cern/download/root_v6.34.00.Linux-ubuntu22.04-x86_64-gcc11.4.tar.gz" |
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.
root version 6.36 exists. Are you sure you want 6.34?
You'd need to change the link if you change to Ubuntu 24.04 runner.
test/samexample.sam
Outdated
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.
Have you tried compressing this file when added to the repo to reduce its size from 5.26 MB?
You could then have the ci uncompress the file if yes.
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 reduced the file size by deleting it contents which were repetitive.
Signed-off-by: AdityaPandeyCN <[email protected]> updating ci.yml Signed-off-by: AdityaPandeyCN <[email protected]> updating versions in ci.yml Signed-off-by: AdityaPandeyCN <[email protected]> changes Signed-off-by: AdityaPandeyCN <[email protected]> changes Signed-off-by: AdityaPandeyCN <[email protected]>
.github/workflows/ci.yml
Outdated
echo "LD_LIBRARY_PATH=/opt/root/lib:$LD_LIBRARY_PATH" >> $GITHUB_ENV | ||
echo "CMAKE_PREFIX_PATH=/opt/root:$CMAKE_PREFIX_PATH" >> $GITHUB_ENV |
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.
These look like they are redundant since these variables are set by the thisroot.sh script you source
.github/workflows/ci.yml
Outdated
|
||
- name: Configure CMake | ||
run: | | ||
source /opt/root/bin/thisroot.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.
Given /opt/root/bin is now part of the path environment variable (e.g. echo "/opt/root/bin" >> $GITHUB_PATH) then this can just be source thisroot.sh
.github/workflows/ci.yml
Outdated
|
||
- name: Build | ||
run: | | ||
source /opt/root/bin/thisroot.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.
Given /opt/root/bin is now part of the path environment variable (e.g. echo "/opt/root/bin" >> $GITHUB_PATH) then this can just be source thisroot.sh
.github/workflows/ci.yml
Outdated
|
||
- name: Run tests | ||
run: | | ||
source /opt/root/bin/thisroot.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.
Given /opt/root/bin is now part of the path environment variable (e.g. echo "/opt/root/bin" >> $GITHUB_PATH) then this can just be source thisroot.sh
sudo apt-get update | ||
sudo apt-get install -y libvdt-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.
sudo not needed I believe, since Github runner workflows are on an admin account by default
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.
Run apt-get update Reading package lists... E: Could not open lock file /var/lib/apt/lists/lock - open (13: Permission denied) E: Unable to lock directory /var/lib/apt/lists/ Error: Process completed with exit code 100.
getting permission issues if not using sudo
|
||
- name: Install ROOT | ||
run: | | ||
ROOT_URL="https://root.cern/download/root_v6.34.06.Linux-ubuntu24.04-x86_64-gcc13.3.tar.gz" |
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 see that you updated to Ubuntu 24.04 , but not to root 6.36 version? There is a root_v6.36.00.Linux-ubuntu24.04-x86_64-gcc13.3.tar.gz file you can download
.github/workflows/ci.yml
Outdated
push: | ||
branches: [add_ci] | ||
pull_request: | ||
branches: [develop] |
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 should probably change the default branch to main...
Signed-off-by: AdityaPandeyCN <[email protected]> cleanup5 Signed-off-by: AdityaPandeyCN <[email protected]>
Signed-off-by: AdityaPandeyCN <[email protected]>
Signed-off-by: AdityaPandeyCN <[email protected]>
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.
LGTM!
This PR adds basic CI to the ramtools repository.