CMake modernization: start using BLT #18933

biagas · 2023-09-18T23:14:18Z

Description

Demonstrate use of BLT, and export of VisIt targets.

BLT: requires SOURCES to be passed to blt_add_library, so VisIt libraries whose sources live in subdirectories must have the subdirs populate a SOURCES list.
The macro visit_append_list was added for this reason.
It can be used to populate any list, and caller is responsible for clearing the created cache vars to avoid polluting CMakeCache.txt.
visitcommon and avtdbatts libraries demonstrate its use.

Simple libraries also updated: lightweight_visit_vtk, visit_vtk, and avtmath.

For export of visit targets, visit_install_export_targets function was added.
The only difference it offers beyond VISIT_INSTALL_TARGETS is use of the EXPORT option in the install command.

All INCLUDES use $<BUILD_INTERFACE: ...> and $<INSTALL_INTERFACE: ...>.
This ensures that VisIt includes are listed with a path relative to the install directory, and makes the exports 'relocatable'.

vtSkew.h was moved from visit_vtk/full to visit_vtk/lightweight, because lightweight includes it but should not have dependencies on visit_vtk/full.

To demonstrate what the export set for VisIt will be, tmp\cmake dir was created to temporarily house the files that get generated and installed. They won't be part of the PR when it is ready for final review.
These cmake files won't be of much use until all of VisIt has been updated to properly export targets, and when Third party targets are properly accounted for/found in visitConfig.cmake (or other file(s) included by visitConfig.cmake).

I have verified the pluginVsInstall tests work with these changes.
Other tests also work, but aren't as much of a concern as basic VisIt functionality isn't being changed.

Type of change

Bug fix~~
New feature~~
Documentation update~~
Other~~

How Has This Been Tested?

Reminders:

Please follow the style guidelines of this project.
Please perform a self-review of your code before submitting a PR and asking others to review it.
Please assign reviewers (see VisIt's PR procedures for more information).

Checklist:

I have commented my code where applicable.~~
I have updated the release notes.~~
I have made corresponding changes to the documentation.~~
I have added debugging support to my changes.~~
I have added tests that prove my fix is effective or that my feature works.~~
I have confirmed new and existing unit tests pass locally with my changes.~~
I have added new baselines for any new tests to the repo.~~
I have NOT made any changes to protocol or public interfaces in an RC branch.~~

biagas · 2023-09-18T23:58:18Z

Added reviewers to get comments/questions.

biagas · 2023-09-19T00:27:50Z

The RTD failure is due to a docs reference to avtdbatts CMakeLists.txt files which have changed.
I will update it prior to taking this out of DRAFT.

JustinPrivitera

The switch to BLT makes a lot of the code here more clear, in my opinion. This looks good.

markcmiller86 · 2023-09-19T23:27:42Z

src/CMake/VisItMacros.cmake

+##############################################################################
+# This macro appends to a CACHE var list denoted by the NAME argument.
+# Caller is responsible for unsetting the CACHE var when no longer needed
+# to avoid polluting the CACHE.


For the less CMake-inclined, what is the actually CMake syntax/function to do that?

exactly what you see on this line:

visit/src/CMake/VisItMacros.cmake

Line 282 in 374d3d5

set(${arg_NAME} ${${arg_NAME}} ${arg_ITEMS} CACHE STRING "" FORCE)

I would have preferred to use list(APPEND ...), and keep all lists in local scope to the PARENT dir, but the only way for a SUBDIR to set a variable that could be used by the PARENT dir and PEER dirs is with set(... CACHE FORCE).
I created the macro to make things easier to read and understand.

BTW, this was all necessitated by the fact that blt_add_library requires the SOURCES list.
With straight CMake, add_library can be called without any sources, then subdirs can add to a library's source list by use of target_sources command.

Well, is that a strength or weakness of BLT? I mean, does BLT's appraoch somehow fix or prevent issues that CMake's allows/enables or vice versa?

I think BLT makes some things with CMake simpler. Other things not so much. Could be just the complexity of VisIt and use-cases they haven't encountered yet. They do some bookkeeping and extra checks under the covers that could be problematic. I encountered an issue with OSPRAY_LIBRARIES yesterday I am trying to figure out the best way to work around. I could just not add it to the DEPENDS_ON list, but call target_link_libraries directly to avoid the issue BLT introduces.

Ok, well, as you work through issues, I'd bet BLT team would be interested to learn your experiences. Its concievable some small changes there could make it easier to use in these cases.

markcmiller86 · 2023-09-19T23:30:18Z

src/avt/DBAtts/CMakeLists.txt

+    DEPENDS_ON visitcommon)
+
+unset(avtdbatts_SOURCES CACHE)
+unset(avtdbatts_INCLUDES CACHE)


Ok, so this maybe answers my prev. question. Is this the basic pattern...on var for headers and a companion var for sources both of which need to be unset after the blt_add_library?

Yes, if visit_append_list is called, then unset for the created lists must be called as well, otherwise the Cache gets polluted, but also multiple calls to cmake-configure will duplicate items in the lists.

Well, pollution sounds bad. Any way to craft things such that when CMake'ing is nearly complete (e.g. at the bottom of top-level CMakeLists.txt), some operations are performed to detect that pollution has happened and issue a warning message of some kind...e.g. there appears to be missing unset() calls for <list-name>

There isn't a way (beyond manual parsing of the Cache) for the root CMakeLists.txt to know what vars have been created.

The need for 'unset' is indeed a possibly bad thing, but I could not figure a way around it except not use blt, or move all code out of subdirs and have one monolithic CMakeLists in the parent dirs again (like we used to).

I wasn't sure I totally understood here either but is unset in fact required or just a nice thing to do? I mean, do we really care if a bunch of variables that list source or include files get left around? If we adopt a standard naming convention, which you demonstrate already in the PR, there isn't really any possibility of a collision is there?

I meant at the beginning of the CMakeLists.txt file that calls blt_add_library with the lists.

Ah, then how about a visit_blt_add_library that just does that in a combined operation?

@markcmiller86 Thanks for the suggestion. I implemented visit_add_library, which calls blt_add_library and also makes the unset calls for the cache vars. It doesn't matter if the vars don't exist, so it unsets all that may have been created. The list may need to be updated as I go through the other libraries and update them for BLT.
What do you think of the change?

I like it! 💪🏻

It seems to resolve a potential maint. issue.

Lets see what others think.

I think adding visit_add_library that calls both the blt_add_library and unset functions is a good idea. That makes it easier to prevent mistakes in the future.

markcmiller86 · 2023-09-19T23:32:21Z

src/avt/Math/CMakeLists.txt

 if(VTK_VERSION VERSION_EQUAL "8.1.0")
    set(vtklibs vtkCommonCore)
 else()
    set(vtklibs VTK::CommonCore)
 endif()

-# Add link directories
-LINK_DIRECTORIES(${LIBRARY_OUTPUT_DIRECTORY} )
+blt_add_library(


So, this is a tad different than prev. example. This is a blt_add_library happening down in a leaf subdirectory. So, just list the sources and includes explicitly there because that list isn't getting shared or extended by any other subdirectories.

markcmiller86 · 2023-09-19T23:34:28Z

src/CMake/VisItMacros.cmake

+##############################################################################
+
+macro(visit_append_list)
+    cmake_parse_arguments(arg "" "NAME" "ITEMS" ${ARGN})


Is the ITEMS label really needed? I mean won't the first thingy after NAME be the name and everything else be the items to add to the list? Maybe this way is more cmake-standard?

ITEMS isn't needed, neither is NAME, but combined with cmake_parse_arguments, use of named arguments is a way to ensure that required arguments are present. When the macro is called, use of named arguments also makes the functionality of what is being called a bit clearer.

markcmiller86 · 2023-09-19T23:36:20Z

src/avt/Math/CMakeLists.txt

-# Add link directories
-LINK_DIRECTORIES(${LIBRARY_OUTPUT_DIRECTORY} )
+blt_add_library(
+    NAME       avtmath


Are these missing pre-fixing by $(CMAKE_CURRENT_SOURCE_DIR}/ as in src/common/Exceptions/Database/CMakeLists.txt

CMAKE_CURRENT_SOURCE_DIR isn't needed here because the files are being processed in this this directory, and if there is no path, or even relative path, CMake assumes CWD.
CMAKE_CURRENT_SOURCE_DIR is needed in src/common/XXX/CMakeLists.txt because the files are being appended to a list and processed in a different dir (/src/common/CMakeLists.txt).

markcmiller86 · 2023-09-19T23:38:34Z

src/common/state/CMakeLists.txt

-    WindowInformation.C
-    Xfer.C
-    XMLNode.C)
+visit_append_list(


Just curious...does CMake support a short-hand notation for all .C files in this directory instead of having to list them all and then constantly edit that list as we had or delete them.

There is not shortcut, except use of file(GLOB) which is highly discouraged.

Why is file(GLOB...) highly discouraged?

Its really too bad because it seems like the CMake equivalent of GNU Make's wildcard function and pattern rules

I think it has to do with CMake's ability to track whether a source file has changed. I don't like it myself because I like looking at the file and knowing exactly what is being added to the project.

markcmiller86 · 2023-09-19T23:40:02Z

src/visit_vtk/full/CMakeLists.txt

+blt_add_library(
+    NAME        visit_vtk
+    SOURCES     InitVTK.C
+                vtkAxisDepthSort.C


Ok, so these source file name listings are also not prefixed by ${CMAKE_CURRENT_SOURCE_DIR}/. Not sure what rationale is except maybe because we don't need releative paths here?

When you call blt_add_library in the same directory as source, the full-path-to-source isn't needed.
When source files live in subdirectories, and they are appended to a list as I have done, then full path is needed, hence use of ${CMAKE_CURRENT_SOURCE_DIR} in those cases.

A case could be made for consistency, and use of CMAKE_CURRENT_SOURCE_DIR everywhere.

tmp/cmake/visitConfig.cmake

biagas · 2023-09-19T23:58:53Z

The switch to BLT makes a lot of the code here more clear, in my opinion. This looks good.

@JustinPrivitera thank you for taking time to look this over

markcmiller86

This looks good, cleaner than previous. The ~310 lines in tmp will eventually get removed and every existing file looses a lot of lines

markcmiller86 · 2023-09-21T01:03:22Z

src/CMake/VisItMacros.cmake

+    unset(${arg_NAME}_INCLUDES CACHE)
+    unset(${arg_NAME}_DEFINES CACHE)
+    unset(${arg_NAME}_DEPENDS CACHE)
+    unset(${arg_NAME}_FEATURES CACHE)


So, I see ${arg_NAME}_FEATURES here but it is not used in blt_add_library()

It is not an option for blt_add_library. We have to call target_compile_features. I just made a commit that makes use of FEATURES.

markcmiller86 · 2023-09-21T01:03:53Z

src/CMake/VisItMacros.cmake

+    cmake_parse_arguments(arg
+        "${options}" "${singleValueArgs}" "${multiValueArgs}" ${ARGN} )
+
+    # Sanity checks


Does blt_add_library() already perform these sanity checks? If so, I'd argue no need to do them here.

Yes, they have their own. But if we remove the check from visit_add_library, the error message will reference blt_add_library.

Isn't that ok though? Maybe if the function was named visit_blt_add_library() or vblt_add_library() or at least had the name blt_add_library in it, I think its ok. I think developers would connect those dots.

biagas · 2023-09-21T18:51:27Z

src/CMake/VisItMacros.cmake

+    unset(${arg_NAME}_SOURCES CACHE)
+    unset(${arg_NAME}_INCLUDES CACHE)
+    unset(${arg_NAME}_DEFINES CACHE)
+    unset(${arg_NAME}_DEPENDS CACHE)
+    unset(${arg_NAME}_FEATURES CACHE)


Ran across another problem with the use of CACHE vars for the lists and having to unset them.
When we build serial and parallel versions of a library, the SOURCES and INCLUDES lists are the same, so it makes sense to only create one list and use a name without the _ser/_par suffices of the target name. For instance, avtpipeline_SOURCES instead of avtpipeline_ser_SOURCES.

But then, the unset calls used here won't work.
I could add logic to remove _ser/_par from the target name before making the unset calls so that they would work.
That creates another problem in that after creating the serial library, all the lists would be unset, and the call to add the parallel library would fail.

Maybe I should abandon use of blt_add_library, and the need to create these lists in order to satisfy its need to supply SOURCES with the call.
Keep the current use of CMake's add_library call without sources, and subdirs add their sources via target_sources call.
Could then use blt_register_library if we really need blt to keep track of our library targets.

Hmm...I don't like abandoning BLT functionality here. Circling back to your comment about CMake's add_library and target_sources functionality vs. BLT's blt_add_library, I think BLT may have been going for a simplification of interface. But, that siimplification forces one to build this list before blt_add_library can be called.

I read in the BLT documentation...

This macro creates a true CMake target that can be altered by other CMake commands like normal, such as set_target_property().

Does this mean you can maybe use blt_add_library with essentially empty lists and use CMake's target_sources to build out those lists from subdirs as you would have via straight CMake?

Does this mean you can maybe use blt_add_library with essentially empty lists and use CMake's target_sources to build out those lists from subdirs as you would have via straight CMake?

I tried passing an empty SOURCES list and it bombs.

So for two targets derived essentially from the same sources just compiled with different flags, is it really a bad thing to wind up with two instances (e.g. one with _ser and the other with _par in their names) of cache variables representing the lists of sources names for those targets?

Sorry, at the moment, feel like I am already jumping through hoops to use blt, creating extra lists containing the same items seems overkill and unnecessary work.

I think creating the second list might be doable, without having to actually list every source file again by creating a copy for the par libraries, eg before the call to visit_add_library for the serial library:
set(somelib_SOURCES_PAR ${somelib_SOURCES})
Then use the _PAR lists in the call to add the parallel library.

cyrush · 2023-09-22T16:03:00Z

Details here are important to me and I want to help, but I won't have cycles until sometime in October. If possible we should use a special topics meeting to discuss high bandwidth.

biagas · 2023-09-22T16:10:33Z

Details here are important to me and I want to help, but I won't have cycles until sometime in October. If possible we should use a special topics meeting to discuss high bandwidth.

Sounds good. I will be on vacation from Oct 2 - 13th. Would Oct 19th work?

cyrush · 2023-09-22T16:49:29Z

Details here are important to me and I want to help, but I won't have cycles until sometime in October. If possible we should use a special topics meeting to discuss high bandwidth.

Sounds good. I will be on vacation from Oct 2 - 13th. Would Oct 19th work?

A few of us will be on travel 10/15-10/20, so 10/26?

biagas · 2023-09-22T16:51:24Z

Details here are important to me and I want to help, but I won't have cycles until sometime in October. If possible we should use a special topics meeting to discuss high bandwidth.

Sounds good. I will be on vacation from Oct 2 - 13th. Would Oct 19th work?

A few of us will be on travel 10/15-10/20, so 10/26?

Sounds good.

Update visitcommon, avtdbatts, avtmath, visit_vtk, lightweight_visit_vtk. Added export for visit targets. Confirm pluginVsInstall tests still work.

The macro will make the `unset` calls for vars that may have been created by `visit_append_list` (for SOURCES, INCLUDES,etc).

Add call to target_compile_features if FEATURES arg is present.

Modify a few operators and databases to demonstrate its usage.

cyrush · 2024-05-09T17:16:11Z

Linking BLT requests:

LLNL/blt#689
LLNL/blt#690

biagas requested review from brugger1, cyrush, markcmiller86 and JustinPrivitera September 18, 2023 23:57

JustinPrivitera previously approved these changes Sep 19, 2023

View reviewed changes

markcmiller86 reviewed Sep 19, 2023

View reviewed changes

markcmiller86 previously approved these changes Sep 20, 2023

View reviewed changes

biagas dismissed stale reviews from markcmiller86 and JustinPrivitera via 66f234e September 20, 2023 21:22

markcmiller86 reviewed Sep 21, 2023

View reviewed changes

biagas commented Sep 21, 2023

View reviewed changes

biagas added 6 commits April 18, 2024 11:05

Inital stab at using BLT as part of CMake modernization.

0307e69

Update visitcommon, avtdbatts, avtmath, visit_vtk, lightweight_visit_vtk. Added export for visit targets. Confirm pluginVsInstall tests still work.

Remove unnecessary setting of _IMPORT_PREFIX.

c0f08de

Add tmp dir with export-generate cmake files for example.

f7317fe

Re-add removal of CMakeCache.txt in recmake_visit.sh.

2b738ce

Add visit_add_library macro (wrapper around blt_add_library).

9354020

The macro will make the `unset` calls for vars that may have been created by `visit_append_list` (for SOURCES, INCLUDES,etc).

Add FEATURES option to visit_add_library.

7c48b9a

Add call to target_compile_features if FEATURES arg is present.

biagas force-pushed the task/biagas/start_using_blt branch from 54f47bd to 7c48b9a Compare April 18, 2024 22:18

biagas added 2 commits May 8, 2024 10:30

Add functions for adding plugins.

943ef2e

Modify a few operators and databases to demonstrate its usage.

Fix cut-n-paste error.

2b29c3a

CMake modernization: start using BLT #18933

Are you sure you want to change the base?

CMake modernization: start using BLT #18933

Conversation

biagas commented Sep 18, 2023 • edited

Description

Type of change

How Has This Been Tested?

Reminders:

Checklist:

biagas commented Sep 18, 2023

biagas commented Sep 19, 2023

JustinPrivitera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcmiller86 Sep 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcmiller86 Sep 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biagas commented Sep 19, 2023

markcmiller86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcmiller86 Sep 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrush commented Sep 22, 2023

biagas commented Sep 22, 2023

cyrush commented Sep 22, 2023

biagas commented Sep 22, 2023

cyrush commented May 9, 2024

biagas commented Sep 18, 2023 •

edited

markcmiller86 Sep 20, 2023 •

edited

markcmiller86 Sep 20, 2023 •

edited

markcmiller86 Sep 21, 2023 •

edited