From 1646468880f479faf25edf891f09674d9e616325 Mon Sep 17 00:00:00 2001 From: Benjamin Mordaunt Date: Mon, 31 Mar 2025 17:43:05 +0100 Subject: [PATCH] fix: Improve quoting for mkdirs and rmrf This is necessary to fix handling of directories with spaces on Windows platform. --- .../private/built_tools_framework.bzl | 12 +++--- foreign_cc/cmake.bzl | 8 ++-- foreign_cc/private/cmake_script.bzl | 4 +- foreign_cc/private/configure_script.bzl | 2 +- foreign_cc/private/framework.bzl | 38 +++++++++---------- .../framework/toolchains/windows_commands.bzl | 12 +++--- foreign_cc/private/run_shell_file_utils.bzl | 4 +- test/symlink_contents_to_dir_test_rule.bzl | 2 +- toolchains/prebuilt_toolchains.py | 1 - 9 files changed, 41 insertions(+), 42 deletions(-) diff --git a/foreign_cc/built_tools/private/built_tools_framework.bzl b/foreign_cc/built_tools/private/built_tools_framework.bzl index 51c33597f..d9e102449 100644 --- a/foreign_cc/built_tools/private/built_tools_framework.bzl +++ b/foreign_cc/built_tools/private/built_tools_framework.bzl @@ -119,12 +119,12 @@ def built_tool_rule_impl(ctx, script_lines, out_dir, mnemonic, additional_tools script = [ "##script_prelude##", ] + env_prelude + [ - "##rm_rf## $$INSTALLDIR$$", - "##rm_rf## $$BUILD_TMPDIR$$", - "##mkdirs## $$INSTALLDIR$$", - "##mkdirs## $$BUILD_TMPDIR$$", - "##copy_dir_contents_to_dir## ./{} $$BUILD_TMPDIR$$".format(root), - "cd $$BUILD_TMPDIR$$", + "##rm_rf## \"$$INSTALLDIR$$\"", + "##rm_rf## \"$$BUILD_TMPDIR$$\"", + "##mkdirs## \"$$INSTALLDIR$$\"", + "##mkdirs## \"$$BUILD_TMPDIR$$\"", + "##copy_dir_contents_to_dir## ./{} \"$$BUILD_TMPDIR$$\"".format(root), + "cd \"$$BUILD_TMPDIR$$\"", ] script.append("##enable_tracing##") diff --git a/foreign_cc/cmake.bzl b/foreign_cc/cmake.bzl index 99cc17d86..7104d1a6c 100644 --- a/foreign_cc/cmake.bzl +++ b/foreign_cc/cmake.bzl @@ -171,11 +171,11 @@ def _cmake_impl(ctx): if "Unix Makefiles" == generator: make_data = get_make_data(ctx) tools_data.append(make_data) - generate_args.append("-DCMAKE_MAKE_PROGRAM={}".format(make_data.path)) + generate_args.append("-DCMAKE_MAKE_PROGRAM=\"{}\"".format(make_data.path)) elif "Ninja" in generator: ninja_data = get_ninja_data(ctx) tools_data.append(ninja_data) - generate_args.append("-DCMAKE_MAKE_PROGRAM={}".format(ninja_data.path)) + generate_args.append("-DCMAKE_MAKE_PROGRAM=\"{}\"".format(ninja_data.path)) attrs = create_attrs( ctx.attr, @@ -230,7 +230,7 @@ def _create_configure_script(configureParameters): # Note that even though directory is always passed, the # following arguments can take precedence. - cmake_commands.append("{cmake} --build {dir} --config {config} {target} {args}".format( + cmake_commands.append("\"{cmake}\" --build \"{dir}\" --config {config} {target} {args}".format( cmake = attrs.cmake_path, dir = ".", args = build_args, @@ -245,7 +245,7 @@ def _create_configure_script(configureParameters): for arg in ctx.attr.install_args ]) - cmake_commands.append("{cmake} --install {dir} --config {config} {args}".format( + cmake_commands.append("\"{cmake}\" --install \"{dir}\" --config {config} {args}".format( cmake = attrs.cmake_path, dir = ".", args = install_args, diff --git a/foreign_cc/private/cmake_script.bzl b/foreign_cc/private/cmake_script.bzl index 593445171..2090f7516 100644 --- a/foreign_cc/private/cmake_script.bzl +++ b/foreign_cc/private/cmake_script.bzl @@ -161,14 +161,14 @@ def create_cmake_script( script += pkgconfig_script(ext_build_dirs) - directory = "$$EXT_BUILD_ROOT$$/" + root + directory = "\"$$EXT_BUILD_ROOT$$/" + root + "\"" script.append("##enable_tracing##") # Configure the CMake generate command cmake_prefixes = [cmake_prefix] if cmake_prefix else [] script.append(" ".join(cmake_prefixes + [ - cmake_path, + "\"{}\"".format(cmake_path), str_cmake_cache_entries, " ".join(options), # Generator is always set last and will override anything specified by the user diff --git a/foreign_cc/private/configure_script.bzl b/foreign_cc/private/configure_script.bzl index bbb50569f..d8d825dd7 100644 --- a/foreign_cc/private/configure_script.bzl +++ b/foreign_cc/private/configure_script.bzl @@ -73,7 +73,7 @@ def create_configure_script( options = " ".join(autoreconf_options), ).lstrip()) - script.append("##mkdirs## $$BUILD_TMPDIR$$/$$INSTALL_PREFIX$$") + script.append("##mkdirs## \"$$BUILD_TMPDIR$$/$$INSTALL_PREFIX$$\"") make_commands = [] script.append("{env_vars} {prefix}\"{configure}\" {prefix_flag}$$BUILD_TMPDIR$$/$$INSTALL_PREFIX$$ {user_options}".format( diff --git a/foreign_cc/private/framework.bzl b/foreign_cc/private/framework.bzl index adfb67b94..bae16ba0b 100644 --- a/foreign_cc/private/framework.bzl +++ b/foreign_cc/private/framework.bzl @@ -320,9 +320,9 @@ def get_env_prelude(ctx, installdir, data_dependencies): """ env_snippet = [ "export EXT_BUILD_ROOT=##pwd##", - "export INSTALLDIR=$$EXT_BUILD_ROOT$$/" + installdir, - "export BUILD_TMPDIR=$$INSTALLDIR$$.build_tmpdir", - "export EXT_BUILD_DEPS=$$INSTALLDIR$$.ext_build_deps", + "export INSTALLDIR=\"$$EXT_BUILD_ROOT$$/" + installdir + "\"", + "export BUILD_TMPDIR=\"$$INSTALLDIR$$.build_tmpdir" + "\"", + "export EXT_BUILD_DEPS=\"$$INSTALLDIR$$.ext_build_deps" + "\"", ] env = dict() @@ -472,14 +472,14 @@ def cc_external_rule_impl(ctx, attrs): "##echo## \"\"", "##script_prelude##", ] + env_prelude + [ - "##path## $$EXT_BUILD_ROOT$$", - "##rm_rf## $$BUILD_TMPDIR$$", - "##rm_rf## $$EXT_BUILD_DEPS$$", - "##mkdirs## $$INSTALLDIR$$", - "##mkdirs## $$BUILD_TMPDIR$$", - "##mkdirs## $$EXT_BUILD_DEPS$$", + "##path## \"$$EXT_BUILD_ROOT$$\"", + "##rm_rf## \"$$BUILD_TMPDIR$$\"", + "##rm_rf## \"$$EXT_BUILD_DEPS$$\"", + "##mkdirs## \"$$INSTALLDIR$$\"", + "##mkdirs## \"$$BUILD_TMPDIR$$\"", + "##mkdirs## \"$$EXT_BUILD_DEPS$$\"", ] + _print_env() + _copy_deps_and_tools(inputs) + [ - "cd $$BUILD_TMPDIR$$", + "cd \"$$BUILD_TMPDIR$$\"", ] + attrs.create_configure_script(ConfigureParameters(ctx = ctx, attrs = attrs, inputs = inputs)) + postfix_script + [ # replace references to the root directory when building ($BUILD_TMPDIR) # and the root where the dependencies were installed ($EXT_BUILD_DEPS) @@ -488,7 +488,7 @@ def cc_external_rule_impl(ctx, attrs): "##replace_absolute_paths## $$INSTALLDIR$$ $$EXT_BUILD_DEPS$$", "##replace_sandbox_paths## $$INSTALLDIR$$ $$EXT_BUILD_ROOT$$", installdir_copy.script, - "cd $$EXT_BUILD_ROOT$$", + "cd \"$$EXT_BUILD_ROOT$$\"", ] + [ "##replace_symlink## {}".format(file.path) for file in ( @@ -704,9 +704,9 @@ def _get_transitive_artifacts(deps): def _print_env(): return [ - "##echo## \"Environment:______________\"", + "##echo## Environment:______________", "##env##", - "##echo## \"__________________________\"", + "##echo## __________________________", ] def _normalize_path(path): @@ -767,14 +767,14 @@ def _copy_deps_and_tools(files): lines += _symlink_contents_to_dir("include", files.headers + files.include_dirs) if files.tools_files: - lines.append("##mkdirs## $$EXT_BUILD_DEPS$$/bin") + lines.append("##mkdirs## \"$$EXT_BUILD_DEPS$$/bin\"") for tool in files.tools_files: tool_prefix = "$EXT_BUILD_ROOT/" tool = tool[len(tool_prefix):] if tool.startswith(tool_prefix) else tool - lines.append("##symlink_to_dir## $$EXT_BUILD_ROOT$$/{} $$EXT_BUILD_DEPS$$/bin/ False".format(tool)) + lines.append("##symlink_to_dir## \"$$EXT_BUILD_ROOT$$/{}\" \"$$EXT_BUILD_DEPS$$/bin/\" False".format(tool)) for ext_dir in files.ext_build_dirs: - lines.append("##symlink_to_dir## $$EXT_BUILD_ROOT$$/{} $$EXT_BUILD_DEPS$$ True".format(_file_path(ext_dir))) + lines.append("##symlink_to_dir## \"$$EXT_BUILD_ROOT$$/{}\" \"$$EXT_BUILD_DEPS$$\" True".format(_file_path(ext_dir))) lines.append("##path## $$EXT_BUILD_DEPS$$/bin") @@ -786,13 +786,13 @@ def _symlink_contents_to_dir(dir_name, files_list): files_list = collections.uniq(files_list) if len(files_list) == 0: return [] - lines = ["##mkdirs## $$EXT_BUILD_DEPS$$/" + dir_name] + lines = ["##mkdirs## \"$$EXT_BUILD_DEPS$$/" + dir_name + "\""] for file in files_list: path = _file_path(file).strip() if path: lines.append("##symlink_contents_to_dir## \ -$$EXT_BUILD_ROOT$$/{} $$EXT_BUILD_DEPS$$/{} True".format(path, dir_name)) +\"$$EXT_BUILD_ROOT$$/{}\" \"$$EXT_BUILD_DEPS$$/{}\" True".format(path, dir_name)) return lines @@ -1121,4 +1121,4 @@ def _expand_locations_in_string(ctx, expandable, data): if "EXT_BUILD_ROOT" in expandable: return ctx.expand_location(expandable, data) else: - return ctx.expand_location(expandable.replace("$(execpath ", "$$EXT_BUILD_ROOT$$/$(execpath "), data) + return ctx.expand_location(expandable.replace("$(execpath ", "\"$$EXT_BUILD_ROOT$$\"/$(execpath "), data) diff --git a/foreign_cc/private/framework/toolchains/windows_commands.bzl b/foreign_cc/private/framework/toolchains/windows_commands.bzl index f74cd944a..40928648e 100644 --- a/foreign_cc/private/framework/toolchains/windows_commands.bzl +++ b/foreign_cc/private/framework/toolchains/windows_commands.bzl @@ -9,7 +9,7 @@ def script_extension(): return ".sh" def pwd(): - return "$(type -t cygpath > /dev/null && cygpath $(pwd) -m || pwd -W)" + return "\"$(type -t cygpath > /dev/null && cygpath \"$(pwd)\" -m || pwd -W)\"" def echo(text): return "echo \"{text}\"".format(text = text) @@ -108,7 +108,7 @@ fi ) def copy_dir_contents_to_dir(source, target): - return """cp -L -r --no-target-directory "{source}" "{target}" && $REAL_FIND "{target}" -type f -exec touch -r "{source}" "{{}}" \\;""".format( + return """cp -L -r --no-target-directory {source} {target} && $REAL_FIND {target} -type f -exec touch -r {source} "{{}}" \\;""".format( source = source, target = target, ) @@ -253,28 +253,28 @@ fi""".format(dir_ = dir_) return FunctionAndCallInfo(text = text) def define_absolute_paths(dir_, abs_path): - return "##replace_in_files## {dir_} {REPLACE_VALUE} {abs_path}".format( + return "##replace_in_files## \"{dir_}\" \"{REPLACE_VALUE}\" \"{abs_path}\"".format( dir_ = dir_, REPLACE_VALUE = "\\${EXT_BUILD_DEPS}", abs_path = abs_path, ) def replace_absolute_paths(dir_, abs_path): - return "##replace_in_files## {dir_} {abs_path} {REPLACE_VALUE}".format( + return "##replace_in_files## \"{dir_}\" \"{abs_path}\" \"{REPLACE_VALUE}\"".format( dir_ = dir_, REPLACE_VALUE = "\\${EXT_BUILD_DEPS}", abs_path = abs_path, ) def define_sandbox_paths(dir_, abs_path): - return "##replace_in_files## {dir_} {REPLACE_VALUE} {abs_path}".format( + return "##replace_in_files## \"{dir_}\" \"{REPLACE_VALUE}\" \"{abs_path}\"".format( dir_ = dir_, REPLACE_VALUE = "\\${EXT_BUILD_ROOT}", abs_path = abs_path, ) def replace_sandbox_paths(dir_, abs_path): - return "##replace_in_files## {dir_} {abs_path} {REPLACE_VALUE}".format( + return "##replace_in_files## \"{dir_}\" \"{abs_path}\" \"{REPLACE_VALUE}\"".format( dir_ = dir_, REPLACE_VALUE = "\\${EXT_BUILD_ROOT}", abs_path = abs_path, diff --git a/foreign_cc/private/run_shell_file_utils.bzl b/foreign_cc/private/run_shell_file_utils.bzl index 89b5e3367..79d66b093 100644 --- a/foreign_cc/private/run_shell_file_utils.bzl +++ b/foreign_cc/private/run_shell_file_utils.bzl @@ -46,8 +46,8 @@ def copy_directory(actions, orig_path, copy_path): return _created_by_script( file = dir_copy, script = "\n".join([ - "##mkdirs## $$EXT_BUILD_ROOT$$/" + dir_copy.path, - "##copy_dir_contents_to_dir## {} $$EXT_BUILD_ROOT$$/{}".format( + "##mkdirs## \"$$EXT_BUILD_ROOT$$/" + dir_copy.path + "\"", + "##copy_dir_contents_to_dir## \"{}\" \"$$EXT_BUILD_ROOT$$/{}\"".format( orig_path, dir_copy.path, ), diff --git a/test/symlink_contents_to_dir_test_rule.bzl b/test/symlink_contents_to_dir_test_rule.bzl index 896bbd94b..03dd6e172 100644 --- a/test/symlink_contents_to_dir_test_rule.bzl +++ b/test/symlink_contents_to_dir_test_rule.bzl @@ -11,7 +11,7 @@ def _symlink_contents_to_dir_test_rule_impl(ctx): dir2 = detect_root(ctx.attr.dir2) script_lines = [ "##mkdirs## aaa", - "##symlink_contents_to_dir## %s aaa False" % dir1, + "##symlink_contents_to_dir## \"%s\" aaa False" % dir1, "##symlink_contents_to_dir## %s aaa False" % dir2, "ls -R aaa > %s" % out.path, ] diff --git a/toolchains/prebuilt_toolchains.py b/toolchains/prebuilt_toolchains.py index 6161b7ba9..8f7f98ac7 100755 --- a/toolchains/prebuilt_toolchains.py +++ b/toolchains/prebuilt_toolchains.py @@ -505,6 +505,5 @@ def main(): ) ) - if __name__ == "__main__": main()