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

fix fact tests dont build without closure #174

Merged
merged 4 commits into from
Aug 30, 2022
Merged

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Aug 30, 2022

PR Summary

I discovered while debugging some issues for @ktsai7 that the tests do not build with -DSINGULARITY_BUILD_CLOSURE=OFF because the closure code is used in singularity_eos.cpp, which is called in the tests.

Long term, @dholladay00 I think decoupling closure code from the C/fortran API is probably a good idea. We can tackle that when the xrage-specific stuff moves out of singularity-eos. Short term, I have simply fixed the issue by adding appropriate include guards.

I also add a test in github to check that a minimal build configuration can still construct unit tests.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file

@Yurlungur Yurlungur added the bug Something isn't working label Aug 30, 2022
@Yurlungur Yurlungur self-assigned this Aug 30, 2022
@jhp-lanl
Copy link
Collaborator

I also add a test in github to check that a minimal build configuration can still construct unit tests.

So the "Tests Minimal" is just a subset of the "Tests Full", right? Am I correct than running both is basically to help debugging CI issues?

@Yurlungur
Copy link
Collaborator Author

So the "Tests Minimal" is just a subset of the "Tests Full", right?

Yes, but the code is built differently, so "Tests Minimal" captures a different code-path in the cmake.

Am I correct than running both is basically to help debugging CI issues?

It's partly CI issues, but also partly that we need to continue support building the code without closures if, for example, we want to build with Kokkos but without kokkos-kernels. This new test ensures that that build path continues to work.

@jhp-lanl
Copy link
Collaborator

It's partly CI issues, but also partly that we need to continue support building the code without closures if, for example, we want to build with Kokkos but without kokkos-kernels. This new test ensures that that build path continues to work.

Yeah I think my confusion was that turning off options is not necessarily a perfect subset of the code compared to having those options on; turning off options has different code associated with it in some instances. In other words, testing with more options doesn't guarantee that the code works when those options are off.

@Yurlungur Yurlungur merged commit fbd9246 into main Aug 30, 2022
@Yurlungur Yurlungur deleted the jmm/fix-no-closure branch August 30, 2022 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants