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.

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

DAOS-623 build: Use AddMethod for adding methods to environments. #10654

Merged
merged 8 commits into from
Nov 3, 2022
36 changes: 16 additions & 20 deletions SConstruct
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@ wrap scons-3.""")

SCons.Warnings.warningAsException()

try:
input = raw_input # pylint: disable=redefined-builtin
except NameError:
pass


def get_version(env):
""" Read version from VERSION file """
Expand All @@ -50,6 +45,8 @@ API_VERSION = f'{API_VERSION_MAJOR}.{API_VERSION_MINOR}.{API_VERSION_FIX}'

def update_rpm_version(version, tag):
""" Update the version (and release) in the RPM spec file """

# pylint: disable=consider-using-f-string
daltonbohning marked this conversation as resolved.
Show resolved Hide resolved
spec = open("utils/rpms/daos.spec", "r").readlines() # pylint: disable=consider-using-with
current_version = 0
release = 0
Expand Down Expand Up @@ -151,6 +148,7 @@ def build_misc(build_prefix):
def scons(): # pylint: disable=too-many-locals,too-many-branches
"""Execute build"""
if COMMAND_LINE_TARGETS == ['release']:
# pylint: disable=consider-using-f-string
try:
# pylint: disable=import-outside-toplevel
import pygit2
Expand Down Expand Up @@ -329,7 +327,6 @@ def scons(): # pylint: disable=too-many-locals,too-many-branches
# Scons strips out the environment, however to be able to build daos using the interception
# library we need to add a few things back in.
if 'LD_PRELOAD' in os.environ:
# pylint: disable=invalid-sequence-index
env['ENV']['LD_PRELOAD'] = os.environ['LD_PRELOAD']

for key in ['D_LOG_FILE', 'DAOS_AGENT_DRPC_DIR', 'D_LOG_MASK', 'DD_MASK', 'DD_SUBSYS']:
Expand All @@ -348,7 +345,6 @@ def scons(): # pylint: disable=too-many-locals,too-many-branches

if 'VIRTUAL_ENV' in os.environ:
env.PrependENVPath('PATH', os.path.join(os.environ['VIRTUAL_ENV'], 'bin'))
# pylint: disable=invalid-sequence-index
env['ENV']['VIRTUAL_ENV'] = os.environ['VIRTUAL_ENV']

prereqs = PreReqComponent(env, opts, commits_file)
Expand Down Expand Up @@ -376,18 +372,21 @@ def scons(): # pylint: disable=too-many-locals,too-many-branches

set_defaults(env, daos_version)

base_env = env.Clone()

base_env_mpi = env.Clone()
# Add project specific methods to SCons environments.
daos_build.setup(env)
compiler_setup.setup(env)

compiler_setup.base_setup(env)
base_env = env.Clone()

if not GetOption('help') and not GetOption('clean'):
mpi = daos_build.configure_mpi(base_env_mpi)
if not mpi:
base_env_mpi = env.d_configure_mpi()
if not base_env_mpi:
print("\nSkipping compilation for tests that need MPI")
print("Install and load mpich or openmpi\n")
base_env_mpi = None
else:
base_env_mpi = None

env.compiler_setup()

args = GetOption('analyze_stack')
if args is not None:
Expand All @@ -407,12 +406,9 @@ def scons(): # pylint: disable=too-many-locals,too-many-branches
buildinfo.save('.build_vars.json')
# also install to $PREFIX/lib to work with existing avocado test code
if prereqs.test_requested():
daos_build.install(env, "lib/daos/",
['.build_vars.sh', '.build_vars.json'])
env.Install('$PREFIX/lib/daos/TESTING/ftest/util',
['site_scons/env_modules.py'])
env.Install('$PREFIX/lib/daos/TESTING/ftest/',
['ftest.sh'])
env.Install('$PREFIX/lib/daos', ['.build_vars.sh', '.build_vars.json'])
env.Install('$PREFIX/lib/daos/TESTING/ftest/util', ['site_scons/env_modules.py'])
env.Install('$PREFIX/lib/daos/TESTING/ftest/', ['ftest.sh'])

env.Install("$PREFIX/lib64/daos", "VERSION")

Expand Down
7 changes: 6 additions & 1 deletion site_scons/compiler_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
'-Wno-unused-function']


def base_setup(env):
def _base_setup(env):
"""Setup the scons environment for the compiler

Include all our preferred compile options for the chosen
Expand Down Expand Up @@ -84,3 +84,8 @@ def base_setup(env):
env.AppendIfSupported(CCFLAGS=PP_ONLY_FLAGS)

env['BSETUP'] = compiler


def setup(env):
"""Add daos specific method to environment"""
env.AddMethod(_base_setup, 'compiler_setup')
63 changes: 32 additions & 31 deletions site_scons/daos_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from SCons.Script import Depends
from SCons.Script import Exit
from env_modules import load_mpi
import compiler_setup

libraries = {}
missing = set()
Expand All @@ -23,12 +22,11 @@ def __hash__(self):
return hash(self.lstr)


def add_rpaths(env, install_off, set_cgo_ld, is_bin):
def _add_rpaths(env, install_off, set_cgo_ld, is_bin):
"""Add relative rpath entries"""
if GetOption('no_rpath'):
if set_cgo_ld:
env.AppendENVPath("CGO_LDFLAGS", env.subst("$_LIBDIRFLAGS "),
sep=" ")
env.AppendENVPath("CGO_LDFLAGS", env.subst("$_LIBDIRFLAGS "), sep=" ")
return
env.AppendUnique(RPATH_FULL=['$PREFIX/lib64'])
rpaths = env.subst("$RPATH_FULL").split()
Expand All @@ -45,18 +43,17 @@ def add_rpaths(env, install_off, set_cgo_ld, is_bin):
continue
relpath = os.path.relpath(rpath, prefix)
if relpath != rpath:
joined = os.path.normpath(os.path.join(install_off, relpath))
path = r'\$$ORIGIN/%s' % (joined)
if set_cgo_ld:
env.AppendENVPath("CGO_LDFLAGS", "-Wl,-rpath=$ORIGIN/%s/%s" %
(install_off, relpath), sep=" ")
env.AppendENVPath("CGO_LDFLAGS", f'-Wl,-rpath=$ORIGIN/{install_off}/{relpath}',
sep=" ")
else:
env.AppendUnique(RPATH=[DaosLiteral(path)])
joined = os.path.normpath(os.path.join(install_off, relpath))
env.AppendUnique(RPATH=[DaosLiteral(fr'\$$ORIGIN/{joined}')])
for rpath in rpaths:
path = os.path.join(prefix, rpath)
if is_bin:
# NB: Also use full path so intermediate linking works
env.AppendUnique(LINKFLAGS=["-Wl,-rpath-link=%s" % path])
env.AppendUnique(LINKFLAGS=[f'-Wl,-rpath-link={path}'])
else:
# NB: Also use full path so intermediate linking works
env.AppendUnique(RPATH=[path])
Expand All @@ -65,11 +62,12 @@ def add_rpaths(env, install_off, set_cgo_ld, is_bin):
env.AppendENVPath("CGO_LDFLAGS", env.subst("$_LIBDIRFLAGS $_RPATH"), sep=" ")


def add_build_rpath(env, pathin="."):
def _add_build_rpath(env, pathin="."):
"""Add a build directory to rpath"""

path = Dir(pathin).path
env.AppendUnique(LINKFLAGS=["-Wl,-rpath-link=%s" % path])
env.AppendENVPath("CGO_LDFLAGS", "-Wl,-rpath-link=%s" % path, sep=" ")
env.AppendUnique(LINKFLAGS=[f'-Wl,-rpath-link={path}'])
env.AppendENVPath('CGO_LDFLAGS', f'-Wl,-rpath-link={path}', sep=' ')
# We actually run installed binaries from the build area to generate
# man pages. In such cases, we need LD_LIBRARY_PATH set to pick up
# the dependencies
Expand Down Expand Up @@ -117,7 +115,7 @@ def _add_lib(libtype, libname, target):
libraries[libname][libtype] = target


def run_command(env, target, sources, daos_libs, command):
def _run_command(env, target, sources, daos_libs, command):
"""Run Command builder"""
static_deps, shared_deps = _known_deps(env, LIBS=daos_libs)
result = env.Command(target, sources + static_deps + shared_deps, command)
Expand All @@ -141,7 +139,7 @@ def library(env, *args, **kwargs):
"""build SharedLibrary with relative RPATH"""
denv = env.Clone()
denv.Replace(RPATH=[])
add_rpaths(denv, kwargs.get('install_off', '..'), False, False)
_add_rpaths(denv, kwargs.get('install_off', '..'), False, False)
lib = denv.SharedLibrary(*args, **kwargs)
libname = _get_libname(*args, **kwargs)
_add_lib('shared', libname, lib)
Expand All @@ -156,7 +154,7 @@ def program(env, *args, **kwargs):
denv = env.Clone()
denv.AppendUnique(LINKFLAGS=['-pie'])
denv.Replace(RPATH=[])
add_rpaths(denv, kwargs.get('install_off', '..'), False, True)
_add_rpaths(denv, kwargs.get('install_off', '..'), False, True)
prog = denv.Program(*args, **kwargs)
static_deps, shared_deps = _known_deps(env, **kwargs)
Depends(prog, static_deps)
Expand All @@ -169,7 +167,7 @@ def test(env, *args, **kwargs):
denv = env.Clone()
denv.AppendUnique(LINKFLAGS=['-pie'])
denv.Replace(RPATH=[])
add_rpaths(denv, kwargs.get("install_off", None), False, True)
_add_rpaths(denv, kwargs.get("install_off", None), False, True)
testbuild = denv.Program(*args, **kwargs)
static_deps, shared_deps = _known_deps(env, **kwargs)
Depends(testbuild, static_deps)
Expand All @@ -182,13 +180,6 @@ def add_static_library(name, target):
_add_lib('static', name, target)


def install(env, subdir, files):
"""install file to the subdir"""
denv = env.Clone()
path = "$PREFIX/%s" % subdir
denv.Install(path, files)


def _find_mpicc(env):
"""find mpicc"""

Expand All @@ -199,7 +190,7 @@ def _find_mpicc(env):
env.Replace(CC="mpicc")
env.Replace(LINK="mpicc")
env.PrependENVPath('PATH', os.path.dirname(mpicc))
compiler_setup.base_setup(env)
env.compiler_setup()

return True

Expand All @@ -220,11 +211,13 @@ def _configure_mpi_pkg(env):
return True


def configure_mpi(env):
def _configure_mpi(self):
"""Check if mpi exists and configure environment"""

if GetOption('help'):
return True
return None

env = self.Clone()

env['CXX'] = None

Expand All @@ -235,8 +228,16 @@ def configure_mpi(env):
if not load_mpi(mpi):
continue
if _find_mpicc(env):
print("%s is installed" % mpi)
return True
print("No %s installed and/or loaded" % mpi)
print(f'{mpi} is installed')
return env
print(f'No {mpi} installed and/or loaded')
print("No MPI installed")
return False
return None


def setup(env):
"""Add daos specific methods to environment"""
env.AddMethod(_add_build_rpath, 'd_add_build_rpath')
env.AddMethod(_configure_mpi, 'd_configure_mpi')
env.AddMethod(_run_command, 'd_run_command')
env.AddMethod(_add_rpaths, 'd_add_rpaths')
Comment on lines +238 to +243
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the real change in this PR, the rest is just fluff.

  1. Is this worthwhile?
  2. do we want a common 'd_' prefix for all our methods to make it clear they're non-standard.
  3. Should we copy the CamelCase naming of existing methods?
  4. Assuming this is worthwhile should we do it for all daos_build functions (which will touch more code than this PR).

Copy link
Contributor

@jolivier23 jolivier23 Oct 24, 2022

Choose a reason for hiding this comment

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

For point 2, I think we already have other precedent where we add stuff and don't name it specially. For instance, Configure CheckFlag and CheckFlagCC. I think it would be fine to do something like AddBuildRPATH. For point 4, possibly. But I wouldn't do that in a single PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would mean that we'd replace daos_build.library() with env.Library(), yet there is already a method with that name, the one that we're wrapping/

Copy link
Contributor

Choose a reason for hiding this comment

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

You could do something like DAOSLibrary, I suppose

2 changes: 1 addition & 1 deletion src/cart/SConscript
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ def scons():

Import('env', 'prereqs', 'swim_targets', 'CART_VERSION')

daos_build.add_build_rpath(env)
env.d_add_build_rpath()

env.Alias('install', '$PREFIX')

Expand Down
4 changes: 1 addition & 3 deletions src/cart/utils/SConscript
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
#
"""Build crt_utils component"""

import daos_build

LIB_SRC = ['crt_utils.c']


Expand All @@ -25,7 +23,7 @@ def scons():

prereqs.require(env, 'protobufc')

daos_build.add_build_rpath(env)
env.d_add_build_rpath()

# Generate cart utility shared objects
build_utility_shared_obj(env)
Expand Down
2 changes: 1 addition & 1 deletion src/client/api/SConscript
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def scons():
"""Execute build"""
Import('env', 'API_VERSION', 'prereqs', 'libdaos_tgts')

daos_build.add_build_rpath(env)
env.d_add_build_rpath()
env.AppendUnique(LIBPATH=[Dir('.')])
denv = env.Clone()
prereqs.require(denv, 'protobufc')
Expand Down
2 changes: 1 addition & 1 deletion src/client/dfs/SConscript
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def scons():
"""Execute build"""
Import('env')

daos_build.add_build_rpath(env)
env.d_add_build_rpath()

denv = env.Clone()

Expand Down
3 changes: 1 addition & 2 deletions src/client/dfuse/SConscript
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
"""Build DFuse"""
import os
import daos_build
import compiler_setup

HEADERS = ['ioil_io.h', 'ioil_defines.h', 'ioil_api.h', 'ioil.h']
COMMON_SRC = ['dfuse_obj_da.c', 'dfuse_vector.c']
Expand Down Expand Up @@ -180,7 +179,7 @@ def scons():
il_env = base_env.Clone()
il_env['CC'] = 'gcc'
il_env['CXX'] = None
compiler_setup.base_setup(il_env)
il_env.compiler_setup()

# Set options which are used throughout the src.
dfuse_env = env.Clone(LIBS=[])
Expand Down
5 changes: 2 additions & 3 deletions src/client/pydaos/SConscript
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
"""Build pydaos client"""
import sys
import daos_build
import compiler_setup


def build_shim_module():
Expand All @@ -10,7 +9,7 @@ def build_shim_module():
if GetOption('help'):
return

version = "{}.{}".format(sys.version_info.major, sys.version_info.minor)
version = f'{sys.version_info.major}.{sys.version_info.minor}'

Import('base_env')

Expand All @@ -24,7 +23,7 @@ def build_shim_module():
new_env['CC'] = 'gcc'
new_env.AppendUnique(CCFLAGS='-pthread')

compiler_setup.base_setup(new_env)
new_env.compiler_setup()

obj = new_env.SharedObject('pydaos_shim', 'pydaos_shim.c',
SHLINKFLAGS=[],
Expand Down
4 changes: 2 additions & 2 deletions src/common/SConscript
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ def scons():

env.AppendUnique(LIBPATH=[Dir('.')])
base_env.AppendUnique(LIBPATH=[Dir('.')])
daos_build.add_build_rpath(base_env)
daos_build.add_build_rpath(env)
base_env.d_add_build_rpath()
env.d_add_build_rpath()

# Hack alert, the argobots headers are required but the shared
# library isn't so add the dependency so the include path
Expand Down
4 changes: 2 additions & 2 deletions src/common/tests/SConscript
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ def scons():
unit_env = tenv.Clone()
unit_env.AppendUnique(OBJPREFIX='utest_')

common_mock_ld_script = "%s/common-mock-ld-opts" % Dir('.').srcnode()
unit_env.AppendUnique(LINKFLAGS=['-Wl,@%s' % common_mock_ld_script])
common_mock_ld_script = f"{Dir('.').srcnode()}/common-mock-ld-opts"
unit_env.AppendUnique(LINKFLAGS=[f'-Wl,@{common_mock_ld_script}'])

mock_test_utils = unit_env.SharedObject(['test_mocks.c', 'test_utils.c'])
drpc_test_utils = unit_env.SharedObject(['../drpc.c']) + mock_test_utils
Expand Down
Loading