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.

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

Jump to bottom

WIP resolve_tools #849

Closed
wants to merge 1 commit into from
Closed

WIP resolve_tools #849

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
20 changes: 12 additions & 8 deletions foreign_cc/private/framework.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ def cc_external_rule_impl(ctx, attrs):
_print_deprecation_warnings(ctx)
lib_name = attrs.lib_name or ctx.attr.name

inputs = _define_inputs(attrs)
inputs = _define_inputs(ctx, attrs)
outputs = _define_outputs(ctx, attrs, lib_name)
out_cc_info = _define_out_cc_info(ctx, attrs, inputs, outputs)

Expand Down Expand Up @@ -458,16 +458,19 @@ def cc_external_rule_impl(ctx, attrs):
# TODO: `additional_tools` is deprecated, remove.
legacy_tools = ctx.files.additional_tools + ctx.files.tools_deps

inputs_from_tools, manifests_from_tools = ctx.resolve_tools(tools = attrs.tools_deps)
Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly should be legacy_tools here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also need to be aware of what this looks like once deprecated attributes are removed. build_data is not included in legacy_tools at the moment.


# The use of `run_shell` here is intended to ensure bash is correctly setup on windows
# environments. This should not be replaced with `run` until a cross platform implementation
# is found that guarantees bash exists or appropriately errors out.
ctx.actions.run_shell(
mnemonic = "Cc" + attrs.configure_name.capitalize() + "MakeRule",
inputs = depset(inputs.declared_inputs),
outputs = rule_outputs + [wrapped_outputs.log_file],
input_manifests = manifests_from_tools,
tools = depset(
[wrapped_outputs.script_file, wrapped_outputs.wrapper_script_file] + ctx.files.data + ctx.files.build_data + legacy_tools,
transitive = [cc_toolchain.all_files] + [data[DefaultInfo].default_runfiles.files for data in data_dependencies],
transitive = [cc_toolchain.all_files, inputs_from_tools] + [data[DefaultInfo].default_runfiles.files for data in data_dependencies],
),
command = wrapped_outputs.wrapper_script_file.path,
execution_requirements = execution_requirements,
Expand Down Expand Up @@ -650,9 +653,9 @@ def _copy_deps_and_tools(files):
lines += _symlink_contents_to_dir("lib", files.libs)
lines += _symlink_contents_to_dir("include", files.headers + files.include_dirs)

if files.tools_files:
if files.tools_roots:
lines.append("##mkdirs## $$EXT_BUILD_DEPS$$/bin")
for tool in files.tools_files:
for tool in files.tools_roots:
lines.append("##symlink_to_dir## $$EXT_BUILD_ROOT$$/{} $$EXT_BUILD_DEPS$$/bin/".format(tool))
Copy link
Member Author

Choose a reason for hiding this comment

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

Does this symlinking break finding runfiles for the tool? It doesn't break it for MAKE but we don't find make via the path so aren't using this mechanism in that case.


for ext_dir in files.ext_build_dirs:
Expand Down Expand Up @@ -763,7 +766,8 @@ InputFiles = provider(
"into $EXT_BUILD_DEPS/include."
),
libs = "Library files built by Bazel. Will be copied into $EXT_BUILD_DEPS/lib.",
tools_files = (
tools_files = "All files (including runfiles needed for the tools",
Copy link
Member Author

Choose a reason for hiding this comment

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

I think that we probably also want a tools_manifests entry here and we should merge this with any additional manifests when we call ctx.actions.run_shell. I've not fully groked what the manifests input does to an action as its not well documented.

tools_roots = (
"Files and directories with tools needed for configuration/building " +
"to be copied into the bin folder, which is added to the PATH"
),
Expand All @@ -777,7 +781,7 @@ InputFiles = provider(
),
)

def _define_inputs(attrs):
def _define_inputs(ctx, attrs):
cc_infos = []

bazel_headers = []
Expand Down Expand Up @@ -832,13 +836,13 @@ def _define_inputs(attrs):
headers = bazel_headers,
include_dirs = bazel_system_includes,
libs = bazel_libs,
tools_files = tools_roots,
tools_roots = tools_roots,
tools_files = tools_files,
deps_compilation_info = cc_info_merged.compilation_context,
deps_linking_info = cc_info_merged.linking_context,
ext_build_dirs = ext_build_dirs,
declared_inputs = filter_containing_dirs_from_inputs(attrs.lib_source.files.to_list()) +
bazel_libs +
tools_files +
input_files +
cc_info_merged.compilation_context.headers.to_list() +
ext_build_dirs,
Expand Down