From 25f4494c484512898671aab57790f4917c0f9319 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 19 Aug 2015 16:02:37 +0000 Subject: RELNOTES: Allow private headers in the srcs attribute. Add a warning if a file is in both srcs and hdrs. This uses a very restrictive definition of private headers. They may only imported by sources in the same target and other private headers. They are not transitively available to dependers, even indirectly, though this may change in the future. -- MOS_MIGRATED_REVID=101028706 --- .../build/lib/rules/objc/CompilationArtifacts.java | 46 +++++++++++++++++++++- .../build/lib/rules/objc/CompilationSupport.java | 38 ++++++++++++++---- .../devtools/build/lib/rules/objc/ObjcCommon.java | 19 ++++----- .../build/lib/rules/objc/ObjcProtoLibrary.java | 4 +- .../build/lib/rules/objc/ObjcProvider.java | 7 +++- .../build/lib/rules/objc/ObjcRuleClasses.java | 33 ++++++++++++---- .../build/lib/rules/objc/XcodeProvider.java | 6 ++- 7 files changed, 122 insertions(+), 31 deletions(-) (limited to 'src/main/java/com') diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationArtifacts.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationArtifacts.java index 7d1377b5a5..5c8c5956af 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationArtifacts.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationArtifacts.java @@ -27,8 +27,11 @@ import com.google.devtools.build.lib.actions.Artifact; */ final class CompilationArtifacts { static class Builder { + // TODO(bazel-team): Should these be sets instead of just iterables? private Iterable srcs = ImmutableList.of(); private Iterable nonArcSrcs = ImmutableList.of(); + private Iterable additionalHdrs = ImmutableList.of(); + private Iterable privateHdrs = ImmutableList.of(); private Optional pchFile; private IntermediateArtifacts intermediateArtifacts; @@ -42,6 +45,25 @@ final class CompilationArtifacts { return this; } + /** + * Adds header artifacts that should be directly accessible to dependers, but aren't specified + * in the hdrs attribute. {@code additionalHdrs} should not be a {@link NestedSet}, as it will + * be flattened when added. + */ + Builder addAdditionalHdrs(Iterable additionalHdrs) { + this.additionalHdrs = Iterables.concat(this.additionalHdrs, additionalHdrs); + return this; + } + + /** + * Adds header artifacts that should not be directly accessible to dependers. + * {@code privateHdrs} should not be a {@link NestedSet}, as it will be flattened when added. + */ + Builder addPrivateHdrs(Iterable privateHdrs) { + this.privateHdrs = Iterables.concat(this.privateHdrs, privateHdrs); + return this; + } + Builder setPchFile(Optional pchFile) { Preconditions.checkState(this.pchFile == null, "pchFile is already set to: %s", this.pchFile); @@ -61,23 +83,30 @@ final class CompilationArtifacts { if (!Iterables.isEmpty(srcs) || !Iterables.isEmpty(nonArcSrcs)) { archive = Optional.of(intermediateArtifacts.archive()); } - return new CompilationArtifacts(srcs, nonArcSrcs, archive, pchFile); + return new CompilationArtifacts( + srcs, nonArcSrcs, additionalHdrs, privateHdrs, archive, pchFile); } } private final Iterable srcs; private final Iterable nonArcSrcs; private final Optional archive; + private final Iterable additionalHdrs; + private final Iterable privateHdrs; private final Optional pchFile; private final boolean hasSwiftSources; private CompilationArtifacts( Iterable srcs, Iterable nonArcSrcs, + Iterable additionalHdrs, + Iterable privateHdrs, Optional archive, Optional pchFile) { this.srcs = Preconditions.checkNotNull(srcs); this.nonArcSrcs = Preconditions.checkNotNull(nonArcSrcs); + this.additionalHdrs = Preconditions.checkNotNull(additionalHdrs); + this.privateHdrs = Preconditions.checkNotNull(privateHdrs); this.archive = Preconditions.checkNotNull(archive); this.pchFile = Preconditions.checkNotNull(pchFile); this.hasSwiftSources = Iterables.any(this.srcs, new Predicate() { @@ -96,6 +125,21 @@ final class CompilationArtifacts { return nonArcSrcs; } + /** + * Returns the public headers that aren't included in the hdrs attribute. + */ + public Iterable getAdditionalHdrs() { + return additionalHdrs; + } + + /** + * Returns the private headers from the srcs attribute, which may by imported by any source or + * header in this target, but not by sources or headers of dependers. + */ + public Iterable getPrivateHdrs() { + return privateHdrs; + } + public Optional getArchive() { return archive; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java index c76f35baa2..b26a702fa5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java @@ -31,7 +31,9 @@ import static com.google.devtools.build.lib.rules.objc.ObjcProvider.SDK_FRAMEWOR import static com.google.devtools.build.lib.rules.objc.ObjcProvider.WEAK_SDK_FRAMEWORK; import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.CLANG; import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.CLANG_PLUSPLUS; +import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.COMPILABLE_SRCS_TYPE; import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.DSYMUTIL; +import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.HEADERS; import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.NON_ARC_SRCS_TYPE; import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.SRCS_TYPE; import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.STRIP; @@ -44,8 +46,10 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.analysis.PrerequisiteArtifacts; import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext; @@ -55,6 +59,7 @@ import com.google.devtools.build.lib.analysis.actions.FileWriteAction; import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.packages.TargetUtils; +import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.rules.cpp.LinkerInputs; import com.google.devtools.build.lib.rules.java.J2ObjcConfiguration; import com.google.devtools.build.lib.rules.objc.ObjcCommon.CompilationAttributes; @@ -63,7 +68,9 @@ import com.google.devtools.build.lib.shell.ShellUtils; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; /** * Support for rules that compile sources. Provides ways to determine files that should be output, @@ -72,7 +79,7 @@ import java.util.List; * *

Methods on this class can be called in any order without impacting the result. */ -final class CompilationSupport { +public final class CompilationSupport { @VisibleForTesting static final String ABSOLUTE_INCLUDES_PATH_FORMAT = @@ -98,18 +105,23 @@ final class CompilationSupport { } } + @VisibleForTesting + static final String FILE_IN_SRCS_AND_HDRS_WARNING_FORMAT = + "File '%s' is in both srcs and hdrs."; + /** * Returns information about the given rule's compilation artifacts. */ // TODO(bazel-team): Remove this information from ObjcCommon and move it internal to this class. static CompilationArtifacts compilationArtifacts(RuleContext ruleContext) { + PrerequisiteArtifacts srcs = ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET) + .errorsForNonMatching(SRCS_TYPE); return new CompilationArtifacts.Builder() - .addSrcs(ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET) - .errorsForNonMatching(SRCS_TYPE) - .list()) + .addSrcs(srcs.filter(COMPILABLE_SRCS_TYPE).list()) .addNonArcSrcs(ruleContext.getPrerequisiteArtifacts("non_arc_srcs", Mode.TARGET) .errorsForNonMatching(NON_ARC_SRCS_TYPE) .list()) + .addPrivateHdrs(srcs.filter(HEADERS).list()) .setIntermediateArtifacts(ObjcRuleClasses.intermediateArtifacts(ruleContext)) .setPchFile(Optional.fromNullable(ruleContext.getPrerequisiteArtifact("pch", Mode.TARGET))) .build(); @@ -121,7 +133,7 @@ final class CompilationSupport { /** * Creates a new compilation support for the given rule. */ - CompilationSupport(RuleContext ruleContext) { + public CompilationSupport(RuleContext ruleContext) { this.ruleContext = ruleContext; this.attributes = new CompilationAttributes(ruleContext); } @@ -216,8 +228,7 @@ final class CompilationSupport { commandLine .add(IosSdkCommands.compileFlagsForClang(objcConfiguration)) - .add(IosSdkCommands.commonLinkAndCompileFlagsForClang( - objcProvider, objcConfiguration)) + .add(IosSdkCommands.commonLinkAndCompileFlagsForClang(objcProvider, objcConfiguration)) .add(objcConfiguration.getCoptsForCompilationMode()) .addBeforeEachPath( "-iquote", ObjcCommon.userHeaderSearchPaths(ruleContext.getConfiguration())) @@ -232,6 +243,7 @@ final class CompilationSupport { .addExecPath("-c", sourceFile) .addExecPath("-o", objFile); + // TODO(bazel-team): Remote private headers from inputs once they're added to the provider. ruleContext.registerAction(ObjcRuleClasses.spawnOnDarwinActionBuilder() .setMnemonic("ObjcCompile") .setExecutable(CLANG) @@ -241,6 +253,7 @@ final class CompilationSupport { .addOutput(objFile) .addOutputs(gcnoFiles.build()) .addTransitiveInputs(objcProvider.get(HEADER)) + .addInputs(compilationArtifacts.getPrivateHdrs()) .addTransitiveInputs(objcProvider.get(FRAMEWORK_FILE)) .addInputs(compilationArtifacts.getPchFile().asSet()) .build(ruleContext)); @@ -732,6 +745,17 @@ final class CompilationSupport { "includes", String.format(ABSOLUTE_INCLUDES_PATH_FORMAT, absoluteInclude)); } + // Check for overlap between srcs and hdrs. + if (ruleContext.attributes().has("srcs", Type.LABEL_LIST)) { + Set hdrsSet = new HashSet<>(attributes.hdrs()); + Set srcsSet = + new HashSet<>(ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET).list()); + for (Artifact header : Sets.intersection(hdrsSet, srcsSet)) { + String path = header.getRootRelativePath().toString(); + ruleContext.attributeWarning( + "srcs", String.format(FILE_IN_SRCS_AND_HDRS_WARNING_FORMAT, path)); + } + } return this; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java index 030164eaf4..3a2ce46dd0 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java @@ -93,6 +93,10 @@ public final class ObjcCommon { } ImmutableList hdrs() { + // Some rules may compile but not have the "hdrs" attribute. + if (!ruleContext.attributes().has("hdrs", Type.LABEL_LIST)) { + return ImmutableList.of(); + } return ImmutableList.copyOf(CcCommon.getHeaders(ruleContext)); } @@ -241,7 +245,6 @@ public final class ObjcCommon { private Iterable directDepObjcProviders = ImmutableList.of(); private Iterable defines = ImmutableList.of(); private Iterable userHeaderSearchPaths = ImmutableList.of(); - private Iterable headers = ImmutableList.of(); private IntermediateArtifacts intermediateArtifacts; private boolean alwayslink; private Iterable extraImportLibraries = ImmutableList.of(); @@ -326,11 +329,6 @@ public final class ObjcCommon { return this; } - public Builder addHeaders(Iterable headers) { - this.headers = Iterables.concat(this.headers, headers); - return this; - } - Builder setIntermediateArtifacts(IntermediateArtifacts intermediateArtifacts) { this.intermediateArtifacts = intermediateArtifacts; return this; @@ -397,7 +395,6 @@ public final class ObjcCommon { .addAll(FRAMEWORK_DIR, uniqueContainers(frameworkImports, FRAMEWORK_CONTAINER_TYPE)) .addAll(INCLUDE, userHeaderSearchPaths) .addAll(DEFINE, defines) - .addAll(HEADER, headers) .addTransitiveAndPropagate(depObjcProviders) .addTransitiveWithoutPropagating(directDepObjcProviders); @@ -453,8 +450,12 @@ public final class ObjcCommon { for (CompilationArtifacts artifacts : compilationArtifacts.asSet()) { Iterable allSources = Iterables.concat(artifacts.getSrcs(), artifacts.getNonArcSrcs()); - objcProvider.addAll(LIBRARY, artifacts.getArchive().asSet()); - objcProvider.addAll(SOURCE, allSources); + // TODO(bazel-team): Add private headers to the provider when we have module maps to enforce + // them. + objcProvider + .addAll(HEADER, artifacts.getAdditionalHdrs()) + .addAll(LIBRARY, artifacts.getArchive().asSet()) + .addAll(SOURCE, allSources); BuildConfiguration configuration = context.getConfiguration(); RegexFilter filter = configuration.getInstrumentationFilter(); if (configuration.isCodeCoverageEnabled() diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibrary.java index 2510fcb80b..ce1709a5ab 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibrary.java @@ -153,6 +153,8 @@ public class ObjcProtoLibrary implements RuleConfiguredTargetFactory { .addNonArcSrcs(protoGeneratedSources) .setIntermediateArtifacts(intermediateArtifacts) .setPchFile(Optional.absent()) + .addAdditionalHdrs(protoGeneratedHeaders) + .addAdditionalHdrs(protoGeneratedSources) .build(); ImmutableSet.Builder searchPathEntriesBuilder = @@ -175,8 +177,6 @@ public class ObjcProtoLibrary implements RuleConfiguredTargetFactory { .addDepObjcProviders(ruleContext.getPrerequisites( ObjcProtoLibraryRule.LIBPROTOBUF_ATTR, Mode.TARGET, ObjcProvider.class)) .setIntermediateArtifacts(intermediateArtifacts) - .addHeaders(protoGeneratedHeaders) - .addHeaders(protoGeneratedSources) .build(); NestedSetBuilder filesToBuild = NestedSetBuilder.stableOrder() diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java index 413ed2d5b2..24f33e479c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java @@ -76,6 +76,9 @@ public final class ObjcProvider implements TransitiveInfoProvider { */ public static final Key FORCE_LOAD_FOR_XCODEGEN = new Key<>(LINK_ORDER); + /** + * Contains all header files. These may be either public or private headers. + */ public static final Key HEADER = new Key<>(STABLE_ORDER); /** @@ -90,7 +93,7 @@ public final class ObjcProvider implements TransitiveInfoProvider { /** * Contains all .gcno files one for every source file if in coverage mode. - * It contains information to reconstruct the basic block graphs and assign source line numbers + * It contains information to reconstruct the basic block graphs and assign source line numbers * to blocks. */ public static final Key GCNO = new Key<>(STABLE_ORDER); @@ -334,7 +337,7 @@ public final class ObjcProvider implements TransitiveInfoProvider { /** * Add elements from providers, but don't propagate them to any dependers on this ObjcProvider. * These elements will be exposed to {@link #get(Key)} calls, but not to any ObjcProviders - * which add this provider to themself. + * which add this provider to themselves. */ public Builder addTransitiveWithoutPropagating(Iterable providers) { for (ObjcProvider provider : providers) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java index ad32d2e36b..ec3f27950b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java @@ -317,10 +317,24 @@ public class ObjcRuleClasses { private static final FileType NON_CPP_SOURCES = FileType.of(".m", ".c", ".s", ".asm"); static final FileType PREPROCESSED_ASSEMBLY_SOURCES = FileType.of(".S"); - + static final FileType SWIFT_SOURCES = FileType.of(".swift"); + /** + * Header files, which are not compiled directly, but may be included/imported from source files. + */ + static final FileType HEADERS = FileType.of(".h", ".inc"); + + /** + * Files allowed in the srcs attribute. This includes private headers. + */ static final FileTypeSet SRCS_TYPE = FileTypeSet.of(NON_CPP_SOURCES, CPP_SOURCES, + PREPROCESSED_ASSEMBLY_SOURCES, SWIFT_SOURCES, HEADERS); + + /** + * Files that should actually be compiled. + */ + static final FileTypeSet COMPILABLE_SRCS_TYPE = FileTypeSet.of(NON_CPP_SOURCES, CPP_SOURCES, PREPROCESSED_ASSEMBLY_SOURCES, SWIFT_SOURCES); static final FileTypeSet NON_ARC_SRCS_TYPE = FileTypeSet.of(FileType.of(".m", ".mm")); @@ -514,8 +528,9 @@ public class ObjcRuleClasses { public RuleClass build(Builder builder, RuleDefinitionEnvironment env) { return builder /* - The list of Objective-C files that are included as headers by source - files in this rule or by users of this library. + The list of C, C++, Objective-C, and Objective-C++ files that are + included as headers by source files in this rule or by users of this + library. ${SYNOPSIS} */ .add(attr("hdrs", LABEL_LIST) @@ -579,12 +594,14 @@ public class ObjcRuleClasses { public RuleClass build(Builder builder, RuleDefinitionEnvironment env) { return builder /* - The list of C, C++, Objective-C, and Objective-C++ files that are - processed to create the library target. + The list of C, C++, Objective-C, and Objective-C++ source and header + files that are processed to create the library target. ${SYNOPSIS} - These are your checked-in source files, plus any generated files. - These are compiled into .o files with Clang, so headers should not go - here (see the hdrs attribute). + These are your checked-in files, plus any generated files. + Source files are compiled into .o files with Clang. Header files + may be included/imported by any source or header in the srcs attribute + of this target, but not by headers in hdrs or any targets that depend + on this rule. */ .add(attr("srcs", LABEL_LIST) .direct_compile_time_input() diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/XcodeProvider.java b/src/main/java/com/google/devtools/build/lib/rules/objc/XcodeProvider.java index 96a124c290..3e77023af2 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/XcodeProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/XcodeProvider.java @@ -526,6 +526,7 @@ public final class XcodeProvider implements TransitiveInfoProvider { .setName(label.getName()) .setLabel(xcodeTargetName(label)) .setProductType(productType.getIdentifier()) + .addSupportFile(buildFilePath) .addAllImportedLibrary(Artifact.toExecPaths(objcProvider.get(IMPORTED_LIBRARY))) .addAllUserHeaderSearchPath(userHeaderSearchPaths) .addAllHeaderSearchPath(headerSearchPaths) @@ -552,8 +553,7 @@ public final class XcodeProvider implements TransitiveInfoProvider { .addAllGeneralResourceFile( Artifact.toExecPaths(objcProvider.get(GENERAL_RESOURCE_FILE))) .addAllGeneralResourceFile( - PathFragment.safePathStrings(objcProvider.get(GENERAL_RESOURCE_DIR))) - .addSupportFile(buildFilePath); + PathFragment.safePathStrings(objcProvider.get(GENERAL_RESOURCE_DIR))); if (CAN_LINK_PRODUCT_TYPES.contains(productType)) { // For builds with --ios_multi_cpus set, we may have several copies of some XCodeProviders @@ -605,6 +605,8 @@ public final class XcodeProvider implements TransitiveInfoProvider { for (CompilationArtifacts artifacts : compilationArtifacts.asSet()) { targetControl .addAllSourceFile(Artifact.toExecPaths(artifacts.getSrcs())) + .addAllSupportFile(Artifact.toExecPaths(artifacts.getAdditionalHdrs())) + .addAllSupportFile(Artifact.toExecPaths(artifacts.getPrivateHdrs())) .addAllNonArcSourceFile(Artifact.toExecPaths(artifacts.getNonArcSrcs())); for (Artifact pchFile : artifacts.getPchFile().asSet()) { -- cgit v1.2.3