Updates and bugfixes for blt@0.5.3 #1120

kennyweiss · 2023-06-19T18:03:17Z

Summary

This PR is maintenance and bugfixes
It updates the blt submodule to blt@0.5.3, which updates the gtest and gmock version
It fixes a compilation error in the axom::Array unit tests -- the templated type requires an operator<<() overload and was missing for a type defined in the tests.
- It also removes some utility functions in the above test from the axom namespace
It also fixes a build error w/ MSVC that has cropped up before. When using defines like M_PI from math.h, we either have to make sure that we load <cmath> after defining _USE_MATH_DEFINES, or include math.h instead due to a guard in msvc's cmath that prevents it from being loaded a second time. (See also: https://github.com/search?q=repo%3ALLNL%2Faxom+cmath&type=commits)
It also fixes a compilation issue w/ Klee's gmock testing macros on LLNL's blueos. clang+nvcc was complaining about a missing constructor when using the MATCHER_P macros. I expanded these to a simplified version of the underlying classes, which appears to have resolved the problem.

TODO:

There's an unresolved segfault in a quest::SignedDistance test that still needs to be resolved. (Fixed)
Update RELEASE_NOTES

kennyweiss · 2023-06-19T18:05:12Z

src/axom/core/tests/core_array.hpp

+  friend std::ostream& operator<<(std::ostream& os, const HasDefault& hd)
+  {
+    return os << hd.member;
+  }


This was the missing operator<<() overload required for axom::Array<HasDefault>

kennyweiss · 2023-06-19T18:06:09Z

src/axom/core/tests/core_array.hpp

-namespace axom
-{
-namespace internal
+namespace


Nearly all the changes in this file are due to removing the utility functions from the axom and axom::internal namespace.

Why did you remove the utility functions from axom and axom::internal, especially if they are only in the tests?

Judgement call, I guess. It was part of my debugging process and deciphering why the test was failing.

I felt that including the code in this file in the axom namespace was obscuring which parts were from the main axom/core vs. which were provided only in this test file. Also, since Array is a common class name, marking it explicitly as axom::Array seemed (to me) to better highlight that we were using an axom class.

…ce_interface tests Gtest changed how they define `GTEST_HAS_DEATH_TEST` from "always defined with a value of 0 or 1" to "defined when available or undefined." This made our previous workaround stop working. The key problem appears to be that death tests are not properly handled in MPI configurations, leading to a "broken pipe" error. See: * google/googletest#3267 * google/googletest@2314284#diff-97f768d897442b94f24002f6bcff7124e3c73d05b3916fb492ee29436ce0701f

…_PI`

kennyweiss · 2023-06-20T04:45:04Z

src/axom/primal/tests/primal_polygon.cpp

@@ -3,11 +3,13 @@
 //
 // SPDX-License-Identifier: (BSD-3-Clause)

-#include "gtest/gtest.h"
-
+#include "axom/config.hpp"


I suspect the "Undefined M_PI" error in MSVC was due to including gtest/gtest.h before axom/config.hpp, which defined _USE_MATH_DEFINES.

There's an extra include guard in <cmath> that requires _USE_MATH_DEFINES to be set before the first time <cmath> is included.

See, e.g.:

https://stackoverflow.com/questions/6563810/m-pi-works-with-math-h-but-not-with-cmath-in-visual-studio

https://github.com/search?q=repo%3Agoogle%2Fgoogletest%20cmath&type=code

Errors were of the form:

2023-06-19T18:33:43.8255199Z D:\a\1\s\src\axom\primal\tests\primal_polyhedron.cpp(603,31): error C2065: 'M_PI': undeclared identifier [D:\a\1\s\build\axom\primal\tests\primal_polyhedron_test.vcxproj] 2023-06-19T18:33:43.8255668Z D:\a\1\s\src\axom\primal\tests\primal_polyhedron.cpp(603,18): error C2737: 'alpha': const object must be initialized [D:\a\1\s\build\axom\primal\tests\primal_polyhedron_test.vcxproj] 2023-06-19T18:33:43.8256083Z D:\a\1\s\src\axom\primal\tests\primal_polyhedron.cpp(612,39): error C2065: 'M_PI': undeclared identifier [D:\a\1\s\build\axom\primal\tests\primal_polyhedron_test.vcxproj] 2023-06-19T18:33:43.8256465Z D:\a\1\s\src\axom\primal\tests\primal_polyhedron.cpp(613,39): error C2065: 'M_PI': undeclared identifier [D:\a\1\s\build\axom\primal\tests\primal_polyhedron_test.vcxproj] 2023-06-19T18:33:43.8256870Z D:\a\1\s\src\axom\primal\tests\primal_polyhedron.cpp(614,43): error C2065: 'M_PI': undeclared identifier [D:\a\1\s\build\axom\primal\tests\primal_polyhedron_test.vcxproj] 2023-06-19T18:33:43.8257267Z D:\a\1\s\src\axom\primal\tests\primal_polyhedron.cpp(615,43): error C2065: 'M_PI': undeclared identifier [D:\a\1\s\build\axom\primal\tests\primal_polyhedron_test.vcxproj]

https://dev.azure.com/healy22/d4445b8f-6e95-49cd-9e5b-8c8ca1ccad31/_apis/build/builds/8213/logs/39

kennyweiss · 2023-06-20T04:50:41Z

src/axom/quest/tests/quest_signed_distance_interface.cpp

+// Note: Disable death tests when MPI is enabled since they're not being properly handled
+#ifdef AXOM_USE_MPI
+  #define _DEATH_TESTS_LOCALLY_DISABLED 1
+#else
+  #define _DEATH_TESTS_LOCALLY_DISABLED 0
+const char IGNORE_OUTPUT[] = ".*";
+#endif


This was an odd one.

GTest's death tests appear to be incompatible w/ MPI since they don't properly shut the app down in the spawned thread (?) leading to "Broken Pipes".

We previously undefined GTEST_HAS_DEATH_TEST` when using MPI to avoid this

GTest recently (~ March 2022) changed how they treat #defines -- they used to always be defined, w/ a value of 1 when enabled and 0 when disabled. They changed it to be defined when enabled and undefined when disabled since this allows users to use the -Wundef flag in their build systems.

This was the only file in axom that had death tests + MPI

My hack (which I'm not thrilled with) is to add another #define to control whether we're using the death tests, and to disable them when using MPI.

… to blt@0.5.3 When using GMock's `MATCHER_P` macro for klee's tests, clang+nvcc on LLNL's blueos was complaining about not finding a compatible constructor. This commit explicitly creates compatible types for `primal::Point`, `primal::Vector`, `axom::numerics::Matrix` and `klee::SliceOperator`s.

kennyweiss · 2023-06-21T07:02:42Z

src/axom/klee/tests/KleeMatchers.hpp

@@ -19,72 +23,135 @@ namespace test
 {
 constexpr double tolerance = 1e-10;

-MATCHER_P(AlmostEqMatrix, m, "")
+/// Create a GMock Matcher for an axom::numerics:Matrix


Note: Expanding the MATCHER_P macros only adds a few lines of boilerplate.

agcapps

Thank you for these cleanups and fixes, @kennyweiss .

white238 · 2023-06-21T17:29:28Z

src/axom/klee/tests/klee_geometry_operators.cpp


-#include <cmath>


Can you add a note why we are using the .h header since that is not a normal thing to do in our code?

cmath should include math.h and be the C++ standard way.

cmath should include math.h and be the C++ standard way.

I couldn't agree more, but the issue appears to be out of our control, and keeps cropping up.
(See: https://github.com/search?q=repo%3ALLNL%2Faxom+cmath&type=commits)

The problem appears to be an ordering dependency:
(a) msvc does not include defines from <math.h> like M_PI unless _USE_MATH_DEFINES is defined, and
(b) there is a guard in <cmath> that prevents it from loading <math.h> more than once.
(c) we define _USE_MATH_DEFINES in axom/config.h, but run into trouble when something brings in <cmath> before we define _USE_MATH_DEFINES. Specifically here gtest was included before axom/config.hpp and it pulls in <cmath> internally in some of its files.

So, it looks like we (and our users) can either require axom/config.hpp to be included before anything else (or at least to define _USE_MATH_DEFINES) and be diligent about this (and hope for the best), or we can include <math.h> (instead of (<cmath>) after including axom/config.hpp.
I don't think the former is a realistic/reasonable thing to expect of users, so I opted for the latter.

Can you add a note why we are using the .h header since that is not a normal thing to do in our code?

@white238 -- would you like me to add a comment to each #include <math.h> or do you think one would do?
Looks like we have about 20 of them -- https://github.com/search?q=repo%3ALLNL%2Faxom%20math.h&type=code

That seems like a lot. I am not sure I care that much.

Could this be solved with a clang-query check to enforce we always include our config first?

... and about 60 #include <cmath> -- https://github.com/search?q=repo%3ALLNL%2Faxom+cmath&type=code
(minus a few from this PR)!

(I only changed the ones that MSVC complained about due to a missing M_PI define)

Could this be solved with a clang-query check to enforce we always include our config first?

Not sure. I suspect that would be more challenging than a few manual changes since msvc lets us know when M_PI is not defined.

My preference would be to #include <cmath> and require axom/config.hpp to be first---just advertise it really well. In the end, it seems that we have to choose which muck heap we step in. (Kenny, note that I did approve the PR. : ) I'm not asking for changes.)

Thanks everyone. Since there's not yet a clear resolution and the code's already tested on all platforms including EAS3, I'm going to merge this as is and we can update the cmath vs. math.h includes in a follow-up PR, as necessary/desired.

kennyweiss added 3 commits June 16, 2023 19:52

Updates blt submodule to blt@0.5.3

48fe571

Removes axom/core_array helper functions from axom namespace

5763a94

Bugfix: Template type for axom::Array needs an operator<<() overload

d02d900

kennyweiss added bug Something isn't working Testing Issues related to testing Axom maintenance Issues related to code maintenance labels Jun 19, 2023

kennyweiss added this to the April 2023 Release milestone Jun 19, 2023

kennyweiss commented Jun 19, 2023

View reviewed changes

kennyweiss added 2 commits June 19, 2023 21:24

Bugfix for MSVC -- need to include math.h instead of cmath for `M…

47886e5

…_PI`

kennyweiss commented Jun 20, 2023

View reviewed changes

kennyweiss added 3 commits June 20, 2023 23:37

Fixes typo in device-guarded code

8f0a97a

Updates RELEASE_NOTES

f2bcac8

kennyweiss marked this pull request as ready for review June 21, 2023 06:52

kennyweiss requested review from white238, cyrush, BradWhitlock, rhornung67, publixsubfan, jcs15c, agcapps and bmhan12 June 21, 2023 06:53

kennyweiss self-assigned this Jun 21, 2023

kennyweiss commented Jun 21, 2023

View reviewed changes

agcapps approved these changes Jun 21, 2023

View reviewed changes

white238 reviewed Jun 21, 2023

View reviewed changes

white238 approved these changes Jun 21, 2023

View reviewed changes

rhornung67 approved these changes Jun 22, 2023

View reviewed changes

kennyweiss merged commit ce44b9a into develop Jun 22, 2023

kennyweiss deleted the feature/kweiss/update-blt branch June 22, 2023 23:58

kennyweiss mentioned this pull request Jul 13, 2023

BLT v0.5.3 has some build errors related to googletest LLNL/blt#649

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates and bugfixes for blt@0.5.3 #1120

Updates and bugfixes for blt@0.5.3 #1120

kennyweiss commented Jun 19, 2023 •

edited

Loading

kennyweiss Jun 19, 2023

kennyweiss Jun 19, 2023

agcapps Jun 21, 2023

kennyweiss Jun 22, 2023

kennyweiss Jun 20, 2023

kennyweiss Jun 20, 2023

kennyweiss Jun 20, 2023

kennyweiss Jun 21, 2023 •

edited

Loading

agcapps left a comment

white238 Jun 21, 2023

rhornung67 Jun 21, 2023

kennyweiss Jun 22, 2023 •

edited

Loading

kennyweiss Jun 22, 2023

white238 Jun 22, 2023

kennyweiss Jun 22, 2023 •

edited

Loading

kennyweiss Jun 22, 2023

agcapps Jun 22, 2023

kennyweiss Jun 22, 2023


		#include <cmath>

Updates and bugfixes for blt@0.5.3 #1120

Updates and bugfixes for blt@0.5.3 #1120

Conversation

kennyweiss commented Jun 19, 2023 • edited Loading

Summary

TODO:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyweiss Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

agcapps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyweiss Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyweiss Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyweiss commented Jun 19, 2023 •

edited

Loading

kennyweiss Jun 21, 2023 •

edited

Loading

kennyweiss Jun 22, 2023 •

edited

Loading

kennyweiss Jun 22, 2023 •

edited

Loading