Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Teuchos: Add optional yaml-cpp parser #12599

Merged
merged 7 commits into from
Jan 22, 2024
Merged

Teuchos: Add optional yaml-cpp parser #12599

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5eb03a9
add optional yaml-cpp parser and flag
cwschilly Dec 11, 2023
eb76740
Teuchos: move yaml-cpp dependency to TeuchosParameterList and refine …
cwschilly Jan 3, 2024
a125832
Teuchos: remove leftover debugging statement
cwschilly Jan 4, 2024
3490517
Teuchos: support long long parsing and read in null nodes as empty su…
cwschilly Jan 10, 2024
60b9c7d
Teuchos: allow for strings ending in ll or LL
cwschilly Jan 10, 2024
9eddcb7
Teuchos: improve support for long long, add yes/no functionality for …
cwschilly Jan 15, 2024
492d668
Teuchos: improve support for bool parsing
cwschilly Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/teuchos/core/cmake/Dependencies.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
TRIBITS_PACKAGE_DEFINE_DEPENDENCIES(
LIB_OPTIONAL_PACKAGES Kokkos
LIB_OPTIONAL_TPLS BinUtils Boost MPI ARPREC QD QT quadmath yaml-cpp Pthread Valgrind
LIB_OPTIONAL_TPLS BinUtils Boost MPI ARPREC QD QT quadmath Pthread Valgrind
)

TRIBITS_ALLOW_MISSING_EXTERNAL_PACKAGES(Kokkos)
5 changes: 0 additions & 5 deletions packages/teuchos/core/src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,6 @@ ENDIF()
# FIXME: Remove this hard-coded setting. We always have C++11 now.
SET(HAVE_TEUCHOSCORE_CXX11 ON)

SET(YAML_CPP_DEF 0)
IF(${PACKAGE_NAME}_ENABLE_yaml-cpp)
SET(YAML_CPP_DEF 1)
ENDIF()

TRIBITS_CONFIGURE_FILE(${PARENT_PACKAGE_NAME}_config.h)
TRIBITS_CONFIGURE_FILE(${PACKAGE_NAME}_config.h)

Expand Down
4 changes: 4 additions & 0 deletions packages/teuchos/parameterlist/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@

TRIBITS_SUBPACKAGE(ParameterList)

IF(TPL_ENABLE_yaml-cpp)
SET(HAVE_TEUCHOSPARAMETERLIST_YAMLCPP ON)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is okay, but unfortunate, that the name of this macro HAVE_TEUCHOSPARAMETERLIST_YAMLCPP does not exactly match name name of the automatically TriBITS-generated macro HAVE_TEUCHOSPARAMETERLIST_YAML-CPP as described in here and here. The problem is that the char - is not valid in a C/C++ macro name (see here). The char - is the minus sign in C/C++.

I wish someone had named this TPL yamcpp or even YamlCpp instead of yaml-cpp. Then a statement like this and a new variable this would not have been necessary.

But I will note that yaml-cpp is the only external package/TPL defined in the file Trilinos/TPLsList.cmake and the only internal package defined in the file Trilinos/PackagesList.cmake (or all of the subpackage names in the Dependencies.cmake files) that uses - in the name.

One option is for TriBITS to automatically adjust package names that have - inside of them to instead use _ when creating this HAVE_<PACKAGE_NAME_UC>_<UPSTREAM_PACKAGE_NAME_UC> macro name which is
a valid C/C++ macro name. But that adds more complexity to TriBITS and would break backward compatibility (but only in this one case for yaml-cpp?) Another option is to update TriBITS to assert that all external and internal package names to match the regex [a-zA-Z][a-zA-Z0-9_]+ which is required for all C/C++ identifiers.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion would be to do the right long term thing which I think would be to rename the tpl to "yamlcpp". The TPL has very little use after removal from the plist years ago. The only actual use is for three snapshotted packages - kokkos-kernels, percept and krino. KK uses it for performance testing while percept and krino have it listed as an optional LIB dependence. It probably would not be that disruptive of a change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically isn't this similar to superlu_dist? And there we (maybe everybody?) just dropped the underscore as Roger suggests, so I'd be in that boat too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the idea of renaming the library. I suggest that be done in a subsequent PR.

ENDIF()

ADD_SUBDIRECTORY(src)

TRIBITS_ADD_TEST_DIRECTORIES(test)
Expand Down
1 change: 1 addition & 0 deletions packages/teuchos/parameterlist/cmake/Dependencies.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
TRIBITS_PACKAGE_DEFINE_DEPENDENCIES(
LIB_REQUIRED_PACKAGES TeuchosCore TeuchosParser
LIB_OPTIONAL_TPLS yaml-cpp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

)
6 changes: 6 additions & 0 deletions packages/teuchos/parameterlist/cmake/TeuchosParameterList_config.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#ifndef TEUCHOSPARAMETERLIST_CONFIG_H
#define TEUCHOSPARAMETERLIST_CONFIG_H

#cmakedefine HAVE_TEUCHOSPARAMETERLIST_YAMLCPP

#endif // TEUCHOSPARAMETERLIST_CONFIG_H
Comment on lines +1 to +6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

10 changes: 10 additions & 0 deletions packages/teuchos/parameterlist/src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@

#
# A) Package-specific configuration options
#

TRIBITS_CONFIGURE_FILE(${PACKAGE_NAME}_config.h)

#
# B) Define the header and source files (and include directories)
Expand All @@ -7,7 +12,12 @@
SET(HEADERS "")
SET(SOURCES "")

APPEND_SET(HEADERS
${CMAKE_CURRENT_BINARY_DIR}/${PACKAGE_NAME}_config.h
)

TRIBITS_INCLUDE_DIRECTORIES(${CMAKE_CURRENT_SOURCE_DIR})
TRIBITS_INCLUDE_DIRECTORIES(${CMAKE_CURRENT_BINARY_DIR})

TRIBITS_SET_AND_INC_DIRS(DIR ${CMAKE_CURRENT_SOURCE_DIR})
APPEND_GLOB(HEADERS ${DIR}/*.hpp)
Expand Down
1 change: 1 addition & 0 deletions packages/teuchos/parameterlist/src/Teuchos_ParameterList.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
\brief Templated Parameter List class
*/

#include "TeuchosParameterList_config.h"
#include "Teuchos_ParameterListExceptions.hpp"
#include "Teuchos_ParameterListModifier.hpp"
#include "Teuchos_ParameterEntry.hpp"
Expand Down
Loading