-
Notifications
You must be signed in to change notification settings - Fork 241
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.
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
WIP resolve_tools #849
WIP resolve_tools #849
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
||
|
@@ -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) | ||
|
||
# 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, | ||
|
@@ -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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
tools_roots = ( | ||
"Files and directories with tools needed for configuration/building " + | ||
"to be copied into the bin folder, which is added to the PATH" | ||
), | ||
|
@@ -777,7 +781,7 @@ InputFiles = provider( | |
), | ||
) | ||
|
||
def _define_inputs(attrs): | ||
def _define_inputs(ctx, attrs): | ||
cc_infos = [] | ||
|
||
bazel_headers = [] | ||
|
@@ -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, | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.