repair builds that check for debug-release compatibility

[jll63]

12 lost the "hybridness" of the DebugRelease and ReleaseDebug builds, making them useless to detect binary compatibility problems.

[derpda]

Ahh now I understand what the purpose of ReleaseDebug and DebugRelease is! In CMake, it is generally problematic to do things like if (CMAKE_BUILD_TYPE STREQUAL "Debug") (or the equivalent if("${YOMM2_BUILD_TYPE}" STREQUAL "DebugRelease") later on). This works when using make/ninja, but will not work with IDEs like VisualStudio and Xcode (see here and in the first answer of this post)

Then again, using different build types for parts of the project will probably not work with MultiBuild tools like VS or Xcode anyway. I don't have much experience with VisualStudio (and none with Xcode) so I can't say much about this.

Since it seems to me that we mainly care about the NDEBUG preprocessor definition, we could just define/undefine that variable.

target_compile_options(example PRIVATE "$<$<CONFIG:ReleaseDebug>:-UNDEBUG>")
target_compile_definitions(example PRIVATE "$<$<CONFIG:DebugRelease>:NDEBUG>")

I don't have time until later today/tomorrow so I haven't tested this yet, but this might be an option!

[jll63]

Ahh now I understand what the purpose of ReleaseDebug and DebugRelease is!

Yeah I introduced these with the check for non-registered classes.

I suspected that it was quite a hack but it seemed to work. Even then, it's a bit silly, because it builds everything twice. It would be more elegant to build the library in both modes, build the tests and the examples in both modes as well, and link each combination. What is the best way to arrange this with cmake, I haven't given it a lot of thought. If you want to work on this (no hurry) you are very welcome.

Then again, using different build types for parts of the project will probably not work with MultiBuild tools like VS or Xcode anyway. I don't have much experience with VisualStudio (and none with Xcode) so I can't say much about this.

Now from a practical point of view, the DebugRelease and ReleaseDebug "modes" are not documented nor meant to be used by a "normal user". They are just for testing compatibility, typically under Travis. Maybe there is a way of hiding them when generating anything other than a make or nmake file?

[derpda]

I think I have found a good solution to this. It involves the find_or_download_package macro I added during my CMake changes. It is normally used to (as the name suggests) download and build dependencies. In this case we can use it to build yomm2 in an independent configuration from the examples.

I have a working version now, should I make my own PR or can I push the repair-mixed-builds branch?

[derpda]

Also, there were two things I noticed while working on the code that aren't directly related to this PR:

In blackbox.cpp two tests have the same name ("states"), resulting in failure to execute the test. Renaming that test makes it work again. We could e ither fix this in this PR or add a comment for later.

Second, and this one is all on me, the boost dependencies are not downloaded automatically if they don't exist. I had a user of my library complain that while my library takes care of its dependencies (including boost), yomm2 does not so he couldn't build without getting boost first. This isn't a major issue, but fully automating the dependency installation might be worthwhile.

[jll63]

I think I have found a good solution to this. It involves the find_or_download_package macro I added during my CMake changes. It is normally used to (as the name suggests) download and build dependencies. In this case we can use it to build yomm2 in an independent configuration from the examples.

I have a working version now, should I make my own PR or can I push the repair-mixed-builds branch?

I'd prefer that you push to this branch (because we have the discussion here FTR) but I don't think it's possible to enable this in this situation. I think it requires the repo to belong to an organisation, and that you are a member. At work we do that but we use our private GitHub Enterprise and admins who take care of setting it up. But if you know how to do it, please tell me or point me to the relevant doc.

I created a yomm2 organisation but I cannot add the repo to it without transferring it I think.

Otherwise, you could probably create a PR on my branch and send me that PR. But in that case it is much simpler for you to create a new branch and me to close this PR and delete my branch.

[jll63]

In blackbox.cpp two tests have the same name ("states"), resulting in failure to execute the test. Renaming that test makes it work again. We could e ither fix this in this PR or add a comment for later.

I never noticed this. How do you run the tests? I use the scripts in dev. Indeed there are two states testcases albeit in different namespaces. I guess it would be a problem if one tries to select which tests to run.

Please create a small PR for this. I think it's good for the project's statistics and visibility if it has more PRs.

Second, and this one is all on me, the boost dependencies are not downloaded automatically if they don't exist. I had a user of my library complain that while my library takes care of its dependencies (including boost), yomm2 does not so he couldn't build without getting boost first. This isn't a major issue, but fully automating the dependency installation might be worthwhile.

This sounds appealing indeed. Make it easy for a prospective user to quickly and easily get to the point where he can run the examples and fiddle with them.

However, we need to make sure that, if Boost is installed, that one is picked.

This definitely deserves its own PR ;-)

[derpda]

Sorry for the late reply, it's been a busy week!

I never noticed this. How do you run the tests?

I ran them with both ctest and by simply executing the created binary. For both I get the same error:

Test setup error: test unit with name 'states' registered multiple times in the test suite 'yomm2'

I'll make a small PR for this!

However, we need to make sure that, if Boost is installed, that one is picked.

This can be done (and I am in fact doing it in our library) by giving the path of the downloaded dependency to find_package as follows

ind_package(${PACKAGE} REQUIRED NO_DEFAULT_PATH PATHS "${DEPENDENCY_INSTALL_PREFIX}")

I'll make this PR as well =)

[derpda]

I'd prefer that you push to this branch (because we have the discussion here FTR) but I don't think it's possible

I think you can grant me push access by going to Settigs (on this repository, next to "Insights") -> Manage Access -> Invite a Collaborator. I haven't tried this yet, but this StackOverflow post suggests it works.

[jll63]

I'd prefer that you push to this branch (because we have the discussion here FTR) but I don't think it's possible

I think you can grant me push access by going to Settigs (on this repository, next to "Insights") -> Manage Access -> Invite a Collaborator. I haven't tried this yet, but this StackOverflow post suggests it works.

OK, I sent the invite. I think this gives you the right to push to master though. Don't do that, eh?

[derpda]

Thank you! I'll get on it when I'm back home.

I think this gives you the right to push to master though. Don't do that, eh?

I think you can set restrictions on branches (such as master) so that only admins can push to it!

[cppguru]

I want to push to master. It makes me feel all powerful. Like Q.

[jll63]

I think you can set restrictions on branches (such as master) so that only admins can push to it!

I think you can do that only for an organization, not a repo. I grabbed https://github.com/yomm2 just in case we want to use it someday.

[derpda]

Restricting push access is possible in regular public repositories as well if I understand this doc page correctly. It also shows how an administrator can set up such restrictions!

[jll63]

Restricting push access is possible in regular public repositories as well if I understand this doc page correctly. It also shows how an administrator can set up such restrictions!

Drat, I google with the wrong terms and the answer was under my nose all along. Thanks!

[derpda]

I have pushed my changes to this branch! Essentially what can now be done is

mkdir build && cd build
cmake ../cmake/ReleaseDebug
make

which will build yomm2 in Release and tests/examples in Debug. Use

cmake ../cmake/ReleaseDebug

for the opposite.

I haven't added the CTest part yet as I think that could use some refactoring (mainly moving the add_test calls to the sub-directory CMakeLists).

Everything is working on my machine, so let me know any feedback and we can finish this PR up! =)

[jll63]

My turn, sorry for the delay ;-) Been busy, lazy, on GF duties etc.

I get the following error (cmake version 3.10.2):

jll@marvin:~/dev/yomm2/build$ cmake ../cmake/ReleaseDebug
-- The C compiler identification is GNU 7.5.0
-- The CXX compiler identification is GNU 7.5.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Package "YOMM2" not found in system.
-- Downloading dependency "YOMM2" and building from source.
Scanning dependencies of target yomm2_build
[ 12%] Creating directories for 'yomm2_build'
[ 25%] No download step for 'yomm2_build'
[ 37%] No patch step for 'yomm2_build'
[ 50%] No update step for 'yomm2_build'
[ 62%] Performing configure step for 'yomm2_build'
-- The C compiler identification is GNU 7.5.0
-- The CXX compiler identification is GNU 7.5.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Boost version: 1.65.1
-- Using Boost libraries from /usr/include
-- Configuring done
-- Generating done
-- Build files have been written to: /home/jll/dev/yomm2/build/YOMM2-download/yomm2
[ 75%] Performing build step for 'yomm2_build'
Scanning dependencies of target yomm2
[ 50%] Building CXX object src/CMakeFiles/yomm2.dir/yomm2.cpp.o
[100%] Linking CXX static library libyomm2.a
[100%] Built target yomm2
[ 87%] Performing install step for 'yomm2_build'
[100%] Built target yomm2
Install the project...
-- Install configuration: "Release"
-- Installing: /home/jll/dev/yomm2/cmake/ReleaseDebug/dependencies/lib/cmake/YOMM2/YOMM2Targets.cmake
-- Installing: /home/jll/dev/yomm2/cmake/ReleaseDebug/dependencies/lib/cmake/YOMM2/YOMM2Targets-release.cmake
-- Installing: /home/jll/dev/yomm2/cmake/ReleaseDebug/dependencies/lib/cmake/YOMM2/YOMM2Config.cmake
-- Installing: /home/jll/dev/yomm2/cmake/ReleaseDebug/dependencies/lib/cmake/YOMM2/YOMM2ConfigVersion.cmake
-- Up-to-date: /home/jll/dev/yomm2/cmake/ReleaseDebug/dependencies/include/yorel
-- Up-to-date: /home/jll/dev/yomm2/cmake/ReleaseDebug/dependencies/include/yorel/yomm2
-- Up-to-date: /home/jll/dev/yomm2/cmake/ReleaseDebug/dependencies/include/yorel/yomm2/runtime.hpp
-- Up-to-date: /home/jll/dev/yomm2/cmake/ReleaseDebug/dependencies/include/yorel/yomm2/cute.hpp
-- Up-to-date: /home/jll/dev/yomm2/cmake/ReleaseDebug/dependencies/include/yorel/yomm2.hpp
-- Installing: /home/jll/dev/yomm2/cmake/ReleaseDebug/dependencies/lib/libyomm2.a
[100%] Completed 'yomm2_build'
[100%] Built target yomm2_build
-- Boost version: 1.65.1
CMake Error at CMakeLists.txt:18 (add_library):
  add_library cannot create ALIAS target "yomm2" because target
  "YOMM2::yomm2" is IMPORTED.

CMake Error at CMakeLists.txt:20 (get_target_property):
  get_target_property() called with non-existent target "yomm2".

Test: 
CMake Error at /home/jll/dev/yomm2/examples/CMakeLists.txt:37 (get_target_property):
  get_target_property() called with non-existent target "yomm2".

-- Configuring incomplete, errors occurred!
See also "/home/jll/dev/yomm2/build/CMakeFiles/CMakeOutput.log".

Same problem with DebugRelease.

[derpda]

And once again my turn to apologize for the delay - my research kept me very busy.

I did not notice that issue because I am working with a newer CMake version. Creating an ALIAS of an imported target was introduced around 3.11, so that might be a bit too recent of a version to require...

Let me think of something!

[derpda]

I have found a solution to the issue, but I am not sure how much you'll like it.

The central issue is that imported CMake targets generally live in a namespace (just like stuff from the stdlib in c++). When we normally compile yomm2 and its examples, we link the examples to yomm2 using, well, yomm2.

However, for the ReleaseDebug and DebugRelease builds we import yomm2, and it thus lives in the namespaceYOMM2:: (the name of the CMake project, line one in the top-level CMakeLists.txt). Thus, the linking will not work since yomm2 doesn't exist, only YOMM2::yomm2.

Normally I would define an ALIAS target yomm2 pointing to YOMM2::yomm2, but CMake <3.11 does not support that for imported targets. So I figure we do the opposite, and create a an alias target YOMM2::yomm2 pointing to yomm2 and use YOMM2::yomm2 whenever we want to link to yomm2 in the CMakeLists.txt for the tests and examples.

I tested this with CMake 3.0 (which is the minimum we require) and was able to do all four build types and hope this will work for you.

PS: I also found a small bug in the namespaces.cpp example: #include <string> is needed earlier, and I have also made a small commit fixing this.

[jll63]

Great, it works now. I tested by re-introducing the #if that caused a link failure, and it was detected.

I will need to rework the Travis script a bit then we are good. I'll look into it tomorrow.

Thanks for working on this.

PS: I also found a small bug in the namespaces.cpp example: #include <string> is needed earlier, and I have also made a small commit fixing this.

Nice. Ah, indirect accidental includes...

[jll63]

Ah, there is another problem. I tried to enable ctest in the hybrid builds. This is necessary because in the past binary compatibility was broken (see https://github.com/jll63/yomm2/issues/4). I tried to do it myself in https://github.com/jll63/yomm2/pull/19/commits/82d85063df0bca94243fd1745e5c090f1521cb95, but there is a problem with the containers example. It builds but ctest doesn't find it (see below). Can you take a look?

CONFIG=DebugRelease HYBRID=1 dev/travis
mkdir: cannot create directory ‘build’: File exists
-- Tests enabled
-- Configuring done
-- Generating done
-- Build files have been written to: /home/jll/dev/yomm2/build
[  6%] Built target next
[ 12%] Built target dl_shared
[ 18%] Built target adventure
[ 25%] Built target synopsis
[ 31%] Built target lab
[ 37%] Built target namespaces
[ 43%] Built target matrix
[ 50%] Built target whitebox
[ 56%] Built target asteroids
[ 68%] Built target accept_no_visitors
[ 68%] Built target blackbox
[ 93%] Built target containers
[100%] Built target dl_main
Test project /home/jll/dev/yomm2/build
    Start 1: synopsis
1/7 Test #1: synopsis .........................   Passed    0.00 sec
    Start 2: matrix
2/7 Test #2: matrix ...........................   Passed    0.00 sec
    Start 3: accept_no_visitors
3/7 Test #3: accept_no_visitors ...............   Passed    0.00 sec
    Start 4: adventure
4/7 Test #4: adventure ........................   Passed    0.00 sec
    Start 5: next
5/7 Test #5: next .............................   Passed    0.00 sec
    Start 6: asteroids
6/7 Test #6: asteroids ........................   Passed    0.00 sec
    Start 7: containers
Could not find executable containers
Looked in the following places:
containers
containers
Release/containers
Release/containers
Debug/containers
Debug/containers
MinSizeRel/containers
MinSizeRel/containers
RelWithDebInfo/containers
RelWithDebInfo/containers
Deployment/containers
Deployment/containers
Development/containers
Development/containers
Unable to find executable: containers
7/7 Test #7: containers .......................***Not Run   0.00 sec

86% tests passed, 1 tests failed out of 7

Total Test time (real) =   0.07 sec

The following tests FAILED:
      7 - containers (Not Run)
Errors while running CTest

[jll63]

Also I think we lost a couple of improvements:

82d8506 | * origin/repair-mixed-builds repair-mixed-builds-travis ctest for hybrid builds
7a96427 | * travis for hybrid builds
8765656 | * Small include fix in example
7090a4b | * Fix for older CMake
c366f46 | | * derpda/boost-download Improvements to find_and_download  <---------------------------------
b0d9b1d | | * derpda/test-name-fix Fix: Test names no longer identical <---------------------------------
        | |/  
09d8921 | * Hybrid build instruction removed from main cmake
f5cfcaf | * DebugRelease, ReleaseDebug scripts added
6e6085c | * Improvements to dependency download script
c219026 | * repair builds that check for debug-release compatibility
        |/  

[derpda]

Rebasing sounds like needless work to change two lines, so I simply made a new commit to the same effect on this branch for the test names. I will delete the test-name-fix branch.

About Ctest: I totally forgot to rework that part of the code. I have a few commits coming in for that now. Ctest wasn't even enabled by default for the DebugRelease and ReleaseDebug builds, but the whole point of those is to test so Ctest will now always be enabled for those two. All add_test calls are moved to subdirectory CmakeLists.txt files.

[derpda]

I am also seeing a segfault when I run benchmarks.

2020-10-02T13:18:21+09:00
Running ./tests/benchmarks
Run on (8 X 1901 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 6144 KiB (x1)
Load Average: 1.15, 1.11, 0.87
***WARNING*** CPU scaling is enabled, the benchmark real time measurements may be noisy and will incur extra overhead.
----------------------------------------------------------------------------------
Benchmark                                        Time             CPU   Iterations
----------------------------------------------------------------------------------
virtual function call                         34.3 ns         34.2 ns     21433354
uni-method call                               51.5 ns         51.4 ns     12858068
double dispatch                               33.9 ns         33.8 ns     20692485
multi-method call                             68.4 ns         68.3 ns     10513494
virtual function call                         31.4 ns         31.4 ns     21163999
uni-method call (virtual inheritance)         47.4 ns         47.3 ns     14762466
double dispatch(virtual inheritance)          48.0 ns         48.0 ns     13445684
multi-method call (virtual inheritance)       66.3 ns         66.2 ns     10512523
virtual function call (hash info in gv)       32.3 ns         32.3 ns     21979804
==8459== Invalid read of size 8
==8459==    at 0x134B0E: hash_in_gv_ns::call_uni_method(double, hash_in_gv_ns::matrix const&) (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==    by 0x12B22B: hash_in_gv_ns::uni_method(benchmark::State&) (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==    by 0x17DE37: benchmark::internal::BenchmarkInstance::Run(unsigned long, int, benchmark::internal::ThreadTimer*, benchmark::internal::ThreadManager*) const (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==    by 0x160CA1: benchmark::internal::(anonymous namespace)::RunInThread(benchmark::internal::BenchmarkInstance const*, unsigned long, int, benchmark::internal::ThreadManager*) (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==    by 0x161632: benchmark::internal::RunBenchmark(benchmark::internal::BenchmarkInstance const&, std::vector<benchmark::BenchmarkReporter::Run, std::allocator<benchmark::BenchmarkReporter::Run> >*) (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==    by 0x13F523: benchmark::RunSpecifiedBenchmarks(benchmark::BenchmarkReporter*, benchmark::BenchmarkReporter*) (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==    by 0x128826: main (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==  Address 0x8 is not stack'd, malloc'd or (recently) free'd
==8459== 
==8459== 
==8459== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==8459==  Access not within mapped region at address 0x8
==8459==    at 0x134B0E: hash_in_gv_ns::call_uni_method(double, hash_in_gv_ns::matrix const&) (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==    by 0x12B22B: hash_in_gv_ns::uni_method(benchmark::State&) (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==    by 0x17DE37: benchmark::internal::BenchmarkInstance::Run(unsigned long, int, benchmark::internal::ThreadTimer*, benchmark::internal::ThreadManager*) const (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==    by 0x160CA1: benchmark::internal::(anonymous namespace)::RunInThread(benchmark::internal::BenchmarkInstance const*, unsigned long, int, benchmark::internal::ThreadManager*) (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==    by 0x161632: benchmark::internal::RunBenchmark(benchmark::internal::BenchmarkInstance const&, std::vector<benchmark::BenchmarkReporter::Run, std::allocator<benchmark::BenchmarkReporter::Run> >*) (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==    by 0x13F523: benchmark::RunSpecifiedBenchmarks(benchmark::BenchmarkReporter*, benchmark::BenchmarkReporter*) (in /home/peter/dev/research/yomm2/build/tests/benchmarks)
==8459==    by 0x128826: main (in /home/peter/dev/research/yomm2/build/tests/benchmarks)

Not sure if that is related to this PR...

[derpda]

The commit on the boost-download, branch is not necessary for this PR. I ran into some issues with downloading boost since their CMake support is still very limited. Unless upstream boost gets a little nicer, we probably won't be able to use automatic download for that. Luckily, boost can easily be installed on most systems and is available on most super computers, so it's not a big deal imo.

[derpda]

Force push was needed after rebase to master. I probably should have asked if you have commits lined up first...

[jll63]

Force push was needed after rebase to master. I probably should have asked if you have commits lined up first...

No problem, I am used to juggling commits and HEADs. We do that a lot at work.

That being said, I am not a big fan of rebase. Being a former Darcs user and fan, I don't mind non-linear histories. And, being a fan of Orwell's 1984, "rewriting history" doesn't sound nice ;-)

[jll63]

Not sure if that is related to this PR...

It's not. I had forgotten about this. I don't see that trace because it is eaten by dev/run-benchmarks. It is a bit of research related to hot-swapping the global data in situations where one thread calls dlopen and update_methods while method calls are in flight. I think I broke it when I introduced the check for unregistered classes (the issue I quoted above). I should fix it or remove it.

[jll63]

Well it looks like we're good! Thanks again. Merging...