WIP: Test out updated CDash URLs for builds and nonpassing tests for same repo version (#483) #484
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I just noticed this while looking over the code.
#299) This was snapshotted from the Trilinos commit: commit 8c1028df724eb0096cd688b381285112bc6f914a Author: Roscoe A. Bartlett <[email protected]> Date: Wed May 18 15:08:17 2022 -0600 FindTPLNetcdf.cmake: Lower-case function names (#274) soon to be merged to Trilinos 'develop' from Trilinos PR #10533. However, this is not an snapshot as I made the following changing before creating this commit: * Kept the spelling fix for 'separated' made in TriBITS 'master' branch from commit c716a74; Author: Greg Sjaardema <[email protected]>; Date: Mon Aug 2 16:02:20 2021 -0600; "Spelling fixes" * I removed a few trailing spaces at the end of some lines Why? There really is no value in using a different version of FindTPLNetcdf.cmake in TriBITS that what is used in Trilinos. When testing Trilinos against updated versions of TriBITS, you really want the logic to match. In the case in testing with Trilinos (as part of #299 and testing with trilinos/Trilinos#10533), the old version of FindTPLNetcdf.cmake before this change was not properly setting TPL_Netcdf_PARALLEL which was not allowing the enable of the test SEACASIoss_exodus_fpp_serialize. With this sync, we get the exact same tests.
This gets rid of logic that was looking for <lib>_INCLUDE_DIRS. With modern CMake, the include dirs are carried along with the library target object itself so there is no need for this var (but we don't remove the var yet, that comes in later commit). As part of this, I created a new module TribitsLibIsTestOnly.cmake with functions to reduce duplication.
This seems to pass all of the tests. This could use some cleanup of the link lines to avoid listing the same library more than once on the link line. But we can do that latter with some fine-tuning.
…#299) Here, I just commented out everything that I don't need. I really need to just remove it once I get there. I also changed <Package>_TPL_LIBRARIES and <Project>_TPL_LIBRARIES to list <tplName>::all_libs instead of library files and list these in generated <Package>Config.cmake and <Project>Config.cmake files. I don't think downsteam customers need this list but it is there if they want to use them for some reason I can't imagine.
There is still a lot of work to clean up all of the junk that is being done in TribitsWriteClientExportFiles.cmake. But this is a good start. NOTE: This does **not** handle the extra link flags/libraries given in <Project>_EXTRA_LINK_FLAGS that is collected into the library target 'last_lib'. That is an open problem that I am going to have to solve in some way :-(
It just makes sense for the function tribits_add_library() and its support code to have its own file.
* Factor out tribits_add_library_assert_deplibs() * Factor out tribits_add_library_assert_importedlibs() * Factor out tribits_add_library_determine_install_lib_and_or_headers() * Improve names of some vars * Remove 'SE' from warning messages * Various other cleanup
This also indirectly tests tribits_create_name_from_current_source_directory(), sort of.
…ctory() (#299) This will help catch the defect in tribits_create_name_from_current_source_directory() (to be fixed next).
…#299) Wen I did the refactoring of tribits_add_executable() to break out some subfunctions, it exposed a defect from the following commit 13 years ago: 654a4ef8843 " SUMMARY: Cmakify Intrepid" Author: Brent M. Perschbacher <[email protected]> Date: Thu Apr 2 15:54:30 2009 +0000 (13 years ago) The problem was an incorrect setting of an output function argument: FUNCTION( PACKAGE_CREATE_NAME_FROM_CURRENT_SOURCE_DIRECTORY DIRECTORY_NAME ) SET(DIRECTORY_NAME "") ... SET(DIRECTORY_NAME ${DIRECTORY_NAME} PARENT_SCOPE) ENDFUNCTION() When I renamed the variable `DIRECTORY_NAME` in the parent scope, it broke this function (because DIRECTORY_NAME was no longer being used). (If you are going to just set a var in the parent scope unconditionally, why bother passing in some name in the function? Just another example of where inexperienced developers have problems with the strange CMake language.) NOTE: That function got renamed and moved around but I was able to track it along the CMake history.
* Propertly set initial value of ${PARSE_ADDED_EXE_TARGET_NAME_OUT} at
beginning
* Factor out tribits_add_executable_assert_correct_call_context()
* Factor out tribits_add_executable_is_skipped()
* Factor out tribits_add_executable_adjust_exe_name()
* Factor out tribits_add_executable_get_adjusted_sources_list()
* Factor out tribits_add_executable_assert_testonlylibs()
* Factor out tribits_add_executable_assert_importedlibs()
* Factor out tribits_add_executable_convert_from_deplibs()
* Other cleanup:
- Rename some local vars
- Move blocks of code around to group them locally
- Add/improve some comments
With the switch to modern CMake, this is much simpler
I also removed a few more code references to these vars as they should not be needed anymore.
, #299) This shows that the new TPL dependencies actually work to bring in indirect TPL dependencies and create the proper link lines. If you comment out the `set(<tplName>_LIB_ENABLED_DEPENDENCIES ...)` statements in `TribitsExampleProject2/TPLsList.cmake`, you will see failing tests.
…s on (#299) It was amazing how little code this required. I added a pretty decent test to show this puts the libraries at the end of every link link. This also removes the last remnants of the 'last_lib' library.
When you have a lot of packages, you have to avoid including their <Package>Config.cmake file more than once during a find_packge() call. Without these include guards, it took like 24 minutes to configure Albany. When I put them in, the configure time for Albany went down to just 7 sec!
…all tree (#299) Turns out that Albany explicitly looks for files under ${Trilinos_INCLUDE_DIRS}. Also, Albany was looking for files like "${Trilinos_INCLUDE_DIRS}/Kokkos_config.h" so it basically requires Trilinos_INCLUDE_DIRS to be just a single directory! Therefore, to avoid having to change Albany, I have set <Project>_INCLUDE_DIRS in the installed <Project>Config.cmake file. This was easy to do for this case. But note that <Project>_INCLUDE_DIRS in <Project>Config.cmake in the build dir still will be set to empty because there is no easy way to set it in that case. This is a stop-gap measure to allow APPs like Albany to keep working during this initial transition to modern CMake. But we really should remove this var from the <Project>Config.cmake file entirely at some point. NOTE: I have not fixed all of the TriBITS tests for this change yet. That is why this is a 'WIP' commit. I will fix the tests in a later commit.
I was trying too reproduce failure I saw testing with Albany where it was trying to link against MPI::all_libs. But this did not do it. But it is nice to have a test case where the MPI TPL is enabled and propgated to a downstream APP.
This will help to expose a defect in TriBITS and provides better TriBITS testing realted to the TriBITS MPI TPL.
This produces the same error I saw when testing Albany against Trilinos and
updated TriBITS:
CMake Error at CMakeLists.txt:??? (add_executable):
Target "app" links to target "MPI::all_libs" but the target was not found.
Perhaps a find_package() call is missing for an IMPORTED target, or an
ALIAS target is missing?
To generate this error, I add to add linking against
${TribitsExProj_TPL_LIBRARIES} which is what Albany currently does as well.
I think this is a common usage of old-style TriBITS so it is important to test
this case.
This avoids needing to check `if (TARGET <Pkg>::all_libs)` all over the place. This makes the failing test: TriBITS_TribitsOldSimpleExampleApp_STATIC_MPI_USE_DEPRECATED_TARGETS from the previous commit pass. Next, we can take out all of these silly checks.
Now that the MPI TPL produces MPI::all_libs, there is no more need for these silly checks. This also makes the code have stronger checking in case of mistakes.
This is to make the file easier to navigate so I can add more unit tests. This file has gotten way too big and needs to be broken into several smaller files at some point here.
This was not printing out the semi-colons in a list passed to message_wrapper(). I noticed this while working on #483.
I also updated the documentation some for unittest_string_regex(). I do not run the new function unittest_string_var_regex() in this commit but will in a later commit where I manually tested as part of work for #483 to ensure it passes and fails when it should.
This makes the module TribitsGeneralMacros.cmake smaller and better aggregates these commands. I also added a new macro tribits_assert_parse_arg_one_value() which I just tested manually. It is a very simple macro so I am not too worried about not unit testing it yet.
This allows more compact argument parsing code and checking
We need to be breaking up TribitsGlobalMacros.cmake because it is too long. I also added a new function to get just the 40-char SHA1 and I added unit tests for it.
This comes up with refactorings being done for #483.
I will be adding functions for getting CDash URLs other than just for builds.
This is part of a refactoring to display more CDash URLs other than just build URLs.
This will make this easier to reuse to get other CDash URLs.
bartlettroscoe
force-pushed
the
299-switch-to-modern-cmake-targets-with-more-cdash-urls
branch
3 times, most recently
from
255cda9
to
30f317d
Compare
…ersion (#483) This makes it easy to click on links to these other CDash URLs to see what is happening with all of the builds for the same repo version. The main target is for the GitHub Actions drivers for TriBITS testing. This makes it easy to get at those results. As part of this, I also renamed the function tribits_get_build_url_and_write_to_file() to tribits_get_cdash_build_url() and removed the ability to write to a file. (We just don't need that anymore and it is trivial to write a string to a file.)
This will post the base repo version info to CDash which provides more info right on CDash on the main index.php page. However, it will set CTEST_DO_UPDATES=OFF unless the base repo is a Git repo and we can extract the SHA1 from the repo with tribits_git_repo_sha1() (which is a very robust function).
bartlettroscoe
force-pushed
the
299-switch-to-modern-cmake-targets-with-more-cdash-urls
branch
from
06b1c02
to
e5e9be8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a temp PR that I am using to test this functionality.