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

Jump to bottom

Adds a Data Transport Layer to DYAD to support different ways of transferring data #24

Merged
merged 9 commits into from
Sep 7, 2023
Merged

Adds a Data Transport Layer to DYAD to support different ways of transferring data #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f5b8ffe
Creates Data Transport Layer (DTL) sublibraries to Core and Module
ilumsden Feb 27, 2023
1f06a9f
Modifies the wrapper library so that DYAD is initialized on library l…
ilumsden May 2, 2023
e8d7689
Changes default DTL mode to UCX
ilumsden May 18, 2023
027a9e1
Implements a unified DTL to replace the sub-DTLs
ilumsden Jul 7, 2023
4850a34
Adds macro to check Flux function return codes and adds more returns …
ilumsden Jul 29, 2023
1be0887
Changes default DTL mode to Flux RPC
ilumsden Jul 29, 2023
defc786
Adds missing flux_future_reset call to Flux RPC DTL
ilumsden Jul 31, 2023
846aa6d
Various PR review QoL changes:
ilumsden Sep 7, 2023
f346084
Converts the global variable for the RPC name into a macro to ensure …
ilumsden Sep 7, 2023
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
49 changes: 24 additions & 25 deletions .clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,25 @@
BasedOnStyle : google
SpaceBeforeParens : Always
IndentWidth : 4
BreakBeforeBraces : Linux
BasedOnStyle: Google
AlignAfterOpenBracket: Align
AlignOperands: 'true'
AlignTrailingComments: 'true'
AllowAllParametersOfDeclarationOnNextLine: 'false'
AllowShortFunctionsOnASingleLine: None
AllowShortIfStatementsOnASingleLine: Never
AllowShortLoopsOnASingleLine: 'false'
BinPackArguments: 'false'
BinPackParameters: 'false'
BreakBeforeBinaryOperators: NonAssignment
BreakBeforeBraces: Linux
BreakBeforeTernaryOperators: 'true'
ColumnLimit: '90'
ConstructorInitializerAllOnOneLineOrOnePerLine: 'true'
IndentWidth: '4'
PenaltyBreakBeforeFirstCallParameter: '10000000'
PenaltyBreakString: '10'
PenaltyReturnTypeOnItsOwnLine: '10000000'
PointerAlignment: Right
SpaceBeforeParens: Always
TabWidth: '4'
UseTab: Never
AllowShortIfStatementsOnASingleLine : false
ConstructorInitializerAllOnOneLineOrOnePerLine : true
AllowShortFunctionsOnASingleLine : false
AllowShortLoopsOnASingleLine : false
BinPackParameters : false
AllowAllParametersOfDeclarationOnNextLine : false
AlignTrailingComments : true
ColumnLimit : 80

# do not put all arguments on one line unless it's the same line as the call
PenaltyBreakBeforeFirstCallParameter : 10000000
PenaltyReturnTypeOnItsOwnLine : 65000
PenaltyBreakString : 10

# These improve formatting results but require clang 3.6/7 or higher
BreakBeforeBinaryOperators : NonAssignment
AlignAfterOpenBracket: true
BinPackArguments : true
AlignOperands : true
BreakBeforeTernaryOperators : true
AllowAllParametersOfDeclarationOnNextLine : false
AlwaysBreakAfterReturnType: None
AlwaysBreakAfterDefinitionReturnType: None
45 changes: 40 additions & 5 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,47 @@ AC_ARG_ENABLE([perfflow],
# TODO Add support for libb64 back once base64 encoding/decoding is fully complete
# AC_ARG_VAR([LIBB64_DIR], [root directory for libb64])

#############################################
# Define PKG_CHECK_VAR if it does not exist #
#############################################

# Macro is copied from https://github.com/pkgconf/pkgconf/blob/master/pkg.m4
# with minor modifications to change comments from using 'dnl' to '#'
m4_ifndef([PKG_CHECK_VAR], [
# PKG_CHECK_VAR(VARIABLE, MODULE, CONFIG-VARIABLE,
# [ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
# -------------------------------------------
# Since: 0.28
#
# Retrieves the value of the pkg-config variable for the given module.
AC_DEFUN([PKG_CHECK_VAR],
[AC_REQUIRE([PKG_PROG_PKG_CONFIG])dnl
AC_ARG_VAR([$1], [value of $3 for $2, overriding pkg-config])dnl
_PKG_CONFIG([$1], [variable="][$3]["], [$2])
AS_VAR_COPY([$1], [pkg_cv_][$1])
AS_VAR_IF([$1], [""], [$5], [$4])dnl
])# End of PKG_CHECK_VAR
])

########################
# Checks for libraries #
########################
# Check for the dl library (specifically, the "dlsym" function)
AC_CHECK_LIB([dl], [dlsym])
# FIXME: Replace 'main' with a function in '-ldyad_fstream':
# AC_CHECK_LIB([dyad_fstream], [main])
# FIXME: Replace 'main' with a function in '-ltap':
# AC_CHECK_LIB([tap], [main])
# Check for UCX v1.6.0 or higher (Required)
# TODO: make UCX optional based on a new AC_ARG_ENABLE flag
PKG_CHECK_MODULES([UCX],
[ucx >= 1.6.0]
)
PKG_CHECK_VAR([UCX_LIBDIR],
[ucx >= 1.6.0],
[libdir],
[],
[AC_MSG_FAILURE([Could not find libdir for UCX])]
)
AS_IF([test "x$UCX_LIBDIR" = "x"],
[AC_MSG_FAILURE([check_var succeeded, but value is incorrect])]
)
# Find and get info for Flux-Core using pkg-config
AX_FLUX_CORE
PKG_CHECK_MODULES([JANSSON],
Expand Down Expand Up @@ -153,7 +185,8 @@ AC_CHECK_FUNCS( \
# Add any necessary compilation flags #
#######################################
if test "x$enable_debug" = xyes; then
CPPFLAGS="$CPPFLAGS -DDYAD_FULL_DEBUG=1 -DDYAD_LOGGING_ON=1"
CFLAGS="$CFLAGS -DDYAD_FULL_DEBUG=1 -DDYAD_LOGGING_ON=1"
ilumsden marked this conversation as resolved.
Show resolved Hide resolved
CXXFLAGS="$CXXFLAGS -DDYAD_FULL_DEBUG=1 -DDYAD_LOGGING_ON=1"
fi

########################
Expand All @@ -162,7 +195,9 @@ fi
AC_CONFIG_FILES([Makefile
src/Makefile
src/utils/Makefile
src/utils/base64/Makefile
src/utils/libtap/Makefile
src/dtl/Makefile
src/core/Makefile
src/stream/Makefile
src/modules/Makefile
Expand Down
2 changes: 1 addition & 1 deletion src/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
SUBDIRS = utils modules core wrapper stream
SUBDIRS = utils dtl modules core wrapper stream
30 changes: 24 additions & 6 deletions src/core/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,29 @@
lib_LTLIBRARIES = libdyad_core.la
libdyad_core_la_SOURCES = dyad_core.c
libdyad_core_la_LIBADD = $(top_builddir)/src/utils/libutils.la $(top_builddir)/src/utils/libmurmur3.la $(FLUX_CORE_LIBS)
libdyad_core_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(top_builddir)/src/utils $(FLUX_CORE_CFLAGS)
libdyad_core_la_LDFLAGS = -export-symbols dyad_core.sym $(AM_LDFLAGS)
libdyad_core_la_SOURCES = \
dyad_core.c
libdyad_core_la_LIBADD = \
$(top_builddir)/src/dtl/libdyad_dtl.la \
$(UCX_LIBS) \
$(JANSSON_LIBS) \
$(FLUX_CORE_LIBS)
libdyad_core_la_CFLAGS = \
$(AM_CFLAGS) \
-I$(top_srcdir)/src/utils \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding -I for finding header files within such cases would make it harder to install the correct files later. The convension is to use till src only in -I so that other libraries can correctly link to DYAD at runtime.

Another general project level comment is that it is good to make distinction between public header files and private header files within a project.

This will help the installer only install the header files expected to be used by applications and rest would not be installed in include but would be compiled together in the so itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I absolutely agree with you @hariharan-devarajan, but I did this because we wanted to stay (relatively) consistent with Flux. This flux-core repo does something similar to this in its Makefile.ams.

However, regardless of what Flux does and how much we want to follow that, I also believe that any changes we might want to make regarding organization of files should belong in a separate PR.

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 that we should put more effort to clean up installation and make distinction between public headers and private headers. This can be worked on in a followup PR.

-I$(top_srcdir)/src/utils/base64 \
-I$(top_srcdir)/src/dtl \
$(UCX_CFLAGS) \
$(JANSSON_CFLAGS) \
$(FLUX_CORE_CFLAGS) \
-DBUILDING_DYAD=1 \
-fvisibility=hidden
libdyad_core_la_CPPFLAGS =
libdyad_core_la_LDFLAGS = \
-Wl,-rpath,'$(UCX_LIBDIR)' \
$(AM_LDFLAGS)
if PERFFLOW
libdyad_core_la_LIBADD += $(PERFFLOW_LIBS)
libdyad_core_la_CPPFLAGS += $(PERFFLOW_PLUGIN_CPPFLAGS) $(PERFFLOW_CFLAGS) -DDYAD_PERFFLOW=1
libdyad_core_la_CFLAGS += $(PERFFLOW_CFLAGS) -DDYAD_PERFFLOW=1
libdyad_core_la_CPPFLAGS += $(PERFFLOW_PLUGIN_CPPFLAGS)
endif

include_HEADERS = dyad_core.h dyad_envs.h dyad_rc.h dyad_flux_log.h
include_HEADERS = dyad_core.h dyad_envs.h
Loading