-
Notifications
You must be signed in to change notification settings - Fork 58
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
aarch64 CI test #829
base: master
Are you sure you want to change the base?
aarch64 CI test #829
Conversation
# the tests need the gprofiler image built (from Dockerfile). I run it separately here, because "docker build" prints the build logs | ||
# more nicely. the tests will then be able to use the built image. | ||
- name: Build gProfiler image | ||
run: ./scripts/build_aarch64_container.sh -t gprofiler_aarch64 --output type=docker |
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.
Since #790, the gProfiler image is a blank image that contains the executable.
Please see the job build-container-x64
- it has
needs:
- build-executable-x64
to make sure the CI runs it after the exe is built, and then it uses the same binary exe built earlier to put it into the image, as done here:
run: ./scripts/build_x86_64_container.sh --skip-exe-build --build-arg EXE_PATH=output/gprofiler_x86_64 -t gprofiler_x86_64
Please do the same here - by not passing --skip-exe-build
you are rebuilding it unnecessarily, and plus, in any case, we'd like them to use the same file exactly.
with: | ||
submodules: true | ||
|
||
- name: Download the executable from previous job |
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.
"Download the image from previous job", no?
- name: Import gProfiler image | ||
run: docker image load < output/gprofiler_aarch64.img | ||
|
||
- name: Extract resources from gProfiler executable |
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.
After you follow my suggestion from https://github.com/Granulate/gprofiler/pull/829/files#r1314022332 you can also simplify the code here to use the gProfiler executable instead of image - so it can run the exe and don't need the docker, the mappings etc, just like the x86_64 job.
- name: setup runner requirements | ||
run: ./scripts/setup_runner_requirements.sh | ||
|
||
# TODO: Add docker layer caching when GitHub Actions cache is stabilized and works good with "satackey/[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.
This comment is not relevant here, remove pls
run: ./scripts/setup_runner_requirements.sh | ||
|
||
# TODO: Add docker layer caching when GitHub Actions cache is stabilized and works good with "satackey/[email protected]" | ||
- name: Run gProfiler tests |
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.
The x86_64 tests run on 3.8, 3.9, 3.10.
I'm actually planning to deprecate 3.8 and 3.9 soon so there's no need to test all 3 - let's focus on 3.10.
Can you use the GH action to install python? i.e
steps:
- name: Set up Python 3.10
uses: actions/setup-python@v2
with:
python-version: "3.10"
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.
You know what, let's actually do it with strategy
like done in the x86_64 tests:
strategy:
matrix:
python-version:
- "3.10"
this is extendable for a future where we'll want to add 3.11, and also shows up nicely in the job name (gonna be "test-container-aarch64 (3.10)" like we have for the x86 ones)
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.
Gave some comments.
This doesn't include test-executable-aarch64
but I'm okay with adding it later.
Note that CICD is failing - seems like a Docker issue where we're not using BuiltKit properly (BuildKit recognizes --platform
and standard Docker doesn't iirc)
|
||
- name: Get and verify tag value | ||
run: ./scripts/verify_tag.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.
Need sudo?
@@ -8,7 +8,7 @@ set -euo pipefail | |||
# Used in CI and checks that last pushed tag is greater than last existing tag. | |||
# Using python package 'cmp_version' to do the compare work | |||
|
|||
pip install cmp_version |
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.
Need it?
#768