From 846218a8bcfd560f5d26de886eaade49f83ce4dc Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Mon, 23 Oct 2023 11:13:19 +0800 Subject: [PATCH 01/11] build: move variable closer to where it is used for better readability. Signed-off-by: Kefu Chai --- configure.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/configure.py b/configure.py index 268b3619ec..023c4b8ec4 100755 --- a/configure.py +++ b/configure.py @@ -19,8 +19,6 @@ import tempfile import textwrap from distutils.spawn import find_executable -curdir = os.getcwd() - outdir = 'build' tempfile.tempdir = f"{outdir}/tmp" @@ -1670,6 +1668,7 @@ forced_ldflags += dynamic_linker_option() user_ldflags = forced_ldflags + ' ' + args.user_ldflags +curdir = os.getcwd() user_cflags = args.user_cflags + f" -ffile-prefix-map={curdir}=." if args.target != '': From ea6bf6b908fe688406cc7cbbc03d61b9e059e214 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Mon, 23 Oct 2023 11:17:22 +0800 Subject: [PATCH 02/11] build: remove unused variable `optional_packages` was introduced in 8b0a26f06d, but we don't offer the alternative versions of libsystemd anymore, and this variable is not used in `configure.py`, so let's drop it. Signed-off-by: Kefu Chai --- configure.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/configure.py b/configure.py index 023c4b8ec4..d73ba00915 100755 --- a/configure.py +++ b/configure.py @@ -1568,10 +1568,6 @@ perf_tests_link_rule = 'link' if args.perf_tests_debuginfo else 'link_stripped' # debug info from the libraries we static link with regular_link_rule = 'link' if args.debuginfo else 'link_stripped' -# a list element means a list of alternative packages to consider -# the first element becomes the HAVE_pkg define -# a string element is a package name with no alternatives -optional_packages = [[]] pkgs = [] # Lua can be provided by lua53 package on Debian-like From 8b76f2a835b27ecbbfd0fc86baf05db477b931ba Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Mon, 23 Oct 2023 11:21:37 +0800 Subject: [PATCH 03/11] build: move pkg closer to where it is used for better readability. Signed-off-by: Kefu Chai --- configure.py | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/configure.py b/configure.py index d73ba00915..78456607e1 100755 --- a/configure.py +++ b/configure.py @@ -1568,15 +1568,6 @@ perf_tests_link_rule = 'link' if args.perf_tests_debuginfo else 'link_stripped' # debug info from the libraries we static link with regular_link_rule = 'link' if args.debuginfo else 'link_stripped' -pkgs = [] - -# Lua can be provided by lua53 package on Debian-like -# systems and by Lua on others. -pkgs.append('lua53' if have_pkg('lua53') else 'lua') - -pkgs.append('libsystemd') -pkgs.append('jsoncpp') - has_sanitize_address_use_after_scope = try_compile(compiler=args.cxx, flags=['-fsanitize-address-use-after-scope'], source='int f() {}') defines = ' '.join(['-D' + d for d in defines]) @@ -1757,13 +1748,14 @@ def query_seastar_flags(pc_file, use_shared_libs, link_static_cxx=False): 'seastar_libs': libs, 'seastar_testing_libs': testing_libs} +pkgs = ['libsystemd', + 'jsoncpp', + 'absl_raw_hash_set', + 'absl_hash'] +# Lua can be provided by lua53 package on Debian-like +# systems and by Lua on others. +pkgs.append('lua53' if have_pkg('lua53') else 'lua') -abseil_pkgs = [ - 'absl_raw_hash_set', - 'absl_hash', -] - -pkgs += abseil_pkgs libs = ' '.join([maybe_static(args.staticyamlcpp, '-lyaml-cpp'), '-latomic', '-llz4', '-lz', '-lsnappy', ' -lstdc++fs', ' -lcrypt', ' -lcryptopp', ' -lpthread', From 8646e6c5d1f89718b5a6e6c9486f161a0876417c Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Mon, 23 Oct 2023 11:26:46 +0800 Subject: [PATCH 04/11] build: move thrift_libs to where it is used for better readability. Signed-off-by: Kefu Chai --- configure.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/configure.py b/configure.py index 78456607e1..8f09da4cd1 100755 --- a/configure.py +++ b/configure.py @@ -1780,10 +1780,6 @@ user_cflags += ' -fvisibility=hidden' user_ldflags += ' -fvisibility=hidden' if args.staticcxx: user_ldflags += " -static-libstdc++" -if args.staticthrift: - thrift_libs = "-Wl,-Bstatic -lthrift -Wl,-Bdynamic" -else: - thrift_libs = "-lthrift" os.makedirs(outdir, exist_ok=True) @@ -2004,7 +2000,8 @@ def write_build_file(f, objs.append('$builddir/' + mode +'/rust-' + mode + '/librust_combined.a') local_libs = '$seastar_libs_{} $libs'.format(mode) if has_thrift: - local_libs += ' ' + thrift_libs + ' ' + maybe_static(args.staticboost, '-lboost_system') + local_libs += ' ' + maybe_static(args.staticthrift, '-lthrift') + local_libs += ' ' + maybe_static(args.staticboost, '-lboost_system') if binary in tests: if binary in pure_boost_tests: local_libs += ' ' + maybe_static(args.staticboost, '-lboost_unit_test_framework') From ec7ac3c750dd4dd796184603a6c0ed9c56688df0 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Thu, 26 Oct 2023 10:34:58 +0800 Subject: [PATCH 05/11] build: extract get_extra_cxxflags() out on top of per-mode cxxflags, we apply more of them based on settings and building environment. to reduce the statements in global scope, let's extract the related code into a function. Refs #15379 Signed-off-by: Kefu Chai --- configure.py | 62 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/configure.py b/configure.py index 8f09da4cd1..65fd66e109 100755 --- a/configure.py +++ b/configure.py @@ -1539,28 +1539,8 @@ for mode_level in args.mode_o_levels: raise Exception(f'Mode {mode} is missing, cannot configure optimization level for it') modes[mode]['optimization-level'] = level -for mode in modes: - modes[mode]['cxxflags'] += f' -O{modes[mode]["optimization-level"]}' - -optimization_flags = [ - '--param inline-unit-growth=300', # gcc - f'-mllvm -inline-threshold={get_clang_inline_threshold()}', # clang - # clang generates 16-byte loads that break store-to-load forwarding - # gcc also has some trouble: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103554 - '-fno-slp-vectorize', -] -optimization_flags = [o - for o in optimization_flags - if flag_supported(flag=o, compiler=args.cxx)] -modes['release']['cxxflags'] += ' ' + ' '.join(optimization_flags) - -if flag_supported(flag='-Wstack-usage=4096', compiler=args.cxx): - for mode in modes: - modes[mode]['cxxflags'] += f' -Wstack-usage={modes[mode]["stack-usage-threshold"]} -Wno-error=stack-usage=' - linker_flags = linker_flags(compiler=args.cxx) -dbgflag = '-g -gz' if args.debuginfo else '' tests_link_rule = 'link' if args.tests_debuginfo else 'link_stripped' perf_tests_link_rule = 'link' if args.perf_tests_debuginfo else 'link_stripped' @@ -1607,12 +1587,9 @@ scylla_release = file.read().strip() file = open(f'{outdir}/SCYLLA-PRODUCT-FILE', 'r') scylla_product = file.read().strip() -for m, mode_config in modes.items(): - mode_config['cxxflags'] += f" -DSCYLLA_BUILD_MODE={m}" +for mode_config in modes.values(): cxxflags = "-DSCYLLA_VERSION=\"\\\"" + scylla_version + "\\\"\" -DSCYLLA_RELEASE=\"\\\"" + scylla_release + "\\\"\"" mode_config["per_src_extra_cxxflags"]["release.cc"] = cxxflags - if mode_config["can_have_debug_info"]: - mode_config['cxxflags'] += ' ' + dbgflag # The relocatable package includes its own dynamic linker. We don't # know the path it will be installed to, so for now use a very long @@ -1783,6 +1760,39 @@ if args.staticcxx: os.makedirs(outdir, exist_ok=True) + +def get_extra_cxxflags(mode, mode_config, cxx, debuginfo): + cxxflags = [] + + optimization_level = mode_config['optimization-level'] + cxxflags.append(f'-O{optimization_level}') + + if mode == 'release': + optimization_flags = [ + '--param inline-unit-growth=300', # gcc + f'-mllvm -inline-threshold={get_clang_inline_threshold()}', # clang + # clang generates 16-byte loads that break store-to-load forwarding + # gcc also has some trouble: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103554 + '-fno-slp-vectorize', + ] + optimization_flags = [o + for o in optimization_flags + if flag_supported(flag=o, compiler=cxx)] + cxxflags += optimization_flags + + if flag_supported(flag='-Wstack-usage=4096', compiler=cxx): + stack_usage_threshold = mode_config['stack-usage-threshold'] + cxxflags += [f'-Wstack-usage={stack_usage_threshold}', + '-Wno-error=stack-usage='] + + cxxflags.append(f'-DSCYLLA_BUILD_MODE={mode}') + + if debuginfo and mode_config['can_have_debug_info']: + cxxflags += ['-g', '-gz'] + + return cxxflags + + def write_build_file(f, arch, ninja, @@ -1888,10 +1898,14 @@ def write_build_file(f, for mode in build_modes: modeval = modes[mode] + modeval.update(query_seastar_flags(f'{outdir}/{mode}/seastar/seastar.pc', modeval['build_seastar_shared_libs'], args.staticcxx)) + extra_cxxflags = ' '.join(get_extra_cxxflags(mode, modeval, args.cxx, args.debuginfo)) + modeval['cxxflags'] += f' {extra_cxxflags}' + fmt_lib = 'fmt' f.write(textwrap.dedent('''\ cxx_ld_flags_{mode} = {cxx_ld_flags} From cb6531b1a8554d158d72663627ed05b311eb21f2 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Thu, 26 Oct 2023 10:59:40 +0800 Subject: [PATCH 06/11] build: extract get_release_cxxflags() out prepare for the change to read the SCYLLA-*-FILE in functions not doing this in global scope. Refs #15379 Signed-off-by: Kefu Chai --- configure.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/configure.py b/configure.py index 65fd66e109..63ed8c8f23 100755 --- a/configure.py +++ b/configure.py @@ -1587,10 +1587,6 @@ scylla_release = file.read().strip() file = open(f'{outdir}/SCYLLA-PRODUCT-FILE', 'r') scylla_product = file.read().strip() -for mode_config in modes.values(): - cxxflags = "-DSCYLLA_VERSION=\"\\\"" + scylla_version + "\\\"\" -DSCYLLA_RELEASE=\"\\\"" + scylla_release + "\\\"\"" - mode_config["per_src_extra_cxxflags"]["release.cc"] = cxxflags - # The relocatable package includes its own dynamic linker. We don't # know the path it will be installed to, so for now use a very long # path so that patchelf doesn't need to edit the program headers. The @@ -1793,6 +1789,13 @@ def get_extra_cxxflags(mode, mode_config, cxx, debuginfo): return cxxflags +def get_release_cxxflags(scylla_version, + scylla_release): + definitions = {'SCYLLA_VERSION': scylla_version, + 'SCYLLA_RELEASE': scylla_release} + return [f'-D{name}="\\"{value}\\""' for name, value in definitions.items()] + + def write_build_file(f, arch, ninja, @@ -1906,6 +1909,9 @@ def write_build_file(f, extra_cxxflags = ' '.join(get_extra_cxxflags(mode, modeval, args.cxx, args.debuginfo)) modeval['cxxflags'] += f' {extra_cxxflags}' + modeval['per_src_extra_cxxflags']['release.cc'] = ' '.join(get_release_cxxflags(scylla_version, + scylla_release)) + fmt_lib = 'fmt' f.write(textwrap.dedent('''\ cxx_ld_flags_{mode} = {cxx_ld_flags} From a25a153e9fa88aa5cefdefc3ed9b51a71335e590 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Thu, 26 Oct 2023 11:09:42 +0800 Subject: [PATCH 07/11] build: extract generate_version() out so we don't do less things with side effects in the global scope. Refs #15379 Signed-off-by: Kefu Chai --- configure.py | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/configure.py b/configure.py index 63ed8c8f23..6655acc7e2 100755 --- a/configure.py +++ b/configure.py @@ -1574,18 +1574,24 @@ if args.artifacts: else: build_artifacts = all_artifacts -date_stamp = f"--date-stamp {args.date_stamp}" if args.date_stamp else "" -status = subprocess.call(f"./SCYLLA-VERSION-GEN {date_stamp}", shell=True) -if status != 0: - print('Version file generation failed') - sys.exit(1) -file = open(f'{outdir}/SCYLLA-VERSION-FILE', 'r') -scylla_version = file.read().strip().replace('-', '~') -file = open(f'{outdir}/SCYLLA-RELEASE-FILE', 'r') -scylla_release = file.read().strip() -file = open(f'{outdir}/SCYLLA-PRODUCT-FILE', 'r') -scylla_product = file.read().strip() +def generate_version(date_stamp): + date_stamp_opt = '' + if date_stamp: + date_stamp_opt = f'--date-stamp {date_stamp}' + status = subprocess.call(f"./SCYLLA-VERSION-GEN {date_stamp_opt}", shell=True) + if status != 0: + print('Version file generation failed') + sys.exit(1) + + with open(f'{outdir}/SCYLLA-VERSION-FILE', 'r') as f: + scylla_version = f.read().strip().replace('-', '~') + with open(f'{outdir}/SCYLLA-RELEASE-FILE', 'r') as f: + scylla_release = f.read().strip() + with open(f'{outdir}/SCYLLA-PRODUCT-FILE', 'r') as f: + scylla_product = f.read().strip() + return scylla_product, scylla_version, scylla_release + # The relocatable package includes its own dynamic linker. We don't # know the path it will be installed to, so for now use a very long @@ -2377,6 +2383,7 @@ check_for_boost(args.cxx) check_for_lz4(args.cxx, args.user_cflags) ninja = find_ninja() with open(buildfile, 'w') as f: + scylla_product, scylla_version, scylla_release = generate_version(args.date_stamp) arch = platform.machine() write_build_file(f, arch, From a375ce2ac1b00d68f35deb939e29ad5ee9c361bb Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Thu, 26 Oct 2023 11:11:26 +0800 Subject: [PATCH 08/11] build: do not rely on updating global with a dict we use `globals().update(vars(args))` for updating the global variables with a dict in `args`, this is convenient, but it hurts the readability. let's reference the parsed options explicitly. Signed-off-by: Kefu Chai --- configure.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure.py b/configure.py index 6655acc7e2..923d9dcfee 100755 --- a/configure.py +++ b/configure.py @@ -2382,7 +2382,7 @@ check_for_minimal_compiler_version(args.cxx) check_for_boost(args.cxx) check_for_lz4(args.cxx, args.user_cflags) ninja = find_ninja() -with open(buildfile, 'w') as f: +with open(args.buildfile, 'w') as f: scylla_product, scylla_version, scylla_release = generate_version(args.date_stamp) arch = platform.machine() write_build_file(f, @@ -2392,4 +2392,4 @@ with open(buildfile, 'w') as f: scylla_version, scylla_release, args) -generate_compdb('compile_commands.json', ninja, buildfile, selected_modes) +generate_compdb('compile_commands.json', ninja, args.buildfile, selected_modes) From 6c7cc927b59058b7c37a06bcb160609561c454ba Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Thu, 26 Oct 2023 11:29:07 +0800 Subject: [PATCH 09/11] build: group the code with side effects together so we can move them into a single function Refs #15379 Signed-off-by: Kefu Chai --- configure.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/configure.py b/configure.py index 923d9dcfee..c766657844 100755 --- a/configure.py +++ b/configure.py @@ -1705,9 +1705,6 @@ def configure_seastar(build_dir, mode, mode_config): os.makedirs(seastar_build_dir, exist_ok=True) subprocess.check_call(seastar_cmd, shell=False, cwd=cmake_dir) -if not args.dist_only: - for mode, mode_config in build_modes.items(): - configure_seastar(outdir, mode, mode_config) def query_seastar_flags(pc_file, use_shared_libs, link_static_cxx=False): if use_shared_libs: @@ -2381,6 +2378,14 @@ def write_build_file(f, check_for_minimal_compiler_version(args.cxx) check_for_boost(args.cxx) check_for_lz4(args.cxx, args.user_cflags) + +if not args.dist_only: + # args.buildfile builds seastar with the rules of + # {outdir}/{mode}/seastar/build.ninja, and + # {outdir}/{mode}/seastar/seastar.pc is queried for building flags + for mode, mode_config in build_modes.items(): + configure_seastar(outdir, mode, mode_config) + ninja = find_ninja() with open(args.buildfile, 'w') as f: scylla_product, scylla_version, scylla_release = generate_version(args.date_stamp) From 85cc9073c9ad43267a1ee5a846dfa81e62f37b6e Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Thu, 26 Oct 2023 11:35:05 +0800 Subject: [PATCH 10/11] build: create outdir when outdir is explictly used actually we've created outdir when using it as the parent directory of `tempfile.tempdir`, but there are many places where we use `tempfile.tempdir` for, for instance, testing the compiler flags, and these tests will be removed once we migrate to CMake, so they do not really matter when reviewing the change which migrates to CMake. the point of this change is to help the review understand the major changes performed by the migration. Refs #15379 Signed-off-by: Kefu Chai --- configure.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure.py b/configure.py index c766657844..5f503e882e 100755 --- a/configure.py +++ b/configure.py @@ -1757,8 +1757,6 @@ user_ldflags += ' -fvisibility=hidden' if args.staticcxx: user_ldflags += " -static-libstdc++" -os.makedirs(outdir, exist_ok=True) - def get_extra_cxxflags(mode, mode_config, cxx, debuginfo): cxxflags = [] @@ -2379,6 +2377,8 @@ check_for_minimal_compiler_version(args.cxx) check_for_boost(args.cxx) check_for_lz4(args.cxx, args.user_cflags) +os.makedirs(outdir, exist_ok=True) + if not args.dist_only: # args.buildfile builds seastar with the rules of # {outdir}/{mode}/seastar/build.ninja, and From bfd99fad7ffc67e38fe938bd359d9735ce6ccbdd Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Thu, 26 Oct 2023 11:43:25 +0800 Subject: [PATCH 11/11] build: move the code with side effects into a single function so that we can optionally utilize CMake for generating the building system instead. Refs #15379 Signed-off-by: Kefu Chai --- configure.py | 49 +++++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/configure.py b/configure.py index 5f503e882e..aeefe4dbb2 100755 --- a/configure.py +++ b/configure.py @@ -2373,28 +2373,33 @@ def write_build_file(f, ''').format(**globals())) -check_for_minimal_compiler_version(args.cxx) -check_for_boost(args.cxx) -check_for_lz4(args.cxx, args.user_cflags) +def create_building_system(args): + check_for_minimal_compiler_version(args.cxx) + check_for_boost(args.cxx) + check_for_lz4(args.cxx, args.user_cflags) -os.makedirs(outdir, exist_ok=True) + os.makedirs(outdir, exist_ok=True) -if not args.dist_only: - # args.buildfile builds seastar with the rules of - # {outdir}/{mode}/seastar/build.ninja, and - # {outdir}/{mode}/seastar/seastar.pc is queried for building flags - for mode, mode_config in build_modes.items(): - configure_seastar(outdir, mode, mode_config) + if not args.dist_only: + # args.buildfile builds seastar with the rules of + # {outdir}/{mode}/seastar/build.ninja, and + # {outdir}/{mode}/seastar/seastar.pc is queried for building flags + for mode, mode_config in build_modes.items(): + configure_seastar(outdir, mode, mode_config) -ninja = find_ninja() -with open(args.buildfile, 'w') as f: - scylla_product, scylla_version, scylla_release = generate_version(args.date_stamp) - arch = platform.machine() - write_build_file(f, - arch, - ninja, - scylla_product, - scylla_version, - scylla_release, - args) -generate_compdb('compile_commands.json', ninja, args.buildfile, selected_modes) + ninja = find_ninja() + with open(args.buildfile, 'w') as f: + scylla_product, scylla_version, scylla_release = generate_version(args.date_stamp) + arch = platform.machine() + write_build_file(f, + arch, + ninja, + scylla_product, + scylla_version, + scylla_release, + args) + generate_compdb('compile_commands.json', ninja, args.buildfile, selected_modes) + + +if __name__ == '__main__': + create_building_system(args)