tribits_add[_advanced]_test(): Address handling of semi-colons in ARGS and ENVIRONMENT (#299) #464

bartlettroscoe · 2022-03-11T00:11:39Z

This PR makes the tribits_add_advanced_test() function handle arguments that need to include a semi-colon ';' without messing up the list handling in CMake. This introduces a new top-level argument LIST_SEPARATOR <sep> used exactly the same way as the LIST_SEPARATOR argument with ExternalProject_Add(). Apparently, this is an accepted workaround for the problem of lists elements containing semi-colons with CMake.

I need to be able to handle semi-colons with tribits_add_advanced_test() for the new tests I am developing for #299.

Note that this PR contains an earlier commit that allowed the user to use quoted args that contained ';' and silently replaced that with <semicolon> but I think it is better to follow the convention from ExternalProject_Add() and make this explicit (even if it is a bit of a hack). I spent many hours experimenting with ways to deal with these arguments with failure after failure but I wanted to document this one way I found that worked with a commit. (It was after I asked Brad King that he mentioned the ExternalProject_Add() way of using LIST_SEPARATOR <sep> to workaround this problem and I refactored to use that approach. The core CMake language is really just broken when it comes to dealing with general lists with spaces and semi-colons.)

NOTE: I also fixed checking for unparsed args to tribits_add_advanced_test() that were not being checked before and this technically breaks backward compatibility. (I had to fix one of the TriBITS tests that had an unparsed/ignored argument so I am sure this is going to break Trilinos once this gets merged to Trilinos 'develop'.)

See the individual commit log messages for more details.

…ning() (TriBITSPub#299) With a newer version of CMake, we can use DEPRECATION instead of WARNING.

TriBITSPub#299) This now allows arguments to 'ARGS' to contain semi-colons ';'. To do this, it adds a new function with unit tests tribits_parse_arguments_for_list() to deal with these cases. A number of other changes in this commit include: * Fixed tribits_add_advanced_test() to actually check and report unused args (this did not work before) and added more tests to show this this. (I had to fix one TriBITS test TriBITS_CTestScripts_cmnd_3_args_1_overall_working_directory defined with tribits_add_advanced_test() that was using MESSAGE incorrectly.) * Fixed setting and testing with <Project>_CHECK_FOR_UNPARSED_ARGUMENTS CUSTOM_FAILURE_MODE and added more automated tests to show this. * Renamed the test TriBITS_TribitsExampleProject_PkgWithUserErrors_UNPARSED_ARGUMENTS and the the name _UNPARSED_ARGS_ to _UNPARSED_ARGUMENTS_ for consistancy. * Added tribits_parse_arguments_in_list() * Added tribits_copy_list() * Added unittest_compare_list_ele_const()

…c args with semi-colons (TriBITSPub#299) This is a better solution that is consistent with how ExternalProject_Add() gets around this problem. What I did: * Added explicit LIST_SEPARATOR to tribits_add_advanced_test() * Removed usage of tribits_parse_arguments_in_list() (it is no longer needed) * Moved top-level arguments to cmake_parse_arguments() into their own self-documenting variables that will be easier to maintain.

…#299) There is no need for these anymore now that an explicit LIST_SEPARATOR is used from the top down. Parsing and everything happens just fine.

TriBITSPub#299) Now I will be able to set env vars that have semi-colons, like CMAKE_PREFIX_PATH. NOTE: I had to use the: cmake_parse_arguments(PARSE_ARGV 0 FWD "" "" "") tick to get the arugments forwarded correctly that I learned from "Professional CMake".

bartlettroscoe · 2022-03-11T01:39:25Z

Hello @MikolajZuzek and @mperrinel. Can one of you review this PR in detail? I don't think both of you need to review the entire PR. But you both might look at changes to these three files:

bartlettroscoe · 2022-03-11T21:08:12Z

FYI: More discussion about how to handle semi-colons ; in list elements in:

bartlettroscoe · 2022-03-11T21:17:41Z

@MikolajZuzek and @mperrinel,

BTW, please let me know (in a comment in this PR) when you have started the review. I am seeing a few things that need addressed so I will be adding a new commit or two to address the issues.

…NT (TriBITSPub#299) This is to complement the same changes made to tribits_add_advanced_test(). I also added some missing tests for tribits_add_advanced_test() and updated CHANGELOG.txt for more info.

…EPARATOR (TriBITSPub#299) NOTE: I reversed the arguments to tribits_add_executable_and_test() in the unit test since when I first added LIST_SEPARATOR <sep> after TIMEOUT 1.5, it passed LIST_SEPARATOR <sep> as elements in TIMEOUT and passed my check at the end. By reversing the argments, this did not happen and the test failed again.

bartlettroscoe · 2022-03-12T01:37:25Z

@MikolajZuzek and @mperrinel,

FYI, I added some new commits to round this out. I don't think I will be adding any more new commits (unless typos or other problems are found).

mperrinel · 2022-03-16T09:04:41Z

@bartlettroscoe I am starting the review on it

mzuzek · 2022-03-16T16:59:53Z

Reviewing this as well

mperrinel

It was a technical one. The code seems good for me. I found a lot of TYPO mistakes. Will create a PR for that.

tribits/CHANGELOG.md

tribits/core/package_arch/TribitsAddAdvancedTest.cmake

tribits/core/utils/TribitsPrintList.cmake

test/core/CTestScriptsUnitTests/echoEnvVars.cmake

tribits/core/package_arch/TribitsAddAdvancedTest.cmake

tribits/core/package_arch/TribitsAddTest.cmake

bartlettroscoe · 2022-03-16T19:22:13Z

@mperrinel, thanks for the review. Yes, please put in a PR with the fixes. (Looks like I forgot to 'flyspell' when I edited these files.)

mzuzek · 2022-03-17T14:39:37Z

@bartlettroscoe @mperrinel

Is LIST_SEPARATOR necessary to solve the underlying problem ?

I was able to get rid of it and make all [new] *semicolon* test pass by fixing some quotes here and there, so that embedded lists are properly carried out to PARENT_SCOPE, etc. Please see b2f0a41 and cd84710 on my 464_fix-test-args-with-semicolons test branch, which builds on top of current head of this PR.

It's more of a PoC than complete fix - there is still handful of tests failing.
Let me know if that makes any sense or am I missing something crucial ?

…po_fixes Fixed typo mistake seen in PR 464

bartlettroscoe · 2022-03-20T17:27:07Z

Thanks for the review @mperrinel and fixing the typos. No problems with any of the source code or tests?

@MikolajZuzek, note the final conclusions on the handling of semicolon ; char data in lists and function and macro arguments in:

@bradking did not refute these conclusions and added some CMake documentation MRs to officially document the workaround LIST_SEPARATOR as the only robust way to deal with data containing semi-colon ; chars.

However, it just occurred to me that we could consider using the generator expression $<SEMICOLON> with tribits_add_test() and tribits_add_advanced_test() since the final interpretation of this data is not needed until running the actual tests and therefore could call all commands that run at generation time and accept generator expressions (like add_test(), set_test_properties() and file(GENERATE)). See:

https://gitlab.kitware.com/cmake/cmake/-/issues/23315#note_1160289

@MikolajZuzek, please don't just jump in and try to implement this. Let's wait for the response back from @bradking in that CMake GitLab Issue. Then, trying this should be fairly easy to try out given the tip of this existing branch (but adapting for the unit tests will be a little tricky since those need to generate the <testName>.cmake file in a cmake -P script).

mzuzek · 2022-03-21T12:39:06Z

As you can see, the changes you made in this branch have broken backward compatibility. The fact that you had to quote a bunch of arguments all over the place is evidence of that. That is unacceptable.

@bartlettroscoe Thank you for the feedback. Here's the fixed version that greens out all relevant tests without changing their content (except for LIST_SEPARATOR retraction): 0001-remove-LIST_SEPARATOR-and-fix-list-passing.patch.zip

If you see any break in backward compatibility or any other issue, please expose it via unit test.

mzuzek · 2022-03-21T12:39:21Z

@bartlettroscoe
⚠ DISCLAIMER: Please use any code presented by me to support the review discussions and focus it on relevant technical details with extreme care: it's barely a side experiment built on top of the implementation brought in by this PR, which helps me understand the problem being solved and the underlying CMake limitations causing it.

mzuzek

@bartlettroscoe @keitat @mperrinel

Looks good. The fix is consistent with semicolon handling in CMake and the test coverage is great!

It could be worthy to also explore handling problematic lists as strings to avoid complicating TriBITS API with LIST_SEPARATOR - see the discussion above starting at #464 (comment).

mzuzek · 2022-03-21T12:39:36Z

@MikolajZuzek, please don't just jump in and try to implement this. Let's wait for the response back from @bradking in that CMake GitLab Issue. Then, trying this should be fairly easy to try out given the tip of this existing branch (but adapting for the unit tests will be a little tricky since those need to generate the .cmake file in a cmake -P script).

Of course. Let me know when the PR is ready for re-review.

bartlettroscoe · 2022-03-21T16:54:02Z

Here's the fixed version that greens out all relevant tests without changing their content (except for LIST_SEPARATOR retraction): 0001-remove-LIST_SEPARATOR-and-fix-list-passing.patch.zip

@MikolajZuzek, can you please post that as a WIP PR so it is easier to see the changes?

However, Kitware's feedback up to this point (from multiple CMake maintainers) is that trying to keep raw semi-colons ; inside of CMake lists is not going to work in general. In fact, CMake does not even handle $<SEMICOLON> correctly with CTest properties as per:

https://gitlab.kitware.com/cmake/cmake/-/issues/23315#note_1160588

so my idea to try out $<SEMICOLON> above is a no go.

Also, see recent comments:

The only robust way to deal with semi-colon ; chars as data to pass around in list elements is to not do that. Hence the LIST_SEPRATOR workaround with specific translations at final point of usage is the only robust way to deal with data in lists that contains semi-colon ; chars . I don't know why we would be arguing with Kitware at this point. They know more about this topic than all of us on the issue put together.

And now the LIST_SEPRATOR workaround is now the officially recommended CMake policy as per:

https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#lists

As noted by @mathstuf in:

https://gitlab.kitware.com/cmake/cmake/-/issues/23315#note_1160334

All in all, I don't know how feasible it is to shuttle semicolons to any arbitrary corner of CMake's workings without fundamentally rethinking CMake's type representations (which touch compatibility points all along the way, but maybe could be smuggled around somehow).

Getting even the internals of CMake to deal with raw semi-colon ; data in lists is still an open question. So it is time give up and just accept the LIST_SEPRATOR workaround in the PR and move on.

bradking · 2022-03-21T18:26:06Z

The only robust way to deal with semi-colon ; chars as data to pass around in list elements is to not do that. Hence the LIST_SEPRATOR workaround with specific translations at final point of usage is the only robust way to deal with data in lists that contains semi-colon ; chars .

Yes. That's why CMake's own ExternalProject module uses LIST_SEPARATOR.

I was able to get rid of it and make all [new] semicolon test pass by fixing some quotes here and there

CMake interfaces that don't document support for semicolons likely don't support them. We've now updated the documentation to clarify this point. It's unfortunate that the language is like this, but it wasn't designed to be as general purpose as today's use cases would like it to be.

In some cases one can add quoting or backslashes near semicolons until things seem to work locally, but the resulting code may be relying on assumptions about implementation details that might change.

bartlettroscoe · 2022-03-21T18:41:46Z

In some cases one can add quoting or backslashes near semicolons until things seem to work locally, but the resulting code may be relying on assumptions about implementation details that might change.

@bradking, right, and CMake needs to write tests and documentation that pin down existing behaviors so at least it is possible to pass data containing semi-colon ; chars where it currently works (by adding the correct number of escape backslashes \ for the given usage) so it will keep working.

bartlettroscoe · 2022-03-23T14:05:25Z

Without further objection, I am going to merge this to 'master'. There is just no other robust alternative (short of creating a new CMake scripting language that treats lists as first-class entities and has the notation to support that).

mzuzek · 2022-03-24T02:20:19Z

@bartlettroscoe @bradking

This PR relies on add_test() and execute_process() properly handling "\\\;".
Should that be considered an undocumented implementation detail that may change anytime ?

Please correct me if I get your discussion wrong: LIST_SEPARATOR shall/will be introduced to various CMake commands so that users won't have to play with backslashes like we do here in this PR ?

mzuzek · 2022-03-24T02:21:56Z

@bartlettroscoe

Here's the fixed version that greens out all relevant tests without changing their content (except for LIST_SEPARATOR retraction): 0001-remove-LIST_SEPARATOR-and-fix-list-passing.patch.zip

@MikolajZuzek, can you please post that as a WIP PR so it is easier to see the changes?

Of course - see bartlettroscoe#2.

Please note that there is no difference in how we feed CMake commands with "\\\;" for test arguments and ";" for test environment on both PRs - the fixes are just about passing test arguments as strings instead of lists from TriBITS interface to bottom CMake commands (to preserve contained semicolons).

bartlettroscoe · 2022-03-24T13:30:24Z

This PR relies on add_test() and execute_process() properly handling "\\\;".
Should that be considered an undocumented implementation detail that may change anytime ?

@MikolajZuzek, yes Kitware knows about that and has promised to address this. See:

https://gitlab.kitware.com/cmake/cmake/-/merge_requests/7066#note_1157828

CC: @bradking

mzuzek · 2022-03-24T14:01:29Z

@bartlettroscoe Thank you for the confirmation.

PS. Year ago I was happily using CMake 3.19+ to pass around JSON structures with recursively embedded lists and dictionaries. Now I'm a bit scarred of passing plain strings because they may eventually contain semicolons, which can lead to undefined behavior on client platforms...

bartlettroscoe · 2022-03-24T14:18:23Z

Now I'm a bit scarred of passing plain strings because they may eventually contain semicolons

Right. The only solution is to develop a new CMake language and parser. I am working with Kitware to start to advertise the idea for a proposal for a new CMake language parser (with both a new declarative language and a new Turing-complete imperative scripting language).

bradking · 2022-03-24T19:40:22Z

@MikolajZuzek passing around plain strings containing JSON is fine so long as nothing tries to interpret it as a list or anything else besides a single string. The problem with semicolons is only when one wants to store that plain string as an element of another list. Cleanly handling lists with arbitrary content in their elements, such as nested lists, will require the new language Roscoe mentioned.

mzuzek · 2022-03-25T12:47:19Z

@MikolajZuzek passing around plain strings containing JSON is fine so long as nothing tries to interpret it as a list or anything else besides a single string. The problem with semicolons is only when one wants to store that plain string as an element of another list. Cleanly handling lists with arbitrary content in their elements, such as nested lists, will require the new language Roscoe mentioned.

Thank you @bradking: that's an excellent summary of my key takeaway and lesson learned from this PR, bartlettroscoe#2 and related discussions on CMake issues.

…/TriBITS#464) Before, this was passing in: FAIL_REGULAR_EXPRESSION "FAIL;BUMMER" which just happened to be added as a list to the test property with the old TriBITS. But with the new TriBITS in TriBITSPub/TriBITS#464 which more correctly deals with semi-colons and list arguments, this was being set as a single property value "FAIL\\;BUMMER" which did not match the output. Passing in the list with: FAIL_REGULAR_EXPRESSION FAIL BUMMER works both with old and new TriBITS and is more logical given how CMake/CTest is supposed to be used. (The old TriBITS documentation was incorrect.)

…BITS#464) Tt turns out that these tests really did not need to pass a list of values to FAIL_REGULAR_EXPRESSION. Each test was really only looking for a single value of 'FAILED' or 'BUMMER'. And you can argue that the tests are stronger and better by taking out the other value that is not supposed to be printed in that test case.

…ESSION (TriBITSPub#464) Turns out that the tribits_add_advanced_test() test-block arguments PASS_REGULAR_EXPRESSION and FAIL_REGULAR_EXPRESSION did not support taking in a list of regexes like the built-in CMake CTest properties of the same names. This commit updates TAAT() to allow a list of arguments and they behave the exact same way as the built-in properties (i.e. matching any). I also updated the documentation to show how to pass in a list of regexes correctly for PASS_REGULAR_EXPRESSION, FAIL_REGULAR_EXPRESSION, FINAL_PASS_REGULAR_EXPRESSION and FINAL_FAIL_REGULAR_EXPRESSION. I also fixed the documentation for tribits_add_test() as well for PASS_REGULAR_EXPRESSION and FAIL_REGULAR_EXPRESSION. Basically, a well constructed CMake function should almost never take in a list of arguments expecting an explicit semi-colon. This was part of the learning in PR TriBITSPub#464 with how to treat semi-colons in CMake. This commit was driven by a usage of FAIL_REGULAR_EXPRESSION in Trilinos while testing updated TriBITS as part of TriBITSPub#299 when I realized the documentation was wrong for how to deal with these lists of regexes.

…expressions Support list for TAAT() PASS_REGULAR_EXPRESSION and FAIL_REGULAR_EXPRESSION (#464)

…/TriBITS#464) Before, this was passing in: FAIL_REGULAR_EXPRESSION "FAIL;BUMMER" which just happened to be added as a list to the test property with the old TriBITS. But with the new TriBITS in TriBITSPub/TriBITS#464 which more correctly deals with semi-colons and list arguments, this was being set as a single property value "FAIL\\;BUMMER" which did not match the output. Passing in the list with: FAIL_REGULAR_EXPRESSION FAIL BUMMER works both with old and new TriBITS and is more logical given how CMake/CTest is supposed to be used. (The old TriBITS documentation was incorrect.)

…BITS#464) Tt turns out that these tests really did not need to pass a list of values to FAIL_REGULAR_EXPRESSION. Each test was really only looking for a single value of 'FAILED' or 'BUMMER'. And you can argue that the tests are stronger and better by taking out the other value that is not supposed to be printed in that test case.

…s:develop' (f7c3706). * trilinos-develop: FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub/TriBITS#274) FindTPLLAPACK.cmake: Lower-case function names (TriBITSPub/TriBITS#274) FindTPLBLAS.cmake: Lower-case function names (TriBITSPub/TriBITS#274) Isorropria: Unconditionally disable tests that depend on EpetraExt (trilinos#10534) Zoltan2: Use single value for FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464) Zoltan2: Properly pass in list to FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464) Sacado: Call target_include_directories() for new TriBITS (TriBITSPub/TriBITS#299) ATDM: Strip leading and trailing whitespace from flags for newer CMakes

…Trilinos:develop' (7d907aa). * trilinos/develop: (31 commits) MiniEM: Fix lambda capture issues in evaluators Phalanx: fix for non-cuda spaces Cmake: Tpetra deprecation removal nightlies Phalanx: cleanup Phalanx: add new ctor to v_of_v Phalanx: another view-of-view utility FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub/TriBITS#274) FindTPLLAPACK.cmake: Lower-case function names (TriBITSPub/TriBITS#274) FindTPLBLAS.cmake: Lower-case function names (TriBITSPub/TriBITS#274) Isorropria: Unconditionally disable tests that depend on EpetraExt (trilinos#10534) Panzer MiniEM: Disable tests for 2nd order elements MueLu Maxwell1: Small fix MiniEM: Add "p coarsen schedule" Zoltan2: Use single value for FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464) Zoltan2: Properly pass in list to FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464) Sacado: Call target_include_directories() for new TriBITS (TriBITSPub/TriBITS#299) Phalanx: Array of Views acceptance test. ATDM: Strip leading and trailing whitespace from flags for newer CMakes Ifpack2 Hiptmair: Add parameter "hiptmair: implicit transpose" Ifpack2 Hiptmair: timers ...

bartlettroscoe added 5 commits March 10, 2022 16:46

Convert to standard function comment block (TriBITSPub#299)

bb9175a

Changed from WARNING to DEPRECATION in parse_arguments_deprecated_war…

625cd02

…ning() (TriBITSPub#299) With a newer version of CMake, we can use DEPRECATION instead of WARNING.

Remove tribits_parse_arguments() and tribits_copy_lists() (TriBITSPub…

37177a5

…#299) There is no need for these anymore now that an explicit LIST_SEPARATOR is used from the top down. Parsing and everything happens just fine.

bartlettroscoe force-pushed the 299-tribits_add_advanced_test_semicolons branch 2 times, most recently from f02f4a1 to 47bc34f Compare March 11, 2022 01:04

bartlettroscoe force-pushed the 299-tribits_add_advanced_test_semicolons branch from 47bc34f to bd550e6 Compare March 11, 2022 01:09

bartlettroscoe added type: enhancement component: core labels Mar 11, 2022

bartlettroscoe assigned mperrinel and mzuzek Mar 11, 2022

bartlettroscoe requested review from mzuzek and mperrinel March 11, 2022 01:34

bartlettroscoe added 2 commits March 11, 2022 16:06

tribits_add_test(): Add LIST_SEPARATOR support for args and ENVIRONME…

434d3ed

…NT (TriBITSPub#299) This is to complement the same changes made to tribits_add_advanced_test(). I also added some missing tests for tribits_add_advanced_test() and updated CHANGELOG.txt for more info.

bartlettroscoe changed the title ~~tribits_add_advanced_test(): Address handling of semi-colons in ARGS and ENVIRONMENT (#299)~~ tribits_add[_advanced]_test(): Address handling of semi-colons in ARGS and ENVIRONMENT (#299) Mar 12, 2022

mperrinel reviewed Mar 16, 2022

View reviewed changes

Fixed typo mistake seen in PR 464

aa684c1

Merge pull request #1 from bartlettroscoe/464-tribits_add_advanced_ty…

975ffa8

…po_fixes Fixed typo mistake seen in PR 464

mzuzek approved these changes Mar 21, 2022

View reviewed changes

bartlettroscoe merged commit d96f0d4 into TriBITSPub:master Mar 23, 2022

mzuzek mentioned this pull request Mar 24, 2022

Use strings to pass test args with semicolons #464 bartlettroscoe/TriBITS#2

Closed

bartlettroscoe mentioned this pull request May 16, 2022

Support list for TAAT() PASS_REGULAR_EXPRESSION and FAIL_REGULAR_EXPRESSION (#464) #481

Merged

2 tasks

bartlettroscoe added a commit that referenced this pull request May 16, 2022

Merge pull request #481 from TriBITSPub/299-tribits_add_test_regular_…

2f3633a

…expressions Support list for TAAT() PASS_REGULAR_EXPRESSION and FAIL_REGULAR_EXPRESSION (#464)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tribits_add[_advanced]_test(): Address handling of semi-colons in ARGS and ENVIRONMENT (#299) #464

tribits_add[_advanced]_test(): Address handling of semi-colons in ARGS and ENVIRONMENT (#299) #464

bartlettroscoe commented Mar 11, 2022 •

edited

Loading

bartlettroscoe commented Mar 11, 2022

bartlettroscoe commented Mar 11, 2022

bartlettroscoe commented Mar 11, 2022

bartlettroscoe commented Mar 12, 2022

mperrinel commented Mar 16, 2022

mzuzek commented Mar 16, 2022

mperrinel left a comment •

edited

Loading

bartlettroscoe commented Mar 16, 2022

mzuzek commented Mar 17, 2022

bartlettroscoe commented Mar 20, 2022

mzuzek commented Mar 21, 2022

mzuzek commented Mar 21, 2022

mzuzek left a comment

mzuzek commented Mar 21, 2022

bartlettroscoe commented Mar 21, 2022

bradking commented Mar 21, 2022

bartlettroscoe commented Mar 21, 2022

bartlettroscoe commented Mar 23, 2022

mzuzek commented Mar 24, 2022

mzuzek commented Mar 24, 2022

bartlettroscoe commented Mar 24, 2022

mzuzek commented Mar 24, 2022

bartlettroscoe commented Mar 24, 2022

bradking commented Mar 24, 2022

mzuzek commented Mar 25, 2022

tribits_add[_advanced]_test(): Address handling of semi-colons in ARGS and ENVIRONMENT (#299) #464

tribits_add[_advanced]_test(): Address handling of semi-colons in ARGS and ENVIRONMENT (#299) #464

Conversation

bartlettroscoe commented Mar 11, 2022 • edited Loading

bartlettroscoe commented Mar 11, 2022

bartlettroscoe commented Mar 11, 2022

bartlettroscoe commented Mar 11, 2022

bartlettroscoe commented Mar 12, 2022

mperrinel commented Mar 16, 2022

mzuzek commented Mar 16, 2022

mperrinel left a comment • edited Loading

Choose a reason for hiding this comment

bartlettroscoe commented Mar 16, 2022

mzuzek commented Mar 17, 2022

bartlettroscoe commented Mar 20, 2022

mzuzek commented Mar 21, 2022

mzuzek commented Mar 21, 2022

mzuzek left a comment

Choose a reason for hiding this comment

mzuzek commented Mar 21, 2022

bartlettroscoe commented Mar 21, 2022

bradking commented Mar 21, 2022

bartlettroscoe commented Mar 21, 2022

bartlettroscoe commented Mar 23, 2022

mzuzek commented Mar 24, 2022

mzuzek commented Mar 24, 2022

bartlettroscoe commented Mar 24, 2022

mzuzek commented Mar 24, 2022

bartlettroscoe commented Mar 24, 2022

bradking commented Mar 24, 2022

mzuzek commented Mar 25, 2022

bartlettroscoe commented Mar 11, 2022 •

edited

Loading

mperrinel left a comment •

edited

Loading