Skip to content

Commit

Permalink
Add implementation of collect_compilation_prerequisites to cc_helper,…
Browse files Browse the repository at this point in the history
… this makes cc_internal's collect_compilation_prerequisites method obsolete.

Add test for cc_binary rule to test compilation prerequisites of OutputGroupInfo.

Expose API from CcCompilationContext necessary for Starlark transliteration.

Add builtin privacy tests for added methods.

CC_AND_OBJC constant's extensions now match native version.

Fix a bug where _check_src_extension would always return True if the file extension matched shared and versioned shared libraries(even if we did not want to include them), required a refactor of the code. This has not been noticed since the two consumers of the method(cc_binary & cc_library) allow shared and versioned shared libraries.

PiperOrigin-RevId: 434702440
  • Loading branch information
Googler authored and copybara-github committed Mar 15, 2022
1 parent f789c0d commit 8d22e46
Show file tree
Hide file tree
Showing 6 changed files with 99 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,13 @@ public NestedSet<Artifact> getTransitiveModules(boolean usePic) {
return usePic ? transitivePicModules : transitiveModules;
}

@Override
public Depset getStarlarkTransitiveModules(boolean usePic, StarlarkThread thread)
throws EvalException {
CcModule.checkPrivateStarlarkificationAllowlist(thread);
return Depset.of(Artifact.TYPE, getTransitiveModules(usePic));
}

/**
* Returns the immutable set of additional transitive inputs needed for compilation, like C++
* module map artifacts.
Expand All @@ -541,6 +548,12 @@ public NestedSet<Artifact> getAdditionalInputs() {
return builder.build();
}

@Override
public Depset getStarlarkAdditionalInputs(StarlarkThread thread) throws EvalException {
CcModule.checkPrivateStarlarkificationAllowlist(thread);
return Depset.of(Artifact.TYPE, getAdditionalInputs());
}

/** Adds additional transitive inputs needed for compilation to builder. */
void addAdditionalInputs(NestedSetBuilder<Artifact> builder) {
builder.addAll(directModuleMaps);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import com.google.devtools.build.docgen.annot.DocCategory;
import com.google.devtools.build.lib.collect.nestedset.Depset;
import com.google.devtools.build.lib.starlarkbuildapi.FileApi;
import net.starlark.java.annot.Param;
import net.starlark.java.annot.ParamType;
import net.starlark.java.annot.StarlarkBuiltin;
import net.starlark.java.annot.StarlarkMethod;
import net.starlark.java.eval.EvalException;
Expand Down Expand Up @@ -133,4 +135,20 @@ public interface CcCompilationContextApi<FileT extends FileApi> extends Starlark
doc = "Returns the set of validation artifacts.",
structField = true)
Depset getStarlarkValidationArtifacts();

@StarlarkMethod(name = "additional_inputs", documented = false, useStarlarkThread = true)
Depset getStarlarkAdditionalInputs(StarlarkThread thread) throws EvalException;

@StarlarkMethod(
name = "transitive_modules",
documented = false,
useStarlarkThread = true,
parameters = {
@Param(
name = "use_pic",
positional = false,
named = true,
allowedTypes = {@ParamType(type = Boolean.class)})
})
Depset getStarlarkTransitiveModules(boolean usePic, StarlarkThread thread) throws EvalException;
}
2 changes: 1 addition & 1 deletion src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ def cc_binary_impl(ctx, additional_linkopts):
Returns:
Appropriate providers for cc_binary/cc_test.
"""
cc_helper.check_srcs_extensions(ctx, ALLOWED_SRC_FILES, "cc_binary")
cc_helper.check_srcs_extensions(ctx, ALLOWED_SRC_FILES, "cc_binary", True)
common = cc_internal.create_common(ctx = ctx)
semantics.validate_deps(ctx)

Expand Down
64 changes: 43 additions & 21 deletions src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -41,23 +41,23 @@ artifact_category = struct(
CLIF_OUTPUT_PROTO = "CLIF_OUTPUT_PROTO",
)

def _check_src_extension(file, allowed_src_files):
def _check_src_extension(file, allowed_src_files, allow_versioned_shared_libraries):
extension = "." + file.extension
if _matches_extension(extension, allowed_src_files) or _is_shared_library_extension_valid(file.path):
if _matches_extension(extension, allowed_src_files) or (allow_versioned_shared_libraries and _is_versioned_shared_library_extension_valid(file.path)):
return True
return False

def _check_srcs_extensions(ctx, allowed_src_files, rule_name):
def _check_srcs_extensions(ctx, allowed_src_files, rule_name, allow_versioned_shared_libraries):
for src in ctx.attr.srcs:
if DefaultInfo in src:
files = src[DefaultInfo].files.to_list()
if len(files) == 1 and files[0].is_source:
if not _check_src_extension(files[0], allowed_src_files) and not files[0].is_directory:
if not _check_src_extension(files[0], allowed_src_files, allow_versioned_shared_libraries) and not files[0].is_directory:
fail("in srcs attribute of {} rule {}: source file '{}' is misplaced here".format(rule_name, ctx.label, str(src.label)))
else:
at_least_one_good = False
for file in files:
if _check_src_extension(file, allowed_src_files) or file.is_directory:
if _check_src_extension(file, allowed_src_files, allow_versioned_shared_libraries) or file.is_directory:
at_least_one_good = True
break
if not at_least_one_good:
Expand Down Expand Up @@ -134,6 +134,24 @@ def _find_cpp_toolchain(ctx):
# We didn't find anything.
fail("In order to use find_cpp_toolchain, you must define the '_cc_toolchain' attribute on your rule or aspect.")

def _collect_compilation_prerequisites(ctx, compilation_context):
direct = []
transitive = []
if hasattr(ctx.attr, "srcs"):
for src in ctx.attr.srcs:
if DefaultInfo in src:
files = src[DefaultInfo].files.to_list()
for file in files:
if _check_src_extension(file, extensions.CC_AND_OBJC, False):
direct.append(file)

transitive.append(compilation_context.headers)
transitive.append(compilation_context.additional_inputs())
transitive.append(compilation_context.transitive_modules(use_pic = True))
transitive.append(compilation_context.transitive_modules(use_pic = False))

return depset(direct = direct, transitive = transitive)

def _build_output_groups_for_emitting_compile_providers(
compilation_outputs,
compilation_context,
Expand All @@ -151,7 +169,7 @@ def _build_output_groups_for_emitting_compile_providers(
use_pic = use_pic,
)
output_groups_builder["compilation_outputs"] = files_to_compile
output_groups_builder["compilation_prerequisites_INTERNAL_"] = cc_internal.collect_compilation_prerequisites(ctx = ctx, compilation_context = compilation_context)
output_groups_builder["compilation_prerequisites_INTERNAL_"] = _collect_compilation_prerequisites(ctx = ctx, compilation_context = compilation_context)

if generate_hidden_top_level_group:
output_groups_builder["_hidden_top_level_INTERNAL_"] = _collect_library_hidden_top_level_artifacts(
Expand Down Expand Up @@ -219,14 +237,6 @@ OBJC_SOURCE = [".m"]
OBJCPP_SOURCE = [".mm"]
CLIF_INPUT_PROTO = [".ipb"]
CLIF_OUTPUT_PROTO = [".opb"]
CC_AND_OBJC = []
CC_AND_OBJC.extend(CC_SOURCE)
CC_AND_OBJC.extend(C_SOURCE)
CC_AND_OBJC.extend(OBJC_SOURCE)
CC_AND_OBJC.extend(OBJCPP_SOURCE)
CC_AND_OBJC.extend(CLIF_INPUT_PROTO)
CC_AND_OBJC.extend(CLIF_OUTPUT_PROTO)

CC_HEADER = [".h", ".hh", ".hpp", ".ipp", ".hxx", ".h++", ".inc", ".inl", ".tlh", ".tli", ".H", ".tcc"]
ASSESMBLER_WITH_C_PREPROCESSOR = [".S"]
ASSEMBLER = [".s", ".asm"]
Expand All @@ -238,6 +248,15 @@ SHARED_LIBRARY = [".so", ".dylib", ".dll"]
OBJECT_FILE = [".o", ".obj"]
PIC_OBJECT_FILE = [".pic.o"]

CC_AND_OBJC = []
CC_AND_OBJC.extend(CC_SOURCE)
CC_AND_OBJC.extend(C_SOURCE)
CC_AND_OBJC.extend(OBJC_SOURCE)
CC_AND_OBJC.extend(OBJCPP_SOURCE)
CC_AND_OBJC.extend(CC_HEADER)
CC_AND_OBJC.extend(ASSEMBLER)
CC_AND_OBJC.extend(ASSESMBLER_WITH_C_PREPROCESSOR)

extensions = struct(
CC_SOURCE = CC_SOURCE,
C_SOURCE = C_SOURCE,
Expand Down Expand Up @@ -392,12 +411,7 @@ def _build_precompiled_files(ctx):
shared_libraries,
)

def _is_shared_library_extension_valid(shared_library_name):
if (shared_library_name.endswith(".so") or
shared_library_name.endswith(".dll") or
shared_library_name.endswith(".dylib")):
return True

def _is_versioned_shared_library_extension_valid(shared_library_name):
# validate agains the regex "^.+\\.((so)|(dylib))(\\.\\d\\w*)+$",
# must match VERSIONED_SHARED_LIBRARY.
for ext in (".so.", ".dylib."):
Expand All @@ -411,9 +425,16 @@ def _is_shared_library_extension_valid(shared_library_name):
if not (c.isalnum() or c == "_"):
return False
return True

return False

def _is_shared_library_extension_valid(shared_library_name):
if (shared_library_name.endswith(".so") or
shared_library_name.endswith(".dll") or
shared_library_name.endswith(".dylib")):
return True

return _is_versioned_shared_library_extension_valid(shared_library_name)

def _get_providers(deps, provider):
providers = []
for dep in deps:
Expand Down Expand Up @@ -538,5 +559,6 @@ cc_helper = struct(
generate_def_file = _generate_def_file,
stringify_linker_input = _stringify_linker_input,
get_linked_artifact = _get_linked_artifact,
collect_compilation_prerequisites = _collect_compilation_prerequisites,
collect_native_cc_libraries = _collect_native_cc_libraries,
)
2 changes: 1 addition & 1 deletion src/main/starlark/builtins_bzl/common/cc/cc_library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ cc_common = _builtins.toplevel.cc_common
cc_internal = _builtins.internal.cc_internal

def _cc_library_impl(ctx):
cc_helper.check_srcs_extensions(ctx, ALLOWED_SRC_FILES, "cc_library")
cc_helper.check_srcs_extensions(ctx, ALLOWED_SRC_FILES, "cc_library", True)

common = cc_internal.create_common(ctx = ctx)
common.report_invalid_options(ctx = ctx)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7080,12 +7080,19 @@ public void testExpandedCompileApiBlocked() throws Exception {
public void testExpandedCcCompilationContextApiBlocked() throws Exception {
scratch.file(
"b/BUILD",
"load('//my_rules:rule.bzl', 'method_rule', 'param_2_rule')",
"load('//my_rules:rule.bzl', 'method_rule', 'param_2_rule', 'additional_inputs_rule',"
+ " 'transitive_modules_rule')",
"param_2_rule(",
" name = 'p2',",
")",
"method_rule(",
" name = 'm',",
")",
"additional_inputs_rule(",
" name = 'ai',",
")",
"transitive_modules_rule(",
" name = 'tm',",
")");
scratch.file("my_rules/BUILD");
scratch.file(
Expand All @@ -7097,16 +7104,30 @@ public void testExpandedCcCompilationContextApiBlocked() throws Exception {
"def _p2_impl(ctx):",
" comp_context = cc_common.create_compilation_context(purpose = 'testing')",
" return [CcInfo(compilation_context = comp_context)]",
"def _additional_inputs_impl(ctx):",
" comp_context = cc_common.create_compilation_context()",
" comp_context.additional_inputs()",
"def _transitive_modules_impl(ctx):",
" comp_context = cc_common.create_compilation_context()",
" comp_context.transitive_modules(use_pic = True)",
"method_rule = rule(",
" implementation = _m_impl,",
")",
"param_2_rule = rule(",
" implementation = _p2_impl,",
" implementation = _p2_impl)",
"additional_inputs_rule = rule(",
" implementation = _additional_inputs_impl)",
"transitive_modules_rule = rule(",
" implementation = _transitive_modules_impl",
")");
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:m"));
assertThat(e).hasMessageThat().contains("Rule in 'my_rules' cannot use private API");
e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:p2"));
assertThat(e).hasMessageThat().contains("Rule in 'my_rules' cannot use private API");
e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:ai"));
assertThat(e).hasMessageThat().contains("Rule in 'my_rules' cannot use private API");
e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//b:tm"));
assertThat(e).hasMessageThat().contains("Rule in 'my_rules' cannot use private API");
}

@Test
Expand Down

0 comments on commit 8d22e46

Please sign in to comment.