Add some error checking and reporting for common TriBITS project developer and user mistakes

[bartlettroscoe]

Next Action Status:

Added checks for usage of TRIBITS_PACKAGE(), TRIBITS_PACKAGE_POSTPROCESS(), etc. and some other things. Next: Need to add checks for TRIBITS_SUBPACKAGE(), TRIBITS_SUBPACKAGE_POSTPROCESS(), etc.

CC: @fryeguy52

Description:

The TriBITS framework can reduce some of the boiler plate raw CMake code that project developers need to write when setting up a CMake build and tests with CTest in their CMakeLists.txt files. However, history has shown that project developers and TriBITS project users made some common mistakes when using TriBITS. TriBITS already contains error checks and reporting for many types of common mistakes (e.g. misspelling the name of a TriBITS SE package, or trying to create a circular dependency) but there are many other types of mistakes that are not currently caught and reported very gracefully (e.g. not calling TRIBITS_PROJECT_POSTPROCESS() at the end of a packages's CMakeLists.txt file)

This Story is to add error checking and reporting (using MESSAGE(FATAL_ERROR ...)) for many common mistakes. An initial list of the mistakes that can be caught and gracefully reported are:

• Check that TRIBITS_PACKAGE() is called before TRIBITS_ADD_LIBRARY() or TRIBITS_ADD_EXECUTABLE(). [DONE]

• Check that TRIBITS_PACKAGE_DECL(), TRIBITS_PACKAGE_DEF(), TRIBITS_PACKAGE_POSTPROCESS(), TRIBITS_PROJECT() and many other such user-facing macros and functions are called only once (i.e. per package for package-level functions and once per project for project-level functions). [DONE]

• Check that TRIBITS_PACKAGE_POSTPROCESS() is called before the end of the package's CMakeLists.txt file after all other commands like TRIBITS_ADD_LIBRARY() and TRIBITS_ADD_EXECUTABLE(). [DONE]

• Check that all of the arguments to TRIBITS_PROJECT_DEFINE_DEPENDENCIES() match named arguments (e.g. look for non-empty PARSE_UNPARSED_ARGUMENTS after calling CMAKE_PARSE_ARGUMENTS(PREFIX ...) and abort if found). [DONE]

• Add checks for all TriBITS functions and macros that use CMAKE_PARSE_ARGUMENTS(PREFIX ...) for PARSE_UNPARSED_ARGUMENTS when they should not exist. [DONE]

• Add checks that TRIBITS_SUBPACKAGE(), TRIBITS_SUBPACKAGE_POSTPROCESS(), etc. are called correctly [DONE]

• Check that TRIBITS_PACKAGE() is not called in the base CMakeLists.txt file for a package that has subpackages. Instead, they must call TRIBITS_PACKAGE_DECL() and TRIBITS_PACKAGE_DEF() (with TRIBITS_PROCESS_SUBPACKAGES() called in between). [DONE]

• Check that missing a call to TRIBITS_PROCESS_SUBPACKAGES() is caught if one forgets to call it before TRIBITS_PACKAGE_DEF() is called. [DONE]

• Check that if only TRIBITS_PACKAGE_DECL() is called in parent packages's CMakeList.txt file that TriBITS reports the user needs to call TRIBITS_PROCESS_SUBPACKAGES() next. [DONE]

• Check that TRIBITS_PACKAGE_DEF() is called once and only once in a parent packages's CMakeLists.txt file. [DONE]

• Check that a subpackages's CMakeLists.txt files do not call any of the commands TRIBITS_PACKAGE(), TRIBITS_PACKAGE_DECL(), TRIBITS_PACKAGE_DEF(), TRIBIITS_PROCESS_SUBPACKAGES(), TRIBITS_PACKAGE_POSTPROCESS(). [DONE]

• Check that TRIBITS_ADD_TEST_DIRECTORIES() and TRIBITS_ADD_EXAMPLE_DIRECTORIES() are called between TRIBITS_SUBPROJECT() and TRIBITS_SUBPROJECT_POSTPROCESS() (and the same for a top-level packages). [DONE]

The above are just a few of the types of mistakes that TriBITS developers and users have made in the past and TriBITS does not catch and report very well. Some more thought and exploration will yield other missing checks. But this story does not have to involve finding and resolving all such errors.

What makes this story non-trivial is that we must set up automated testing to check that TriBITS can catch and report these mistakes. To do that, one could set up new testing dummy TriBITS package(s) that can be configured to make all of these mistakes and then add automated tests in the TriBITS test suite to make sure that TriBITS code catches the mistakes and prints the right error messages.

[bartlettroscoe]

@fryeguy52, I think this is a natural story to go along with setting up a tutorial for TriBITS in #190.

[bartlettroscoe]

@fryeguy52 and I pair programmed the starter to this in:

• https://github.com/TriBITSPub/TriBITS/tree/user-error-checking-200

@fryeguy52, please checkout out this branch in your local git repo on my machine crf450 and continue one. Let me know if you have questions.

[bartlettroscoe]

I just merged the branch 'user-error-checking-200' that takes care of several of these checks (see above 'description' field).

@fryeguy52,

Note that there are still several types of checks mentioned above.

[bartlettroscoe]

@fryguy52,

I cherry-picked, tested, and pushed the commit 9c09636 which was pushed as the commit 3559d6b.

I then deleted the user-error-checking-200 branch:

$ git push github --delete user-error-checking-200 
To [email protected]:TriBITSPub/TriBITS.git
 - [deleted]         user-error-checking-200

For any future work on this story, we should create follow-on topic branches like assert-single-arg-values-200 to add checking for single-value arguments.

[fryeguy52]

@bartlettroscoe it looks like your commit fixed this. Do you need me to do anything?

[bartlettroscoe]

@bartlettroscoe it looks like your commit fixed this. Do you need me to do anything?

I think I fixed this use case. If we see other cases like this come up, we may need to dig in and try to improve the error checking and error messages. It turns out that in the case of SCOREC that it used to have subpackages then it was refactored to not have subpackages but the top-level SCOREC CMakeLists.txt file never got updated. It turns out that worked just fine. But the new checking did not allow that use case. I added a check to disallow that. But I still allowed one to call TRIBITS_PACKAGE_DECL() and TRIBIT_PACAKGE_DEF() instead of TRIBITS_PACKAGE() for packages with no subpackages. I am not sure if we should allow that or not but it seems okay to do that.

[bartlettroscoe]

FYI: I have descoped this story and added the remaining checks to the new story #339. Therefore, I will close this story as complete.