A way to register additional test targets and support .py only tests.

[iwanders]

Description

@rwgk asked for help on the CMake bullet point in this comment. I proposed some changes in the comment after that, moving it to a PR for easier discussion.

Changes in this PR are two-fold:

• Make it easier to register additional targets necessary to run a test.
• Support tests where a python file does not map to a single cpp file.

The first commit, solves the first problem. So before, there was special handling for PYBIND11_CROSS_MODULE_TESTS, PYBIND11_CROSS_MODULE_GIL_TESTS and PYBIND11_CLASS_SH_MODULE_LOCAL_TESTS in the new test @rwgk added in the smart_holder branch. I thought that that block, I initially thought the desire was to make that handling uniform. So what I made to handle that using common logic is a lookup where we can register additional targets if a particular test is present. Registering extra targets is done through the new tests_extra_targets macro. The first argument it takes (needles), are test that will be looked for in the PYTEST lists, if any of the needles is found, the additional targets specified in the second argument (additions) will be added. The registered mappings are stored in an emulated map called PYBIND11_TEST_EXTRA_TARGETS.

So, registration is done through tests_extra_targets, then, after all the filtering is performed on the tests, we iterate through the registered test->targets mapping (PYBIND11_TEST_EXTRA_TARGETS), check if any of the needles are found in the tests, if so we add the additional targets that are stored as values in the map.

The second commit addresses the actual concern @rwgk had with the new test addition not behaving nicely with the override and test filtering. So, the original CMakeLists file assumed that for each cpp file in PYBIND11_TEST_FILES, there would be a single .py file with the same name. This didn't play nicely with the new test being added, which has just a single python file, this broke the assumption that the big lists of tests is a list of cpp files for which a .py file exists. So, change made here is to have the big list of files use no extension when both a .py file AND a .cpp file is present. If an extension is specified this test is assumed to be stand alone, without a .py or .cpp counterpart of the same name. After filtering, the test list it split with python files going into the already existing PYTEST list and the PYBIND11_TEST_FILES variable is overwritten with the .cpp files.

To preserve backwards compatibility on the test filter, we made the filter function ignore any extensions. After all, the big lists of tests now contains either no extension, or .cpp or .py, but we don't want to break anyone's already existing filters. Then, this still broke the test override, so we fixed (third commit) that by instead of directly overriding PYBIND11_TEST_FILES with the override, we now instead match the override list against the test list (ignoring extensions again), and filter out anything that is NOT present in the override. This ensures that overrides specified with a .cpp file, but in the test list without an extension will still be respected as without an extension. This should make it fully backwards compatible, both for the filter flag and the override flag.

It adds a bit of code, but it's fully backwards compatible and should allow for more uniform handling of tests made up of .py, .cpp or both.

Suggested changelog entry:

* More uniform handling of test targets.

[iwanders]

@rwgk , as for:

From very rusty memory, my main worry is about running only selected tests. I have it in my mind that my change is breaking that feature. I'll look again asap to make that more concrete.

I think I see what you mean, looking at that specific point... the filtering seems to happen in this section above the addition of your new python test file, which happens on line 192. I'll think a bit about how best to solve this in a backwards compatible way.

[rwgk]

the filtering seems to happen in this section above

Yes, that's it! Thanks for digging it up.

Tagging @henryiii for awareness / comment.

I was thinking it may be best to change the behavior of PYBIND11_TEST_OVERRIDE, to work on a list of .py files instead of .cpp (because pytest also works on .py files, without any awareness of the .cpp dependencies). But obviously that's not backward compatible. @henryiii, how much of a concern is that? Related ideas:

• Support .cpp and .py (but I think that'll get too messy).
• Issue a helpful error message if .cpp encountered.

[iwanders]

Support .cpp and .py (but I think that'll get too messy).

That would be backwards compatible I think though? Not too sure where the distinction between cpp and python is needed. Maybe we can just do this and filter it into two lists before anything consumes it? I'll take a closer look this evening.

[henryiii]

Not following too closely yet, but the reason for filtering is to filter out cpp files that don't need to be built, allowing incremental/partial testing.

Long term I don't think we should do this at all, but rather have individual test targets for each test, and a "total" test target that runs all of them. That way, you could just build a single test and run a single test, or you could build/run all of them, and it would even reuse the existing builds.

I don't think backward compatibility is that important for test running, FWIW.

[iwanders]

So, I took a look at the 'real' issue. So, for my own understanding I wrote out what actually happens;

• PYBIND11_TEST_FILES holds cpp files.
  On this we apply filtering, we apply the defines and drop depending on async / cuda.

• Then, PYBIND11_PYTEST_FILES gets written from the cpp list, by doing a substitution cpp -> py.

• Then, we add test_class_sh_module_local.py to the PYTEST only.

• If we don't have eigen, we remove the eigen unit test from PYBIND11_TEST_FILES. But we keep it in the PYTEST flavour.

So, there's some 'special' handling for the new test file. Less than ideal. I think assuming a test has both a cpp and a py file is acceptable in this project, given what the goal is. Overall, it's pretty straightforward, we just need two lists, one with cpp files, one with Python files.

So, I still went the way of supporting both .cpp and .py files in the same list. When there's no extension specified, we assume that both a .cpp and a .py file will exist for the test. If an extension is specified, this is taken as-is and sorted into the appropriate list.

I did foresee some issues with this and the test filtering, and wanted to preserve backward compatibility for that. To achieve this I made the test filter function ignore file extensions. This is a macro so I ran into the argument issues around that, hence the ugly set(_TMP_ACTUAL_LIST) there...

I was thinking it may be best to change the behavior of PYBIND11_TEST_OVERRIDE, to work on a list of .py files instead of .cpp (because pytest also works on .py files, without any awareness of the .cpp dependencies)

So, despite my best efforts this is the one flag that is changed. Since this directly overrides the list, instead of acting as a 'whitelist' it was not easy to fix this. The only way would be intersecting this variable with the PYBIND11_TEST_FILES variable (and ignoring extensions again) to preserve backwards compatibility. That would mean duplicating the test filter function, and I really didn't feel like doing that. Additionally, I think the current setup allows you to build & run a unit test that is not even in the CMakeLists.txt file yet. So I didn't see any way to preserve that functionality. I updated the documentation for this change already, but it's something that may cause developers that are used to the old behaviour some frustration.

I've tested this with

``` cd build/ rm -rf * cmake ../pybind11/ -DPYBIND11_TEST_FILTER="test_class_sh_module_local.py" #~ cmake ../pybind11/ -DPYBIND11_TEST_FILTER="test_async" # this also works. cmake --build . --target check cd ../ ``` and ``` cd build/ rm -rf * cmake ../pybind11/ -DPYBIND11_TEST_OVERRIDE="test_stl;test_smart_ptr" cmake --build . --target check cd ../ ```

Both filter and override still seem to behave as expected.

Long term I don't think we should do this at all, but rather have individual test targets for each test, and a "total" test target that runs all of them. That way, you could just build a single test and run a single test, or you could build/run all of them, and it would even reuse the existing builds.

Agreed, the current is a bit odd, but doing that refactor is beyond my understanding of best practices around this unfortunately.

I don't think backward compatibility is that important for test running, FWIW.

I think preserving the filter functionality is important. There may be build systems out there that ignore tests that take long or are flaky. I know for sure that at my work we do that for some upstream packages. I'd say preserving backwards compatibility is preferred if possible, even when it comes to the build system.

@rwgk , could you check if the current version addresses your outstanding bullet point?

[rwgk]

Awesome, thanks a lot! I cannot meaningfully review the cmake details b/o lack of relevant background, but I see test_class_sh_module_local fits in very nicely with your approach.

I'm guessing the 50 failures due to a small bug?

What do you think about porting your infra-structure changes to the master branch? That would make it easier to maintain the smart_holder branch until it's ready for merging. I could (actually want to) take care of the smart_holder-specific cmake diffs.

[iwanders]

I'm guessing the 50 failures due to a small bug?

Well... there was supposed to be zero failures of course :D So turned out one of the phases in the CI test ran:

cmake -S . -B build3 -DPYBIND11_WERROR=ON -DDOWNLOAD_CATCH=ON -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=17 -DCMAKE_CXX_FLAGS="-DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT " -DPYBIND11_INTERNALS_VERSION=10000000 "-DPYBIND11_TEST_OVERRIDE=test_call_policies.cpp;test_gil_scoped.cpp;test_thread.cpp" 

Notice that PYBIND11_TEST_OVERRIDE, which was the only thing that I didn't preserve backwards compatibility with. So it would fail, since that now needed to be specified as test_call_policies;test_gil_scoped;test_thread, without the extensions. I considered modifying the workflow, but thought I might as well just fix it in a fully backwards compatible manner. So that's what I did, the test override now compares the override against the known lists of tests, and anything that's not in the override goes into the filter list, which conveniently happens after the override. So anything that's not in the override will be filtered. If the override is empty we don't add anything to the filter list. So I think this should address the failures. 🤞 This does mean that you can no longer use PYBIND11_TEST_OVERRIDE to specify a test that is not yet listed in the CMakeLists.txt file, but I expect no one ever did that anyway.

What do you think about porting your infra-structure changes to the master branch? That would make it easier to maintain the smart_holder branch until it's ready for merging

Yeah, for sure, I'm indifferent. I can cherrypick these commits to master when the pipeline is green. I'd like to stay on this branch for now to have the test case you introduced in there, as master doesn't have that yet.

[rwgk]

Looking great!

[rwgk]

I just spent 10 minutes looking through the changes but TBH I'm having trouble following the code.

In general I'm a big fan of merge-and-iterate, but in this situation I'm afraid it could get messy. I've been super conscious about keeping git diff master as small as possible and never git cherry-picked from smart_holder to master. This is to keep things as simple and obvious as possible. (Note that I also have to handle imports into the Google environment.)

I'm practically certain that there will be tweaks in the review on master; it just always happens. It'll be tricky to handle the conflicts when doing the git merge master here after the master PR is merged. I believe it'll be significantly easier overall to split this PR now, bulk changes PR against master, using the new features here in a reduced PR. I'd rather try that than the merge-and-cherry-pick-to-master route. If there is a chance you could handle that, that would be fantastic. But let me know, I could try myself, too.

[iwanders]

I just spent 10 minutes looking through the changes but TBH I'm having trouble following the code.

I've just rebased it on master, let me push and update the description of this PR. I'll add some explanation what the changes do and how they achieve the desired goal.

[rwgk]

I've just rebased it on master

Awesome, I think that's a great idea, thanks!

[iwanders]

Ok, sorry about the double force push, turned out I lost a commit when I switched to master... it was in the repo on my work laptop, not on my own desktop. It's fixed now, so all is good.

I've updated the description to provide an explanation of what happened and how it is all done. @rwgk , I hope this clarifies a bit how this PR makes your new unit tests fit in nicely. (Also, test_classh_mock.cpp on the smart_holder branch is named inconsistently with the rest, missing the underscore.)

[rwgk]

Thanks a lot! I'll try this out asap.

(Also, test_classh_mock.cpp on the smart_holder branch is named inconsistently with the rest, missing the underscore.)

It's intentional, that test is very much unlike the others. test_classh_mock.py is basically doing nothing (just confirms that the extension exists). I'm guessing with your changes I could even delete it, but I think I'd want to keep it anyway, kind-of-nice to have the test name show up in the pytest list.

[iwanders]

Thanks a lot! I'll try this out asap.

👍 give it a shot, see if it does what you want it to do. Like I said, I mainly saw an ask for help and answered the call. I don't really know how most of the cmake flags are actually used by developers in this repo :) Once you are happy with the proposed changes it should be good for review, pipeline is green again.

It's intentional, that test is very much unlike the others. [..] I'm guessing with your changes I could even delete it, but I think I'd want to keep it anyway, kind-of-nice to have the test name show up in the pytest list.

Ah, check, yeah, there indeed shouldn't be a strict requirement on a .cpp having a .py file equivalent anymore.

[rwgk]

Minor update:

I'm currently mystified

• why [smart_holder] Companion to @iwanders PR #3590 #3601 has CUDA 11 and PGI failures (https://github.com/pybind/pybind11/actions/runs/1669312656).
• but [smart_holder] JUST TO TRIGGER CI #3603 does not (everything passes).

I guess I'll have to dig in.

[iwanders]

Hmm, interesting. I presume this section is related to it... maybe something isn't backwards compatible afterall? @rwgk

[iwanders]

@rwgk , it's a problem in my changes to the pybind11_filter_tests macro. I used regular expressions... so I added a problem.

Specifically, it's a bug in the extension-remover, it's greedy and in your PR the .py occurs in the center of the list, removing... more than just the extension. I'll probably replace it with a normal string replace.

In short; problem understood, fix in the works.

[rwgk]

Hmm, interesting. I presume this section is related to it... maybe something isn't backwards compatible afterall? @rwgk

Thanks for the hint! I looked at the logs, your guess is definitely true for the CUDA 11 job (below). I'm still trying to understand the difference for PGI. @iwanders

$ grep -e 'Z -- The CXX compiler identification' -e 'Skipping test_constants_and_functions due to incompatible exception specifications' */1______3.8_____CUDA_11_____Ubuntu_20.04.txt
estblshd/1______3.8_____CUDA_11_____Ubuntu_20.04.txt:2022-01-07T22:02:29.9070107Z -- The CXX compiler identification is GNU 9.3.0
estblshd/1______3.8_____CUDA_11_____Ubuntu_20.04.txt:2022-01-07T22:02:32.6928939Z -- Skipping test_constants_and_functions due to incompatible exception specifications
iwanders/1______3.8_____CUDA_11_____Ubuntu_20.04.txt:2022-01-07T20:57:02.3532476Z -- The CXX compiler identification is GNU 9.3.0
$

[iwanders]

set(Z "test_async;test_buffers;test_builtin_casters;test_call_policies;test_callbacks;test_chrono;test_class;test_class_sh_basic;test_class_sh_disowning;test_class_sh_disowning_mi;test_class_sh_factory_constructors;test_class_sh_inheritance;test_class_sh_module_local.py;test_class_sh_shared_ptr_copy_move;test_class_sh_trampoline_basic;test_class_sh_trampoline_self_life_support;test_class_sh_trampoline_shared_from_this;test_class_sh_trampoline_shared_ptr_cpp_arg;test_class_sh_trampoline_unique_ptr;test_class_sh_unique_ptr_member;test_class_sh_virtual_py_cpp_mix;test_classh_mock;test_const_name;test_constants_and_functions;test_copy_move;test_custom_type_casters;test_custom_type_setup;test_docstring_options;test_eigen;test_enum;test_eval;test_exceptions;test_factory_constructors;test_gil_scoped;test_iostream;test_kwargs_and_defaults;test_local_bindings;test_methods_and_attributes;test_modules;test_multiple_inheritance;test_numpy_array;test_numpy_dtypes;test_numpy_vectorize;test_opaque_types;test_operator_overloading;test_pickling;test_pytypes;test_sequences_and_iterators;test_smart_ptr;test_stl;test_stl_binders;test_tagbased_polymorphic;test_thread;test_union;test_virtual_functions")

string(REGEX REPLACE "\\.[^.]*;" ";" FOO "${Z}")
message("\n\nFOO ${FOO}")

This manifests the issue, that clips the list at the .py extension. You can just throw this into a text file and invoke cmake on it to confirm.

[iwanders]

And the fix is I think changing it to string(REGEX REPLACE "\\.[^.;]*;" ";" FOO "${Z}") to terminate on the list seperator, instead of being able to run to the end of the list.

Let me run some tests.

[iwanders]

Ok, did a few local tests, I think the fix is good. @rwgk , you can cherry-pick a36de2c into your branch to test, I added that commit on top of this branch to hopefully fix that issue.

Glad you caught it, I only tested with the extension at the end of the list so didn't notice it was greedy.

[rwgk]

For easy reference, below are the tests/CMakeLists.txt diffs relative to master

• before this PR
• after this PR

Note importantly: without this PR, the PYBIND11_TEST_OVERRIDE behavior on the smart_holder branch is weird/broken, although not completely.

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index b00c043a..7ae2496f 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -101,6 +101,20 @@ set(PYBIND11_TEST_FILES
     test_callbacks.cpp
     test_chrono.cpp
     test_class.cpp
+    test_class_sh_basic.cpp
+    test_class_sh_disowning.cpp
+    test_class_sh_disowning_mi.cpp
+    test_class_sh_factory_constructors.cpp
+    test_class_sh_inheritance.cpp
+    test_class_sh_shared_ptr_copy_move.cpp
+    test_class_sh_trampoline_basic.cpp
+    test_class_sh_trampoline_self_life_support.cpp
+    test_class_sh_trampoline_shared_from_this.cpp
+    test_class_sh_trampoline_shared_ptr_cpp_arg.cpp
+    test_class_sh_trampoline_unique_ptr.cpp
+    test_class_sh_unique_ptr_member.cpp
+    test_class_sh_virtual_py_cpp_mix.cpp
+    test_classh_mock.cpp
     test_const_name.cpp
     test_constants_and_functions.cpp
     test_copy_move.cpp
@@ -163,6 +177,8 @@ if(PYBIND11_CUDA_TESTS)
 endif()
 
 string(REPLACE ".cpp" ".py" PYBIND11_PYTEST_FILES "${PYBIND11_TEST_FILES}")
+list(APPEND PYBIND11_PYTEST_FILES test_class_sh_module_local.py)
+list(SORT PYBIND11_PYTEST_FILES)
 
 # Contains the set of test files that require pybind11_cross_module_tests to be
 # built; if none of these are built (i.e. because TEST_OVERRIDE is used and
@@ -172,6 +188,8 @@ set(PYBIND11_CROSS_MODULE_TESTS test_exceptions.py test_local_bindings.py test_s
 
 set(PYBIND11_CROSS_MODULE_GIL_TESTS test_gil_scoped.py)
 
+set(PYBIND11_CLASS_SH_MODULE_LOCAL_TESTS test_class_sh_module_local.py)
+
 set(PYBIND11_EIGEN_REPO
     "https://gitlab.com/libeigen/eigen.git"
     CACHE STRING "Eigen repository to use for tests")
@@ -367,6 +385,16 @@ foreach(t ${PYBIND11_CROSS_MODULE_GIL_TESTS})
   endif()
 endforeach()
 
+foreach(t ${PYBIND11_CLASS_SH_MODULE_LOCAL_TESTS})
+  list(FIND PYBIND11_PYTEST_FILES ${t} i)
+  if(i GREATER -1)
+    list(APPEND test_targets class_sh_module_local_0)
+    list(APPEND test_targets class_sh_module_local_1)
+    list(APPEND test_targets class_sh_module_local_2)
+    break()
+  endif()
+endforeach()
+
 # Support CUDA testing by forcing the target file to compile with NVCC
 if(PYBIND11_CUDA_TESTS)
   set_property(SOURCE ${PYBIND11_TEST_FILES} PROPERTY LANGUAGE CUDA)

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index 0c146396..0667f0a7 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -122,6 +122,21 @@ set(PYBIND11_TEST_FILES
     test_callbacks
     test_chrono
     test_class
+    test_class_sh_basic
+    test_class_sh_disowning
+    test_class_sh_disowning_mi
+    test_class_sh_factory_constructors
+    test_class_sh_inheritance
+    test_class_sh_module_local.py
+    test_class_sh_shared_ptr_copy_move
+    test_class_sh_trampoline_basic
+    test_class_sh_trampoline_self_life_support
+    test_class_sh_trampoline_shared_from_this
+    test_class_sh_trampoline_shared_ptr_cpp_arg
+    test_class_sh_trampoline_unique_ptr
+    test_class_sh_unique_ptr_member
+    test_class_sh_virtual_py_cpp_mix
+    test_classh_mock
     test_const_name
     test_constants_and_functions
     test_copy_move
@@ -221,6 +236,8 @@ tests_extra_targets("test_exceptions.py;test_local_bindings.py;test_stl.py;test_
 
 # And add additional targets for other tests.
 tests_extra_targets("test_gil_scoped.py" "cross_module_gil_utils")
+tests_extra_targets("test_class_sh_module_local.py"
+                    "class_sh_module_local_0;class_sh_module_local_1;class_sh_module_local_2")
 
 set(PYBIND11_EIGEN_REPO
     "https://gitlab.com/libeigen/eigen.git"

[rwgk]

Looking for a 2nd review from @henryiii or @Skylion007.

[rwgk]

Error is not at the bottom though, other parallel jobs are interleaved in the terminal output from the looks of it, obscuring the issue.

I'm practically certain that's wasn't the issue: it's a warning, and it also appears in the output of the job that worked. I even compared the compilation commands, they are exactly identical.

If you think digging deeper could lead to valuable insights, these are the links to the two logs (not sure how long they will work):

• "established" https://pipelines.actions.githubusercontent.com/klHErCdD4MAAfh7Wd9wmuj1B4kqdVc9ZmKcBK6FIYqveU2FGrj/_apis/pipelines/1/runs/15460/signedlogcontent/5?urlExpires=2022-01-08T01%3A44%3A00.5477704Z&urlSigningMethod=HMACV1&urlSignature=XgQ40GCH1o6nvQsT2ryCiYFg5Mpzzb1J0L%2FXT7V6bZE%3D

• "before the not-greedy fix" https://pipelines.actions.githubusercontent.com/klHErCdD4MAAfh7Wd9wmuj1B4kqdVc9ZmKcBK6FIYqveU2FGrj/_apis/pipelines/1/runs/15449/signedlogcontent/42?urlExpires=2022-01-08T01%3A45%3A42.4583189Z&urlSigningMethod=HMACV1&urlSignature=iEINgAGdzYMt1%2B9TaIl6G5SoplyEk60KrAIPoCwMT2Q%3D

Slight less direct links that may work for a longer time:

• "established" https://github.com/pybind/pybind11/runs/4744167370

• "before the not-greedy fix" https://github.com/pybind/pybind11/runs/4743626230

[iwanders]

I even compared the compilation commands, they are exactly identical.
If you think digging deeper could lead to valuable insights, these are the links to the two logs (not sure how long they will work):

Links expired, but yeah, compile command looked the same. this morning I grabbed that toolchain and the Python3.6m headers and tried to reproduce the failure locally (on 41a549c, which had the failure), but both test_constants_and_functions.cpp and test_smart_ptr.cpp compile fine with the commands from the build log, with a warning in the former, exactly like you said. Both have a return code of 0, so doubt they'd fail the build.

I would've liked to understand the failure, but I'm clueless as well. Can't reproduce it locally, so I'm ok accepting it as a fluke from this proprietary toolchain.

[rwgk]

I would've liked to understand the failure

Me too (to maximize confidence for this PR).

@iwanders I think I found a clue.

First a simple sanity check: I triggered the testing again to see if the PGI failure was just a random flake, but it reproduces. (Not surprisingly, it reproduced between CI and CI-SH-DEF already; but now we're sure beyond a reasonable doubt.)

Looking at the "established" and "broken" logs from yesterday again, I see this in both (modulo time stamps):

2022-01-07T22:08:16.5960587Z ##[group]Run source /etc/profile.d/modules.sh
2022-01-07T22:08:16.5961332Z ^[[36;1msource /etc/profile.d/modules.sh^[[0m
2022-01-07T22:08:16.5961934Z ^[[36;1mmodule load /opt/nvidia/hpc_sdk/modulefiles/nvhpc/20.9^[[0m
2022-01-07T22:08:16.5962557Z ^[[36;1mcmake3 -S . -B build -DDOWNLOAD_CATCH=ON \^[[0m
2022-01-07T22:08:16.5963067Z ^[[36;1m                    -DCMAKE_CXX_STANDARD=11 \^[[0m
2022-01-07T22:08:16.5963738Z ^[[36;1m                    -DPYTHON_EXECUTABLE=$(python3 -c "import sys; print(sys.executable)") \^[[0m
2022-01-07T22:08:16.5964527Z ^[[36;1m                    -DCMAKE_CXX_FLAGS="-Wc,--pending_instantiations=0" \^[[0m
2022-01-07T22:08:16.5965298Z ^[[36;1m                    -DPYBIND11_TEST_FILTER="test_smart_ptr.cpp;test_virtual_functions.cpp"^[[0m

In the "broken" log I see this:

2022-01-07T21:13:50.9463305Z [ 90%] Building CXX object tests/CMakeFiles/pybind11_tests.dir/test_smart_ptr.cpp.o

But this does NOT appear in the "established" log.

In tests/CMakeLists.txt I see this:

set(PYBIND11_TEST_FILTER
    ""
    CACHE STRING "Tests from ;-separated list of *.cpp files will be removed from all tests")

Clearly that didn't work in the "broken" log. As a consequence it ran into an internal compiler error when trying to compile test_smart_ptr.cpp.

Does that make sense?

[iwanders]

Me too (to maximize confidence for this PR).

Of course, it's so easy to make mistakes in cmake, I honestly sometimes feel like there should be a test system for the buildsystem 🙄

@iwanders I think I found a clue.

Yes, you found a clue indeed! I understand the failure now that you showed it runs with a test filter. So the filter is applied here, and then we go to the filter macro. This then strips the extensions with the regex replace and then discards anything that matches. However, recall that in your branch, the tests looked like this;

set(PYBIND11_TEST_FILES
    test_async
    test_buffers
    <things>
    test_class_sh_module_local.py
    <things>
    test_smart_ptr
    <things>
    test_virtual_functions)

In which case, the greedy regex would replace everything from the .py onward in the ;-separated string, so effectively the list would be truncated to everything before the first entry with a filename. Which meant that test_smart_ptr wasn't present in the list without extensions, the list(FIND would never return a valid index, and it would not be filtered out.

I've pushed a minimal working example that reproduces the situation. This shows the filter doesn't work in the greedy-regex situation if you have a file with an extension in the middle of the list (bec4e0b). That prints a list that includes the test_smart_ptr.cpp file, when we cherry-pick a36de2c on top of that and run the shell script again it is indeed filtered correctly.

Good to understand that fully 👍

[rwgk]

Good to understand that fully 👍

Awesome. Thanks for the in-depth explanation!

[henryiii]

Would have been cleaner if we could have used a few features from modern CMake, but we are stuck on older versions for the moment for core stuff and this seems reasonable.

[iwanders]

Would have been cleaner if we could have used a few features from modern CMake,

Hah, agreed, list(FILTER would've made things a lot easier indeed!

[rwgk]

Retesting after git rebase master (no issues expected, but better safe than sorry).

[rwgk]

Thanks a lot Ivor, and thanks Aaron and Henry for the reviews!