Windows tests #2

krasznaa · 2023-10-09T20:35:53Z

Since I quickly realized while working with Detray that this project's code will still need some work for Windows compatibility, I went ahead and set up the basic ability to perform a Windows based build. But this just opened can of worms... 😦

I tried to set up the same sort of header tests for this project that we have in the other R&D projects as well. Which would make sure that the headers include everything that they need, and that the CMake targets are configured correctly. But this quickly resulted in:

[ 18%] Building CXX object lib/core/CMakeFiles/test_core_covfie_core_backend_transformer_backup_hpp.dir/CMakeFiles/test_core_covfie_core_backend_transformer_backup_hpp.cpp.o
In file included from /data/ssd-1tb/projects/covfie/build/lib/core/CMakeFiles/test_core_covfie_core_backend_transformer_backup_hpp.cpp:1:
/data/ssd-1tb/projects/covfie/covfie/lib/core/covfie/core/backend/transformer/backup.hpp: In static member function ‘static void covfie::backend::backup<_backend_t>::owning_data_t::write_binary(std::ostream&, const covfie::backend::backup<_backend_t>::owning_data_t&)’:
/data/ssd-1tb/projects/covfie/covfie/lib/core/covfie/core/backend/transformer/backup.hpp:124:49: error: invalid use of member ‘covfie::backend::backup<_backend_t>::owning_data_t::m_min’ in static member function
  124 |                 reinterpret_cast<const char *>(&m_min), sizeof(decltype(m_min))
      |                                                 ^~~~~
/data/ssd-1tb/projects/covfie/covfie/lib/core/covfie/core/backend/transformer/backup.hpp:139:50: note: declared here
  139 |         typename contravariant_input_t::vector_t m_min, m_max;
      |                                                  ^~~~~
/data/ssd-1tb/projects/covfie/covfie/lib/core/covfie/core/backend/transformer/backup.hpp:127:49: error: invalid use of member ‘covfie::backend::backup<_backend_t>::owning_data_t::m_max’ in static member function
  127 |                 reinterpret_cast<const char *>(&m_max), sizeof(decltype(m_max))
      |                                                 ^~~~~
/data/ssd-1tb/projects/covfie/covfie/lib/core/covfie/core/backend/transformer/backup.hpp:139:57: note: declared here
  139 |         typename contravariant_input_t::vector_t m_min, m_max;
      |                                                         ^~~~~
/data/ssd-1tb/projects/covfie/covfie/lib/core/covfie/core/backend/transformer/backup.hpp:130:49: error: invalid use of member ‘covfie::backend::backup<_backend_t>::owning_data_t::m_default’ in static member function
  130 |                 reinterpret_cast<const char *>(&m_default),
      |                                                 ^~~~~~~~~
/data/ssd-1tb/projects/covfie/covfie/lib/core/covfie/core/backend/transformer/backup.hpp:140:47: note: declared here
  140 |         typename covariant_output_t::vector_t m_default;
      |                                               ^~~~~~~~~
make[2]: *** [lib/core/CMakeFiles/test_core_covfie_core_backend_transformer_backup_hpp.dir/build.make:76: lib/core/CMakeFiles/test_core_covfie_core_backend_transformer_backup_hpp.dir/CMakeFiles/test_core_covfie_core_backend_transformer_backup_hpp.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:524: lib/core/CMakeFiles/test_core_covfie_core_backend_transformer_backup_hpp.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

Which is a fundamental enough issue that I'd not want to sort it out in this PR. (Is that file used anywhere? I guess not.)

But at least I went through the unit tests, and made sure that those would all compile without warnings with MSVC. And to think that I was really just trying to replace uint with unsigned int when I started with this all...

P.S. The CUDA test on Windows just refuses to build. 😦 But I gave up on it for today...

Switched the code to using std::filesystem instead of boost::filesystem, and taught the project how to build GoogleTest for itself if needed.

Introduced tests for all of the public headers in those two libraries, and made sure that those and the existing unit tests would compile without warnings using MSVC.

stephenswat

Looks in order, some minor questions though. But I am not against any of this. I'll take a look at the issue you pointed out tomorrow (i.e. today).

stephenswat · 2023-10-09T22:54:00Z

lib/core/covfie/core/algebra/vector.hpp

-            (std::is_scalar_v<Args> && ...) &&
-                (std::is_convertible_v<Args, T> && ...) && sizeof...(Args) == N,
+            std::conjunction_v<std::is_scalar<Args>...> &&
+                std::conjunction_v<std::is_convertible<Args, T>...> &&
+                sizeof...(Args) == N,


What's the problem here? Does MSVC not like fold expressions? 😕

Similar situation here. In C++17 mode this fails with:

C:\Users\krasz\ATLAS\covfie\covfie\lib\core\covfie/core/algebra/vector.hpp(45,59): error C2059: syntax error: '...' [C:\Users\krasz\ATLAS\covfie\build\tests\core\test_core.vcxproj] C:\Users\krasz\ATLAS\covfie\covfie\lib\core\covfie/core/algebra/vector.hpp(63,2): message : see reference to class template instantiation 'covfie::algebra::vector<N,T,I>' being compiled [C:\Users\krasz\ATLAS\covfie\build\tests\core\test_core.vcxproj] C:\Users\krasz\ATLAS\covfie\covfie\lib\core\covfie/core/algebra/vector.hpp(48,19): error C2059: syntax error: '=' [C:\Users\krasz\ATLAS\covfie\build\tests\core\test_core.vcxproj] C:\Users\krasz\ATLAS\covfie\covfie\lib\core\covfie/core/algebra/vector.hpp(50,9): error C2059: syntax error: ':' [C:\Users\krasz\ATLAS\covfie\build\tests\core\test_core.vcxproj] C:\Users\krasz\ATLAS\covfie\covfie\lib\core\covfie/core/algebra/vector.hpp(50,1): error C2334: unexpected token(s) preceding ':'; skipping apparent function body [C:\Users\krasz\ATLAS\covfie\build\tests\core\test_core.vcxproj] C:\Users\krasz\ATLAS\covfie\covfie\lib\core\covfie/core/algebra/vector.hpp(63): error C2143: syntax error: missing '>' before ';' [C:\Users\krasz\ATLAS\covfie\build\tests\core\test_core.vcxproj] C:\Users\krasz\ATLAS\covfie\covfie\lib\core\covfie/core/algebra/vector.hpp(63): error C2059: syntax error: '>' [C:\Users\krasz\ATLAS\covfie\build\tests\core\test_core.vcxproj] C:\Users\krasz\ATLAS\covfie\covfie\lib\core\covfie/core/algebra/vector.hpp(63): error C2238: unexpected token(s) preceding ';' [C:\Users\k rasz\ATLAS\covfie\build\tests\core\test_core.vcxproj] C:\Users\krasz\ATLAS\covfie\covfie\lib\core\covfie/core/algebra/vector.hpp(63): fatal error C1201: unable to continue after syntax error in class template definition [C:\Users\krasz\ATLAS\covfie\build\tests\core\test_core.vcxproj]

But in C++20 mode it accepts the original code as well.

Since we were talking about this exact formalism in acts-project/algebra-plugins#95 earlier yesterday, I tried that, and that worked with both C++ standards...

That's a shame, and quite odd since fold expressions are a C++17 feature. Again, I don't mind too much but it is a bit annoying that MSVC seems to refuse completely valid C++, and also I think the fold syntax is a lot clearer than the conjunction syntax... 😦

lib/core/covfie/core/field_view.hpp

lib/core/covfie/core/field.hpp

stephenswat · 2023-10-09T23:02:03Z

tests/CMakeLists.txt

+option(COVFIE_SETUP_GOOGLETEST "Set up the GoogleTest targets explicitly"
+    TRUE)
+option(COVFIE_USE_SYSTEM_GOOGLETEST
+    "Pick up an existing installation of GoogleTest from the build environment"
+    TRUE)
+if(COVFIE_SETUP_GOOGLETEST)
+    if(COVFIE_USE_SYSTEM_GOOGLETEST)
+        find_package(GTest CONFIG REQUIRED)
+    else()
+        add_subdirectory(googletest)
+    endif()
+endif()


Is there any good reason for any project importing this to ever need to build the tests? I'm okay with the COVFIE_USE_SYSTEM_GOOGLETEST flag but I'm not sure if the COVFIE_SETUP_GOOGLETEST flag would ever be useful?

It is indeed not too likely that we'll want to build the covfie unit tests when building covfie as part of a downstream project. But on the off chance that we ever want to do that, it's good to have the theoretical ability.

The logic with the two cache variables is the same that we use in all the other R&D projects. So I'm keen to use it in the same way here as well.

tests/utils/CMakeLists.txt

stephenswat · 2023-10-09T23:15:56Z

tests/utils/tmp_file.cpp

+    char fname[L_tmpnam];
+    char * dummy = std::tmpnam(fname);
+    (void)dummy;
+    return std::filesystem::temp_directory_path() /
+           std::filesystem::path(fname);


Yeah this is one of the main reasons I used Boost, temporary file handling in std::filesystem is awful... But I guess it's a price we can pay for removing the dependency... I'm not against it per se. But if you want to remove Boost completely it will also need to be removed from the benchmarks.

Oh, I do hate this. 😦 The Boost version looks a bit more elegant, though that's not any safer either. So I understand why they left this out of the standard implementation. (Not to give people the false sense that using that function would result in safe code.)

As long as Boost is only used for this one purpose in the benchmarks and examples as well, then sure, I can look into that.

Boost is used for way more than just for temporary file creation in the benchmarks and the examples. I won't rewrite all of those to Boost-less C++ myself...

For Detray, which is my current objective, that is not really needed anyway.

stephenswat · 2023-10-09T23:21:45Z

Also it might be useful to add a Windows CI job.

krasznaa · 2023-10-10T07:25:09Z

I changed my mind... Let's just keep the scope of this PR as is. I'm afraid of making it into an unreviewable mess by the end.

It does mean adding some commented lines into lib/core/CMakeLists.txt for now, but those will hopefully get activated later today. 🤔

krasznaa · 2023-10-10T09:38:20Z

We'll discuss this in person, but just to give you a heads' up: What makes the CUDA test fail is something that I believe would fail with MSVC by itself as well, we just don't test for it at the moment. It's the fact that size_t is not unsigned long on Windows. And as such std::array<size_t, N> can't be auto-converted into std::array<unsigned long, N> and vice versa. 😦

Instead of adding explicit conversions between these in a bunch of places, I think we'll just need to make the code use one of those 2 types consistently everywhere.

stephenswat · 2023-10-12T12:27:06Z

I've factored the remainder of this PR into #13 and #14. Thanks a lot!

krasznaa added 2 commits October 9, 2023 22:09

Made the tests work without Boost.

84c12e7

Switched the code to using std::filesystem instead of boost::filesystem, and taught the project how to build GoogleTest for itself if needed.

Made the core and cpu libraries work with MSVC.

ae2678c

Introduced tests for all of the public headers in those two libraries, and made sure that those and the existing unit tests would compile without warnings using MSVC.

stephenswat reviewed Oct 9, 2023

View reviewed changes

krasznaa marked this pull request as ready for review October 10, 2023 07:23

krasznaa mentioned this pull request Oct 10, 2023

Core header fixes #3

Closed

stephenswat mentioned this pull request Oct 10, 2023

Make the core and CPU libraries work with MSVC #8

Merged

stephenswat changed the title ~~Windows Tests, main branch (2023.10.09.)~~ Windows tests Oct 11, 2023

stephenswat added bug Something isn't working core Issues and changes related to the core library cpu Issues and improvements to the CPU library tests Improvements or additions to tests refactor Refactoring or cleanup of existing code labels Oct 11, 2023

stephenswat closed this Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows tests #2

Windows tests #2

krasznaa commented Oct 9, 2023

stephenswat left a comment

stephenswat Oct 9, 2023

krasznaa Oct 10, 2023

stephenswat Oct 10, 2023

stephenswat Oct 9, 2023

krasznaa Oct 10, 2023

stephenswat Oct 9, 2023

krasznaa Oct 10, 2023

krasznaa Oct 10, 2023

stephenswat commented Oct 9, 2023

krasznaa commented Oct 10, 2023

krasznaa commented Oct 10, 2023

stephenswat commented Oct 12, 2023

Windows tests #2

Windows tests #2

Conversation

krasznaa commented Oct 9, 2023

stephenswat left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenswat commented Oct 9, 2023

krasznaa commented Oct 10, 2023

krasznaa commented Oct 10, 2023

stephenswat commented Oct 12, 2023