Skip to content
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

Add ctest to spack build pipelines. #118

Merged
merged 13 commits into from
Jun 21, 2024
Merged

Add ctest to spack build pipelines. #118

merged 13 commits into from
Jun 21, 2024

Conversation

cameronrutherford
Copy link
Collaborator

Adds a ctest invocation to the CPU spack builds (same fix can be extended to ExaGO + HiOp if functioning well...)

@@ -130,6 +130,9 @@ jobs:
- name: Install
run: spack -e . install --no-check-signature

- name: Test Installation
run: . buildsystem/spack/spack/share/spack/setup-env.sh && spack -e . load cmake && cd `ls | grep spack-build | grep -v txt` && ctest -VV
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be easier to just source this script during "Setup Spack" stage of the job instead of doing it haphazardly here

@cameronrutherford
Copy link
Collaborator Author

Tests were false positive-ing last run. Hoping my latest change fixes that

@cameronrutherford
Copy link
Collaborator Author

For some reason no tests are being found... Need to debug + diagnose further, as a similar workflow works just fine locally

@superwhiskers
Copy link
Collaborator

i suggest we enable address sanitizer (and consider adding ubsan and isan to the mix) with -DRESOLVE_USE_ASAN=yes in the flags to cmake if we're going to run tests this way, and if not immediately, we should make this a priority, as it'd keep pull requests from introducing obvious memory safety issues (and if we add ubsan/isan, integer overflow and undefined behavior)

this, of course, breaks a lot of tests immediately due to the amount of memory leaks present, but i'm sure the current ones aren't too hard to fix

@pelesh
Copy link
Collaborator

pelesh commented Jun 20, 2024

For some reason no tests are being found... Need to debug + diagnose further, as a similar workflow works just fine locally

@cameronrutherford, do we know why tests are not run?

@cameronrutherford
Copy link
Collaborator Author

i suggest we enable address sanitizer (and consider adding ubsan and isan to the mix) with -DRESOLVE_USE_ASAN=yes in the flags to cmake if we're going to run tests this way, and if not immediately, we should make this a priority, as it'd keep pull requests from introducing obvious memory safety issues (and if we add ubsan/isan, integer overflow and undefined behavior)

this, of course, breaks a lot of tests immediately due to the amount of memory leaks present, but i'm sure the current ones aren't too hard to fix

This is a great idea and should be captured in another issue. Would just involve adding a flag to CMake through spack, and then also adding that to the CMakePresets.json for the relevant build configurations.

@cameronrutherford
Copy link
Collaborator Author

For some reason no tests are being found... Need to debug + diagnose further, as a similar workflow works just fine locally

@cameronrutherford, do we know why tests are not run?

Part of the difficulty is that the GitHub actions runners are a VM, and not a strict container. Any debugging locally doesn't 1-1 map to the CI environment.

We could do the following:

@cameronrutherford
Copy link
Collaborator Author

I force pushed to re-run CI and will try a few things, but I don't know if I will get anywhere, and the spack test support #152 seems like the more sensible approach perhaps? Maybe it would also be easier to support some easier way from running tests out of the installation directory, instead of the build directory like I am trying to do.

@pelesh
Copy link
Collaborator

pelesh commented Jun 20, 2024

I force pushed to re-run CI and will try a few things, but I don't know if I will get anywhere, and the spack test support #152 seems like the more sensible approach perhaps? Maybe it would also be easier to support some easier way from running tests out of the installation directory, instead of the build directory like I am trying to do.

Shouldn't we have both -- test build and test installation? We have separate make test and make test_install commands.

@pelesh
Copy link
Collaborator

pelesh commented Jun 20, 2024

The issue I see here is that ctest is not run:

Run cd `ls | grep spack-build | grep -v txt` && ctest -VV
  cd `ls | grep spack-build | grep -v txt` && ctest -VV
  shell: /usr/bin/bash -e {0}
  env:
    SPACK_COLOR: always
    REGISTRY: ghcr.io/ornl
    IMAGE_NAME: resolve
    USERNAME: resolve-bot
    BASE_VERSION: ubuntu-[2](https://github.com/ORNL/ReSolve/actions/runs/9601113332/job/26479373106?pr=118#step:11:2)2.04-fortran
UpdateCTestConfiguration  from :/home/runner/DartConfiguration.tcl
Test project /home/runner
Constructing a list of testsTest project /home/runner
Constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
No tests were found!!!

@cameronrutherford
Copy link
Collaborator Author

This is rebased on #168 and builds are running and so is ctest in CI after the spack builds!! ◡̈

Copy link
Collaborator Author

@cameronrutherford cameronrutherford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I undid all the applied linting to the CMake, so I think this is ready to merge @pelesh

Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests run correctly and pass. Great job!

@pelesh pelesh merged commit b77ca06 into develop Jun 21, 2024
4 checks passed
@pelesh pelesh deleted the spack-build-add-test branch June 26, 2024 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants