Skip to content

Commit

Permalink
Add toolchain executables to the PATH
Browse files Browse the repository at this point in the history
Previously, some toolchain executables, e.g. make, pkg-config etc. were
not added to the PATH correctly. As such, even if make and pkg-config
were built from source, the system executables were used.

This commit modifies the tools_files attribute of the rules_foreign_cc
framework to instead be "tools_data", which provides path to the
executable and the target that provides it.
  • Loading branch information
jheaff1 committed Nov 29, 2022
1 parent 2c6262f commit 0a243b4
Show file tree
Hide file tree
Showing 7 changed files with 27 additions and 26 deletions.
2 changes: 1 addition & 1 deletion foreign_cc/built_tools/pkgconfig_build.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def _pkgconfig_tool_impl(ctx):
"%s install" % make_data.path,
]

additional_tools = depset(transitive = [dep.files for dep in make_data.deps])
additional_tools = depset(transitive = [make_data.target.files])

return built_tool_rule_impl(
ctx,
Expand Down
11 changes: 4 additions & 7 deletions foreign_cc/cmake.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -159,21 +159,18 @@ load(
def _cmake_impl(ctx):
cmake_data = get_cmake_data(ctx)

tools_deps = cmake_data.deps

# TODO: `tool_deps` is deprecated. Remove
tools_deps += ctx.attr.tools_deps
tools_data = [cmake_data]

env = dict(ctx.attr.env)

generator, generate_args = _get_generator_target(ctx)
if "Unix Makefiles" == generator:
make_data = get_make_data(ctx)
tools_deps.extend(make_data.deps)
tools_data.append(make_data)
generate_args.append("-DCMAKE_MAKE_PROGRAM={}".format(make_data.path))
elif "Ninja" in generator:
ninja_data = get_ninja_data(ctx)
tools_deps.extend(ninja_data.deps)
tools_data.append(ninja_data)
generate_args.append("-DCMAKE_MAKE_PROGRAM={}".format(ninja_data.path))

attrs = create_attrs(
Expand All @@ -183,7 +180,7 @@ def _cmake_impl(ctx):
generate_args = generate_args,
configure_name = "CMake",
create_configure_script = _create_configure_script,
tools_deps = tools_deps,
tools_data = tools_data,
cmake_path = cmake_data.path,
)

Expand Down
4 changes: 2 additions & 2 deletions foreign_cc/configure.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def _configure_make(ctx):
make_data = get_make_data(ctx)
pkg_config_data = get_pkgconfig_data(ctx)

tools_deps = ctx.attr.tools_deps + make_data.deps + pkg_config_data.deps
tools_data = [make_data, pkg_config_data]

if ctx.attr.autogen and not ctx.attr.configure_in_place:
fail("`autogen` requires `configure_in_place = True`. Please update {}".format(
Expand All @@ -50,7 +50,7 @@ def _configure_make(ctx):
configure_name = "Configure",
create_configure_script = _create_configure_script,
postfix_script = copy_results + "\n" + ctx.attr.postfix_script,
tools_deps = tools_deps,
tools_data = tools_data,
make_path = make_data.path,
)
return cc_external_rule_impl(ctx, attrs)
Expand Down
4 changes: 2 additions & 2 deletions foreign_cc/make.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@ load("//toolchains/native_tools:tool_access.bzl", "get_make_data")
def _make(ctx):
make_data = get_make_data(ctx)

tools_deps = ctx.attr.tools_deps + make_data.deps
tools_data = [make_data]

attrs = create_attrs(
ctx.attr,
configure_name = "Make",
create_configure_script = _create_make_script,
tools_deps = tools_deps,
tools_data = tools_data,
make_path = make_data.path,
)
return cc_external_rule_impl(ctx, attrs)
Expand Down
4 changes: 2 additions & 2 deletions foreign_cc/ninja.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ def _ninja_impl(ctx):
"""
ninja_data = get_ninja_data(ctx)

tools_deps = ctx.attr.tools_deps + ninja_data.deps
tools_data = [ninja_data]

attrs = create_attrs(
ctx.attr,
configure_name = "Ninja",
create_configure_script = _create_ninja_script,
tools_deps = tools_deps,
tools_data = tools_data,
ninja_path = ninja_data.path,
)
return cc_external_rule_impl(ctx, attrs)
Expand Down
24 changes: 14 additions & 10 deletions foreign_cc/private/framework.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ load("@bazel_skylib//lib:collections.bzl", "collections")
load("@bazel_skylib//lib:paths.bzl", "paths")
load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain")
load("//foreign_cc:providers.bzl", "ForeignCcArtifactInfo", "ForeignCcDepsInfo")
load("//foreign_cc/private:detect_root.bzl", "detect_root", "filter_containing_dirs_from_inputs")
load("//foreign_cc/private:detect_root.bzl", "filter_containing_dirs_from_inputs")
load(
"//foreign_cc/private/framework:helpers.bzl",
"convert_shell_script",
Expand Down Expand Up @@ -409,7 +409,10 @@ def cc_external_rule_impl(ctx, attrs):
installdir_copy = copy_directory(ctx.actions, "$$INSTALLDIR$$", "copy_{}/{}".format(lib_name, lib_name))
target_root = paths.dirname(installdir_copy.file.dirname)

data_dependencies = ctx.attr.data + ctx.attr.build_data + ctx.attr.toolchains + attrs.tools_deps
data_dependencies = ctx.attr.data + ctx.attr.build_data + ctx.attr.toolchains
for tool in attrs.tools_data:
if tool.target:
data_dependencies.append(tool.target)

# Also add legacy dependencies while they're still available
data_dependencies += ctx.attr.tools_deps + ctx.attr.additional_tools
Expand Down Expand Up @@ -683,12 +686,13 @@ def _copy_deps_and_tools(files):
if files.tools_files:
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))

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("##children_to_path## $$EXT_BUILD_DEPS$$/bin")
lines.append("##path## $$EXT_BUILD_DEPS$$/bin")

return lines
Expand Down Expand Up @@ -836,14 +840,14 @@ def _define_inputs(attrs):
# but filter out repeating directories
ext_build_dirs = uniq_list_keep_order(ext_build_dirs)

tools_roots = []
tools = []
tools_files = []
input_files = []
for tool in attrs.tools_deps:
tool_root = detect_root(tool)
tools_roots.append(tool_root)
for file_list in tool.files.to_list():
tools_files += _list(file_list)
for tool in attrs.tools_data:
tools.append(tool.path)
if tool.target:
for file_list in tool.target.files.to_list():
tools_files += _list(file_list)

# TODO: Remove, `additional_tools` is deprecated.
for tool in attrs.additional_tools:
Expand All @@ -862,7 +866,7 @@ def _define_inputs(attrs):
headers = bazel_headers,
include_dirs = bazel_system_includes,
libs = bazel_libs,
tools_files = tools_roots,
tools_files = tools,
deps_compilation_info = cc_info_merged.compilation_context,
deps_linking_info = cc_info_merged.linking_context,
ext_build_dirs = ext_build_dirs,
Expand Down
4 changes: 2 additions & 2 deletions toolchains/native_tools/tool_access.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,14 @@ def _access_and_expect_label_copied(toolchain_type_, ctx):
cmd_file = f
break
return struct(
deps = [tool_data.target],
target = tool_data.target,
env = tool_data.env,
# as the tool will be copied into tools directory
path = "$EXT_BUILD_ROOT/{}".format(cmd_file.path),
)
else:
return struct(
deps = [],
target = None,
env = tool_data.env,
path = tool_data.path,
)

0 comments on commit 0a243b4

Please sign in to comment.