Skip to content

draft: fix: Overhaul quoting #1389

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

draft: fix: Overhaul quoting #1389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 6 additions & 6 deletions foreign_cc/built_tools/private/built_tools_framework.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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##")
Expand Down
8 changes: 4 additions & 4 deletions foreign_cc/cmake.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions foreign_cc/private/cmake_script.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion foreign_cc/private/configure_script.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
38 changes: 19 additions & 19 deletions foreign_cc/private/framework.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" + "\"",
Comment on lines +324 to +325
Copy link
Member

Choose a reason for hiding this comment

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

nit: these two lines don't need the string concatenation for the final quote

]

env = dict()
Expand Down Expand Up @@ -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)
Expand All @@ -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 (
Expand Down Expand Up @@ -704,9 +704,9 @@ def _get_transitive_artifacts(deps):

def _print_env():
return [
"##echo## \"Environment:______________\"",
"##echo## Environment:______________",
"##env##",
"##echo## \"__________________________\"",
"##echo## __________________________",
]

def _normalize_path(path):
Expand Down Expand Up @@ -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")

Expand All @@ -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

Expand Down Expand Up @@ -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)
12 changes: 6 additions & 6 deletions foreign_cc/private/framework/toolchains/windows_commands.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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,
)
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions foreign_cc/private/run_shell_file_utils.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
),
Expand Down
2 changes: 1 addition & 1 deletion test/symlink_contents_to_dir_test_rule.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
]
Expand Down
1 change: 0 additions & 1 deletion toolchains/prebuilt_toolchains.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,5 @@ def main():
)
)


if __name__ == "__main__":
main()