From 24946adcf50e1eb9f7b903adb60cb1b7f1fec796 Mon Sep 17 00:00:00 2001 From: Jerry Marino Date: Fri, 8 Sep 2017 18:42:35 -0700 Subject: [PATCH 1/3] Implicit HeaderMaps Request for feedback on an implementation of C++ HeaderMaps in Bazel. A HeaderMap is a data structure that allows the compiler to lookup included headers in constant time. Traditionally, the compiler has to parse a large string of `iquote` includes, and then search these directories for a given header. This is slow for many reasons. The protocol of HeaderMap is implemented within compilers. Please find the Lexer implementation in Clang. https://clang.llvm.org/doxygen/HeaderMapTypes_8h.html https://clang.llvm.org/doxygen/HeaderMap_8cpp_source.html Use case: I'm seeing a massive increase in build performance by using this. It cut my clean build time in half. Performance data: Build time before HeaderMap: Target //Pinterest/iOS/App:PinterestDevelopment up-to-date: bazel-bin/Pinterest/iOS/App/PinterestDevelopment.ipa ____Elapsed time: 373.588s, Critical Path: 18.86s Build time after header maps on the entire project: Target //Pinterest/iOS/App:PinterestDevelopment up-to-date: bazel-bin/Pinterest/iOS/App/PinterestDevelopment.ipa ____Elapsed time: 188.971s, Critical Path: 17.11s Additionally, this solves the problem of having namespaced headers which is used in CocoaPods all over. Using a namespace makes includes more clear since it is easier for the user to distinguish where the header was derived. Implementation: At the ObjC level, headermaps are created with a namespace of the given target. In `objc_library` it is possible for the user to override the value of the namespace via the new attribute, `header_namespace`. By using 2 headermaps the headersearchs are most efficient: a headermap for the current target, and a header map with namespaced includes. Users can include headers from ObjC targets in the convention of `Namespace/Header.h`. Projects that don't use namespacing should see benefits as well: includes of the form `Header.h` will be read from the headermap. `HeaderMapInfo` contains all of the transitive info for dependent header maps, and is merged together into a single map. This yields much better performance than multiple headermaps. This is my first PR to the Bazel repo, so any suggestions or feedback is greatly appreciated! --- .../lib/rules/cpp/CcCompilationHelper.java | 71 ++++ .../lib/rules/cpp/CcCompilationInfo.java | 26 +- .../build/lib/rules/cpp/ClangHeaderMap.java | 325 ++++++++++++++++++ .../build/lib/rules/cpp/CppConfiguration.java | 7 + .../build/lib/rules/cpp/CppOptions.java | 12 + .../build/lib/rules/cpp/HeaderMapAction.java | 87 +++++ .../build/lib/rules/cpp/HeaderMapInfo.java | 86 +++++ .../lib/rules/cpp/HeaderMapInfoProvider.java | 33 ++ .../build/lib/rules/objc/ObjcCommon.java | 66 +++- .../build/lib/rules/objc/ObjcLibrary.java | 2 + .../build/lib/rules/objc/ObjcRuleClasses.java | 8 + .../lib/rules/cpp/ClangHeaderMapTest.java | 41 +++ .../build/lib/rules/objc/ObjcLibraryTest.java | 35 ++ 13 files changed, 793 insertions(+), 6 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMap.java create mode 100644 src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapAction.java create mode 100644 src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfo.java create mode 100644 src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfoProvider.java create mode 100644 src/test/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMapTest.java diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java index d2b4145e745c72..a3454e466098bf 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java @@ -71,6 +71,10 @@ import java.util.TreeMap; import javax.annotation.Nullable; + +import com.google.common.collect.Sets; +import com.google.common.collect.Streams; + /** * A class to create C/C++ compile actions in a way that is consistent with cc_library. Rules that * generate source files and emulate cc_library on top of that should use this class instead of the @@ -1035,6 +1039,73 @@ public CcCompilationInfo initializeCcCompilationInfo() { } } + if (ruleContext.getFragment(CppConfiguration.class).experimentalEnableImplicitHeaderMaps()) { + ImmutableList.Builder headerMapsBuilder = ImmutableList.builder(); + String targetName = ruleContext.getTarget().getName(); + + HeaderMapInfo.Builder internalHeaderMapInfo = new HeaderMapInfo.Builder(); + internalHeaderMapInfo.addHeaders(publicHeaders.getHeaders()); + internalHeaderMapInfo.addHeaders(privateHeaders); + internalHeaderMapInfo.addHeaders(publicTextualHeaders); + Artifact internalHeaderMap = ruleContext.getPackageRelativeArtifact(PathFragment.create(targetName + "_internal.hmap"), + ruleContext + .getConfiguration() + .getGenfilesDirectory(ruleContext.getRule().getRepository())); + ruleContext.registerAction( + new HeaderMapAction(ruleContext.getActionOwner(), + internalHeaderMapInfo.build().getSources(), + internalHeaderMap)); + ccCompilationInfoBuilder.addQuoteIncludeDir(internalHeaderMap.getExecPath()); + headerMapsBuilder.add(internalHeaderMap); + + String namespace; + if (ruleContext.attributes().has("header_namespace")) { + namespace = ruleContext.attributes().get("header_namespace", Type.STRING); + } else { + namespace = ruleContext.getRule().getName(); + } + + // Construct the dep headermap. + // This header map additionally contains namespaced headers so that a user + // can import headers of the form Namespace/Header.h from headers within + // the current target. + HeaderMapInfo.Builder depHeaderMapInfo = new HeaderMapInfo.Builder(); + depHeaderMapInfo.setNamespace(namespace); + depHeaderMapInfo.addNamespacedHeaders(publicHeaders.getHeaders()); + depHeaderMapInfo.addHeaders(publicHeaders.getHeaders()); + depHeaderMapInfo.addNamespacedHeaders(privateHeaders); + depHeaderMapInfo.addHeaders(privateHeaders); + depHeaderMapInfo.addNamespacedHeaders(publicTextualHeaders); + depHeaderMapInfo.addHeaders(publicTextualHeaders); + + // Merge all of the header map info from deps. The headers within a given + // target have precedence over over dep headers ( See + // HeaderMapInfo.build() ). + if (ruleContext.attributes().has("deps")){ + for (HeaderMapInfoProvider hmapProvider : ruleContext.getPrerequisites("deps", Mode.TARGET, HeaderMapInfoProvider.class)) { + depHeaderMapInfo.mergeHeaderMapInfo(hmapProvider.getInfo()); + } + } + Artifact depHeaderMap = ruleContext.getPackageRelativeArtifact(PathFragment.create(targetName + ".hmap"), + ruleContext + .getConfiguration() + .getGenfilesDirectory(ruleContext.getRule().getRepository())); + ruleContext.registerAction( + new HeaderMapAction(ruleContext.getActionOwner(), + depHeaderMapInfo.build().getSources(), + depHeaderMap)); + ccCompilationInfoBuilder.addIncludeDir(depHeaderMap.getExecPath()); + headerMapsBuilder.add(depHeaderMap); + + // If we have headermaps, then we need to add an include of + // the working directory ( i.e. exec root ) in this form + // and it must be after including the header map files + ccCompilationInfoBuilder.addIncludeDir(PathFragment.create(".")); + ImmutableList headerMaps = headerMapsBuilder.build(); + ccCompilationInfoBuilder.setHeaderMaps(headerMaps); + } + + if (featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAPS)) { if (cppModuleMap == null) { cppModuleMap = CppHelper.createDefaultCppModuleMap(ruleContext, /*suffix=*/ ""); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationInfo.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationInfo.java index b0208627231b7f..fcc895552721f9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationInfo.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationInfo.java @@ -23,6 +23,8 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.MiddlemanFactory; import com.google.devtools.build.lib.analysis.RuleContext; +import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; @@ -84,6 +86,7 @@ public final class CcCompilationInfo extends NativeInfo { private final CppModuleMap cppModuleMap; private final CppModuleMap verificationModuleMap; + private final ImmutableList headerMaps; private final boolean propagateModuleMapAsActionInput; @@ -105,7 +108,8 @@ public final class CcCompilationInfo extends NativeInfo { NestedSet directModuleMaps, CppModuleMap cppModuleMap, @Nullable CppModuleMap verificationModuleMap, - boolean propagateModuleMapAsActionInput) { + boolean propagateModuleMapAsActionInput, + ImmutableList headerMaps) { super(PROVIDER); Preconditions.checkNotNull(commandLineCcCompilationInfo); this.commandLineCcCompilationInfo = commandLineCcCompilationInfo; @@ -117,6 +121,7 @@ public final class CcCompilationInfo extends NativeInfo { this.moduleInfo = moduleInfo; this.picModuleInfo = picModuleInfo; this.cppModuleMap = cppModuleMap; + this.headerMaps = headerMaps; this.nonCodeInputs = nonCodeInputs; this.verificationModuleMap = verificationModuleMap; this.compilationPrerequisites = compilationPrerequisites; @@ -233,6 +238,9 @@ public NestedSet getAdditionalInputs() { if (cppModuleMap != null && propagateModuleMapAsActionInput) { builder.add(cppModuleMap.getArtifact()); } + if (headerMaps != null) { + builder.addAll(headerMaps); + } return builder.build(); } @@ -282,7 +290,8 @@ public static CcCompilationInfo disallowUndeclaredHeaders(CcCompilationInfo ccCo ccCompilationInfo.directModuleMaps, ccCompilationInfo.cppModuleMap, ccCompilationInfo.verificationModuleMap, - ccCompilationInfo.propagateModuleMapAsActionInput); + ccCompilationInfo.propagateModuleMapAsActionInput, + ccCompilationInfo.headerMaps); } /** @@ -335,7 +344,8 @@ public static CcCompilationInfo mergeForLipo( mergeSets(ownerCcCompilationInfo.directModuleMaps, libCcCompilationInfo.directModuleMaps), libCcCompilationInfo.cppModuleMap, libCcCompilationInfo.verificationModuleMap, - libCcCompilationInfo.propagateModuleMapAsActionInput); + libCcCompilationInfo.propagateModuleMapAsActionInput, + libCcCompilationInfo.headerMaps); } /** @@ -405,6 +415,8 @@ public static class Builder { private final Set defines = new LinkedHashSet<>(); private CppModuleMap cppModuleMap; private CppModuleMap verificationModuleMap; + private ImmutableList headerMaps; + private boolean propagateModuleMapAsActionInput = true; /** The rule that owns the context */ @@ -637,6 +649,11 @@ public Builder setCppModuleMap(CppModuleMap cppModuleMap) { return this; } + public Builder setHeaderMaps(ImmutableList headerMaps) { + this.headerMaps = headerMaps; + return this; + } + /** Sets the C++ module map used to verify that headers are modules compatible. */ public Builder setVerificationModuleMap(CppModuleMap verificationModuleMap) { this.verificationModuleMap = verificationModuleMap; @@ -705,7 +722,8 @@ public CcCompilationInfo build(ActionOwner owner, MiddlemanFactory middlemanFact directModuleMaps.build(), cppModuleMap, verificationModuleMap, - propagateModuleMapAsActionInput); + propagateModuleMapAsActionInput, + headerMaps); } /** diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMap.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMap.java new file mode 100644 index 00000000000000..66510cb614f08c --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMap.java @@ -0,0 +1,325 @@ +package com.google.devtools.build.lib.rules.cpp; + +import java.io.File; +import java.io.FileOutputStream; + +import java.util.Map; +import java.util.HashMap; +import java.util.Set; +import java.util.ArrayList; +import java.util.List; +import java.nio.ByteBuffer; +import java.nio.CharBuffer; +import java.nio.ByteOrder; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; +import java.nio.channels.FileChannel; + +import java.nio.file.Path; +import java.nio.file.Paths; + +import static java.lang.Math.max; + +public final class ClangHeaderMap { + // Logical representation of a bucket. + // The actual data is stored in the string pool. + private class HMapBucket { + String key; + String prefix; + String suffix; + + HMapBucket(String key, String prefix, String suffix) { + this.key = key; + this.prefix = prefix; + this.suffix = suffix; + } + } + + private static final int HEADER_MAGIC = ('h' << 24) | ('m' << 16) | ('a' << 8) | 'p'; + private static final short HEADER_VERSION = 1; + private static final short HEADER_RESERVED = 0; + private static final int EMPTY_BUCKET_KEY = 0; + + private static final int HEADER_SIZE = 24; + private static final int BUCKET_SIZE = 12; + + private static final int INT_SIZE = Integer.SIZE/8; + + // Data stored in accordance to Clang's lexer types + /** + enum { + HMAP_HeaderMagicNumber = ('h' << 24) | ('m' << 16) | ('a' << 8) | 'p', + HMAP_HeaderVersion = 1, + HMAP_EmptyBucketKey = 0 + }; + + struct HMapBucket { + uint32_t Key; // Offset (into strings) of key. + uint32_t Prefix; // Offset (into strings) of value prefix. + uint32_t Suffix; // Offset (into strings) of value suffix. + }; + + struct HMapHeader { + uint32_t Magic; // Magic word, also indicates byte order. + uint16_t Version; // Version number -- currently 1. + uint16_t Reserved; // Reserved for future use - zero for now. + uint32_t StringsOffset; // Offset to start of string pool. + uint32_t NumEntries; // Number of entries in the string table. + uint32_t NumBuckets; // Number of buckets (always a power of 2). + uint32_t MaxValueLength; // Length of longest result path (excluding nul). + // An array of 'NumBuckets' HMapBucket objects follows this header. + // Strings follow the buckets, at StringsOffset. + }; + */ + public ByteBuffer buff; + + private int numBuckets; + private int numUsedBuckets; + private int stringsOffset; + private int stringsSize; + private int maxValueLength; + private int maxStringsSize; + + // Used only for creation + private HMapBucket[] buckets; + + // Create a headermap from a raw Map of keys to strings + // Usage: + // A given path to a header is keyed by that header. + // .e. Header.h -> Path/To/Header.h + // + // Additionally, it is possible to alias custom paths to headers. + // For example, it is possible to namespace a given target + // i.e. MyTarget/Header.h -> Path/To/Header.h + // + // The HeaderMap format is defined by the lexer of Clang + // https://clang.llvm.org/doxygen/HeaderMap_8cpp_source.html + ClangHeaderMap(Map headerPathsByKeys) { + int dataOffset = 1; + setMap(headerPathsByKeys); + + int endBuckets = HEADER_SIZE + numBuckets * BUCKET_SIZE; + stringsOffset = endBuckets - dataOffset; + int totalBufferSize = endBuckets + maxStringsSize; + buff = ByteBuffer.wrap(new byte[totalBufferSize]).order(ByteOrder.LITTLE_ENDIAN); + + // Write out the header + buff.putInt(HEADER_MAGIC); + buff.putShort(HEADER_VERSION); + buff.putShort(HEADER_RESERVED); + buff.putInt(stringsOffset); + + // For each entry, we write a key, suffix, and prefix + int stringPoolSize = headerPathsByKeys.size() * 3; + buff.putInt(stringPoolSize); + buff.putInt(numBuckets); + buff.putInt(maxValueLength); + + // Write out buckets and compute string offsets + byte[] stringBytes = new byte[maxStringsSize]; + + // Used to compute the current offset + stringsSize = 0; + for (int i = 0; i < numBuckets; i++) { + HMapBucket bucket = buckets[i]; + if (bucket == null) { + buff.putInt(EMPTY_BUCKET_KEY); + buff.putInt(0); + buff.putInt(0); + } else { + int keyOffset = stringsSize; + buff.putInt(keyOffset + dataOffset); + stringsSize = addString(bucket.key, stringsSize, stringBytes); + + int prefixOffset = stringsSize; + stringsSize = addString(bucket.prefix, stringsSize, stringBytes); + buff.putInt(prefixOffset + dataOffset); + + int suffixOffset = stringsSize; + stringsSize = addString(bucket.suffix, stringsSize, stringBytes); + buff.putInt(suffixOffset + dataOffset); + } + } + buff.put(stringBytes, 0, stringsSize); + } + + // For testing purposes. Implement a similiar algorithm as the clang + // lexer. + public String get(String key) { + int bucketIdx = clangKeyHash(key) & (numBuckets - 1); + while (bucketIdx < numBuckets) { + // Buckets are right after the header + int bucketOffset = HEADER_SIZE + (BUCKET_SIZE * bucketIdx); + int keyOffset = buff.getInt(bucketOffset); + + // Note: the lexer does a case insensitive compare here but + // it isn't necessary for test purposes + if (key.equals(getString(keyOffset)) == false) { + bucketIdx++; + continue; + } + + // Start reading bytes from the prefix + int prefixOffset = buff.getInt(bucketOffset + INT_SIZE); + int suffixOffset = buff.getInt(bucketOffset + INT_SIZE * 2); + return getString(prefixOffset) + getString(suffixOffset); + } + return null; + } + + // Return a string from an offset + // This method is used for testing only. + private String getString(int offset) { + int readOffset = stringsOffset + offset; + int endStringsOffset = stringsOffset + stringsSize; + int idx = readOffset; + byte[] stringBytes = new byte[2048]; + while(idx < endStringsOffset) { + byte c = (byte)buff.getChar(idx); + if (c == 0) { + break; + } + stringBytes[idx] = c; + idx++; + } + try { + return new String(stringBytes).trim(); + } catch(Exception e) { + return null; + } + } + + private void addBucket(HMapBucket bucket, HMapBucket[] buckets, int numBuckets) { + // Use a load factor of 0.5 + if (((numUsedBuckets + 1) / numBuckets) > 0.5 == false) { + int bucketIdx = clangKeyHash(bucket.key) & (numBuckets - 1); + // Base case, the bucket Idx is free + if (buckets[bucketIdx] == null) { + buckets[bucketIdx] = bucket; + this.numUsedBuckets++; + return; + } + + // Handle collisions. + // + // The lexer does a linear scan of the hash table when keys do + // not match, starting at the bucket. + while(bucketIdx < numBuckets) { + bucketIdx = (bucketIdx + 1) & (numBuckets - 1); + if (buckets[bucketIdx] == null) { + buckets[bucketIdx] = bucket; + this.numUsedBuckets++; + return; + } + } + } + + // If there are no more slots left, grow by a power of 2 + int newNumBuckets = numBuckets * 2; + HMapBucket[] newBuckets = new HMapBucket[newNumBuckets]; + + HMapBucket[] oldBuckets = buckets; + this.buckets = newBuckets; + this.numBuckets = newNumBuckets; + this.numUsedBuckets = 0; + for(HMapBucket cpBucket: oldBuckets) { + if (cpBucket != null) { + addBucket(cpBucket, newBuckets, newNumBuckets); + } + } + + // Start again + addBucket(bucket, newBuckets, newNumBuckets); + } + + private void setMap(Map headerPathsByKeys){ + // Compute header metadata + maxValueLength = 1; + maxStringsSize = 0; + numUsedBuckets = 0; + + // Per the format, buckets need to be powers of 2 in size + numBuckets = getNextPowerOf2(headerPathsByKeys.size() + 1); + buckets = new HMapBucket[numBuckets]; + + for(Map.Entry entry: headerPathsByKeys.entrySet()){ + String key = entry.getKey(); + String path = entry.getValue(); + + // Get the prefix and suffix + String suffix; + String prefix; + Path pathValue = Paths.get(path); + if (pathValue.getNameCount() < 2) { + // The suffix is empty when the file path just a filename + prefix = ""; + suffix = pathValue.getFileName().toString(); + } else { + prefix = pathValue.getParent().toString() + "/"; + suffix = pathValue.getFileName().toString(); + } + + HMapBucket bucket = new HMapBucket(key, prefix, suffix); + addBucket(bucket, buckets, numBuckets); + int prefixLen = prefix.getBytes().length + 1; + int suffixLen = suffix.getBytes().length + 1; + int keyLen = key.getBytes().length + 1; + maxStringsSize += prefixLen + suffixLen + keyLen; + + maxValueLength = max(maxValueLength, keyLen); + maxValueLength = max(maxValueLength, suffixLen); + maxValueLength = max(maxValueLength, prefixLen); + } + } + + // Utils + + private static int addString(String str, int totalLength, byte[] stringBytes) { + for (byte b : str.getBytes(StandardCharsets.UTF_8)) { + stringBytes[totalLength] = b; + totalLength++; + } + stringBytes[totalLength] = (byte) 0; + totalLength++; + return totalLength; + } + + private static int getNextPowerOf2(int a) { + int b = 1; + while (b < a) { + b = b << 1; + } + return b; + } + + // The same hashing algorithm as the Lexer. + // Buckets must be inserted according to this. + private static int clangKeyHash(String key) { + // Keys are case insensitve. + String lowerCaseKey = toLowerCaseAscii(key); + int hash = 0; + for (byte c : lowerCaseKey.getBytes(StandardCharsets.UTF_8)) { + hash += c * 13; + } + return hash; + } + + // Utils from Guava ( FIXME: use those utils ) + public static String toLowerCaseAscii(String string) { + int length = string.length(); + StringBuilder builder = new StringBuilder(length); + for (int i = 0; i < length; i++) { + builder.append(toLowerCaseAscii(string.charAt(i))); + } + return builder.toString(); + } + + public static char toLowerCaseAscii(char c) { + return isUpperCase(c) ? (char) (c ^ 0x20) : c; + } + + public static boolean isUpperCase(char c) { + return (c >= 'A') && (c <= 'Z'); + } +} + diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java index 8ff1f8c065e2c0..869143e43349ee 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java @@ -1035,6 +1035,13 @@ public boolean fissionIsActiveForCurrentCompilationMode() { return cppOptions.fissionModes.contains(compilationMode); } + /** + * Returns whether we are implicitly creating header maps. + */ + public boolean experimentalEnableImplicitHeaderMaps() { + return cppOptions.experimentalEnableImplicitHeaderMaps; + } + /** Returns true if --build_test_dwp is set on this build. */ public boolean buildTestDwpIsActivated() { return cppOptions.buildTestDwp; diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java index e129cc8d839763..5c943758a93bfd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java @@ -315,6 +315,18 @@ public LipoModeConverter() { ) public boolean processHeadersInDependencies; + @Option( + name = "experimental_enable_implicit_headermaps", + defaultValue = "false", + category = "semantics", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "When building a target //a:a, generate a header map containing all of the headrs that //a:a depends " + + "on (if header processing is enabled for the toolchain)." + ) + public boolean experimentalEnableImplicitHeaderMaps; + @Option( name = "copt", allowMultiple = true, diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapAction.java new file mode 100644 index 00000000000000..af41ef08a6900a --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapAction.java @@ -0,0 +1,87 @@ +package com.google.devtools.build.lib.rules.cpp; + +//TODO: Cleanup Imports +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Optional; +import com.google.common.base.Strings; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.actions.ActionExecutionContext; +import com.google.devtools.build.lib.actions.ActionKeyContext; +import com.google.devtools.build.lib.actions.ActionOwner; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; +import com.google.devtools.build.lib.actions.CommandLine; +import com.google.devtools.build.lib.actions.CommandLineExpansionException; +import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction; +import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; +import com.google.devtools.build.lib.util.Fingerprint; +import com.google.devtools.build.lib.vfs.PathFragment; +import java.io.IOException; +import java.io.OutputStream;//? +import java.io.DataOutputStream;//? +import java.nio.ByteBuffer; +import java.nio.channels.Channels; +import java.nio.channels.WritableByteChannel; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.nio.channels.FileChannel; +import java.io.FileOutputStream; + +@Immutable +public final class HeaderMapAction extends AbstractFileWriteAction { + + private static final String GUID = "4f407081-1951-40c1-befc-d6b4daff5de3"; + + // C++ header map of the current target + private final Map headerMap; + + public HeaderMapAction( + ActionOwner owner, + Map headerMap, + Artifact output + ) { + super( + owner, + ImmutableList.builder().build(), + output, + /*makeExecutable=*/ false); + this.headerMap = headerMap; + } + + @Override + public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { + return new DeterministicWriter() { + @Override + public void writeOutputFile(OutputStream out) throws IOException { + ClangHeaderMap hmap = new ClangHeaderMap(headerMap); + ByteBuffer b = hmap.buff; + b.flip(); + WritableByteChannel channel = Channels.newChannel(out); + channel.write(b); + out.flush(); + out.close(); + } + }; + } + + @Override + public String getMnemonic() { + return "CppHeaderMap"; + } + + @Override + protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint f) + throws CommandLineExpansionException { + f.addString(GUID); + for(Map.Entry entry: headerMap.entrySet()){ + String key = entry.getKey(); + String path = entry.getValue(); + f.addString(key + path); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfo.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfo.java new file mode 100644 index 00000000000000..6d9dd99bd3d948 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfo.java @@ -0,0 +1,86 @@ +package com.google.devtools.build.lib.rules.cpp; + +import com.google.devtools.build.lib.vfs.Path; +import java.util.Collection; +import java.util.List; +import java.util.HashMap; +import java.util.Map; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableList; + +public class HeaderMapInfo { + private final ImmutableMap sources; + + public final static HeaderMapInfo EMPTY = new HeaderMapInfo(ImmutableMap.of()); + + private HeaderMapInfo(ImmutableMap sources) { + this.sources = sources; + } + + public ImmutableMap getSources() { + return sources; + } + + /** Builder for HeaderMapInfo */ + public static class Builder { + private final ImmutableList.Builder basicHeaders = ImmutableList.builder(); + private final ImmutableList.Builder namespacedHeaders = ImmutableList.builder(); + private final ImmutableList.Builder mergedHeaderMapInfos = ImmutableList.builder(); + private String namespace = ""; + + /** Set the namespace. */ + public Builder setNamespace(String namespace) { + this.namespace = namespace; + return this; + } + + /** Signals that the build uses headers. */ + public Builder addHeaders(Iterable headers) { + this.basicHeaders.addAll(headers); + return this; + } + + /** Signals that the build uses headers under the namespace. */ + public Builder addNamespacedHeaders(Iterable headers) { + this.namespacedHeaders.addAll(headers); + return this; + } + + /** + * Merge a header map info. + * Merged HeaderMapInfos are merged in reverse that they were added. + * Directly added headers take precedence over those that were merged. + */ + public Builder mergeHeaderMapInfo(HeaderMapInfo info) { + this.mergedHeaderMapInfos.add(info); + return this; + } + + public HeaderMapInfo build() { + Map inputMap = new HashMap(); + for (HeaderMapInfo info: mergedHeaderMapInfos.build().reverse()){ + for (Map.Entry entry: info.getSources().entrySet()){ + inputMap.put(entry.getKey(), entry.getValue()); + } + } + + for (Artifact hdr : basicHeaders.build()) { + inputMap.put(hdr.getPath().getBaseName(), hdr.getExecPath().getPathString()); + } + + // If there is no namespace, don't add a slash + if (namespace.equals("") == false) { + for (Artifact hdr : namespacedHeaders.build()) { + String namespacedKey = namespace + "/" + hdr.getPath().getBaseName(); + inputMap.put(namespacedKey, hdr.getExecPath().getPathString()); + } + } else { + for (Artifact hdr : namespacedHeaders.build()) { + inputMap.put(hdr.getPath().getBaseName(), hdr.getExecPath().getPathString()); + } + } + return new HeaderMapInfo(ImmutableMap.copyOf(inputMap)); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfoProvider.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfoProvider.java new file mode 100644 index 00000000000000..0fa716cc37d99f --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfoProvider.java @@ -0,0 +1,33 @@ +package com.google.devtools.build.lib.rules.cpp; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; +import com.google.devtools.build.lib.util.FileTypeSet; + +public class HeaderMapInfoProvider implements TransitiveInfoProvider { + private HeaderMapInfo info; + + public static HeaderMapInfoProvider EMPTY = new HeaderMapInfoProvider(HeaderMapInfo.EMPTY); + + public HeaderMapInfo getInfo() { + return info; + } + + private HeaderMapInfoProvider(HeaderMapInfo info) { + this.info = info; + } + + /** Builder for HeaderMapInfoProvider */ + public static class Builder { + private HeaderMapInfo info; + + public Builder setHeaderMapInfo(HeaderMapInfo info) { + this.info = info; + return this; + } + + public HeaderMapInfoProvider build() { + return new HeaderMapInfoProvider(this.info); + } + } +} 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 bb20db497cb43d..d681ce24e83be5 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 @@ -66,19 +66,36 @@ import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode; import com.google.devtools.build.lib.packages.Info; import com.google.devtools.build.lib.packages.NativeProvider; +import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.rules.apple.AppleToolchain; import com.google.devtools.build.lib.rules.cpp.CcCompilationInfo; import com.google.devtools.build.lib.rules.cpp.CcLinkParams; import com.google.devtools.build.lib.rules.cpp.CcLinkParamsInfo; + import com.google.devtools.build.lib.rules.cpp.CppFileTypes; import com.google.devtools.build.lib.rules.cpp.CppModuleMap; import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; + +import com.google.devtools.build.lib.rules.cpp.CppConfiguration; +import com.google.devtools.build.lib.rules.cpp.CppFileTypes; +import com.google.devtools.build.lib.rules.cpp.CppModuleMap; +import com.google.devtools.build.lib.rules.cpp.HeaderMapAction; +import com.google.devtools.build.lib.rules.cpp.HeaderMapInfo; +import com.google.devtools.build.lib.rules.cpp.HeaderMapInfoProvider; +import com.google.devtools.build.lib.syntax.Type; + + import com.google.devtools.build.lib.util.FileType; import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.HashMap; +import java.util.Map; +/** +import com.google.devtools.build.lib.actions.Root; +*/ /** * Contains information common to multiple objc_* rules, and provides a unified API for extracting @@ -153,6 +170,7 @@ static class Builder { private Iterable dynamicFrameworkImports = ImmutableList.of(); private Optional compilationArtifacts = Optional.absent(); private Iterable depObjcProviders = ImmutableList.of(); + private Iterable headerMapInfoProviders = ImmutableList.of(); private Iterable directDepObjcProviders = ImmutableList.of(); private Iterable runtimeDepObjcProviders = ImmutableList.of(); private Iterable defines = ImmutableList.of(); @@ -346,6 +364,30 @@ Builder setAlwayslink(boolean alwayslink) { return this; } + public HeaderMapInfoProvider getHeaderMapInfoProvider(RuleContext ruleContext, ImmutableListhdrs){ + HeaderMapInfo.Builder headerMapInfo = new HeaderMapInfo.Builder(); + String namespace; + if (ruleContext.attributes().has("header_namespace")) { + namespace = ruleContext.attributes().get("header_namespace", Type.STRING); + } else { + namespace = ruleContext.getRule().getName(); + } + + headerMapInfo.setNamespace(namespace); + headerMapInfo.addHeaders(hdrs); + headerMapInfo.addNamespacedHeaders(hdrs); + + if (ruleContext.attributes().has("deps")){ + // Propagate all of the dep sources + for (HeaderMapInfoProvider hmapProvider : ruleContext.getPrerequisites("deps", Mode.TARGET, HeaderMapInfoProvider.class)) { + headerMapInfo.mergeHeaderMapInfo(hmapProvider.getInfo()); + } + } + return new HeaderMapInfoProvider.Builder() + .setHeaderMapInfo(headerMapInfo.build()).build(); + } + + /** * Specifies that this target has a clang module map. This should be called if this target * compiles sources or exposes headers for other targets to use. Note that this does not add @@ -392,6 +434,7 @@ Builder setLinkmapFile(Artifact linkmapFile) { ObjcCommon build() { Iterable bundleImports = BundleableFile.bundleImportsFromRule(context); + ImmutableList.Builder headerMapHeaders = ImmutableList.builder(); ObjcProvider.Builder objcProvider = new ObjcProvider.Builder() @@ -420,6 +463,8 @@ ObjcCommon build() { for (CcCompilationInfo headerProvider : depCcHeaderProviders) { objcProvider.addAll(HEADER, filterFileset(headerProvider.getDeclaredIncludeSrcs())); + headerMapHeaders.addAll(filterFileset(headerProvider.getDeclaredIncludeSrcs())); + objcProvider.addAll(INCLUDE, headerProvider.getIncludeDirs()); // TODO(bazel-team): This pulls in stl via // CppHelper.mergeToolchainDependentCcCompilationInfo but @@ -457,6 +502,9 @@ ObjcCommon build() { AppleToolchain.sdkDir() + "/usr/include/", Iterables.transform(attributes.sdkIncludes(), PathFragment::getSafePathString)), PathFragment::create); + + headerMapHeaders.addAll(filterFileset(attributes.hdrs())); + headerMapHeaders.addAll(filterFileset(attributes.textualHdrs())); objcProvider .addAll(HEADER, filterFileset(attributes.hdrs())) .addAll(HEADER, filterFileset(attributes.textualHdrs())) @@ -499,6 +547,7 @@ ObjcCommon build() { Iterables.concat(artifacts.getSrcs(), artifacts.getNonArcSrcs()); // TODO(bazel-team): Add private headers to the provider when we have module maps to enforce // them. + headerMapHeaders.addAll(artifacts.getPrivateHdrs()); objcProvider .addAll(HEADER, filterFileset(artifacts.getAdditionalHdrs())) .addAll(LIBRARY, artifacts.getArchive().asSet()) @@ -564,7 +613,14 @@ ObjcCommon build() { .add(DEBUG_SYMBOLS_PLIST, intermediateArtifacts.dsymPlist(dsymOutputType)); } - return new ObjcCommon(objcProvider.build(), compilationArtifacts); + HeaderMapInfoProvider headerMapProvider; + if (context.isLegalFragment(CppConfiguration.class) && context.getFragment(CppConfiguration.class).experimentalEnableImplicitHeaderMaps()) { + headerMapProvider = getHeaderMapInfoProvider(context, headerMapHeaders.build()); + } else { + headerMapProvider = HeaderMapInfoProvider.EMPTY; + } + + return new ObjcCommon(objcProvider.build(), headerMapProvider, compilationArtifacts); } private static boolean isCcLibrary(ConfiguredTargetAndData info) { @@ -601,12 +657,14 @@ private static boolean useLaunchStoryboard(RuleContext ruleContext) { public static final FileType FRAMEWORK_CONTAINER_TYPE = FileType.of(".framework"); private final ObjcProvider objcProvider; + private final HeaderMapInfoProvider headerMapInfoProvider; private final Optional compilationArtifacts; private ObjcCommon( - ObjcProvider objcProvider, Optional compilationArtifacts) { + ObjcProvider objcProvider, HeaderMapInfoProvider headerMapInfoProvider, Optional compilationArtifacts) { this.objcProvider = Preconditions.checkNotNull(objcProvider); + this.headerMapInfoProvider = headerMapInfoProvider; this.compilationArtifacts = Preconditions.checkNotNull(compilationArtifacts); } @@ -614,6 +672,10 @@ public ObjcProvider getObjcProvider() { return objcProvider; } + public HeaderMapInfoProvider getHeaderMapInfoProvider() { + return headerMapInfoProvider; + } + public Optional getCompilationArtifacts() { return compilationArtifacts; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcLibrary.java index 0fa2870432328b..1d263b77f6ae27 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcLibrary.java @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.rules.cpp.CcCompilationInfo; import com.google.devtools.build.lib.rules.cpp.CcLinkParamsInfo; +import com.google.devtools.build.lib.rules.cpp.HeaderMapInfoProvider; import com.google.devtools.build.lib.rules.objc.ObjcCommon.ResourceAttributes; import com.google.devtools.build.lib.syntax.Type; import java.util.Map; @@ -101,6 +102,7 @@ public ConfiguredTarget create(RuleContext ruleContext) return ObjcRuleClasses.ruleConfiguredTarget(ruleContext, filesToBuild.build()) .addNativeDeclaredProvider(common.getObjcProvider()) .addNativeDeclaredProvider(ccCompilationInfo) + .addProvider(common.getHeaderMapInfoProvider()) .addProvider(J2ObjcEntryClassProvider.class, j2ObjcEntryClassProvider) .addProvider(J2ObjcMappingFileProvider.class, j2ObjcMappingFileProvider) .addProvider( 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 4cb6fe39533d9f..391c7cff1a4a8e 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 @@ -703,6 +703,14 @@ Enables clang module support (via -fmodules). provided module map to the compiler. */ .add(attr("module_map", LABEL).allowedFileTypes(FileType.of(".modulemap"))) + + /* + The namespace that headers in this rule may be imported as. + If not set, it defaults to the name of the target. + */ + .add(attr("header_namespace", Type.STRING)) + /* Provides the label for header_scanner tool that is used to scan inclusions for ObjC sources and provide a list of required headers via a .header_list file. diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMapTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMapTest.java new file mode 100644 index 00000000000000..ef7decb0235549 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMapTest.java @@ -0,0 +1,41 @@ +// Copyright 2015 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.rules.cpp; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.devtools.build.lib.testutil.FoundationTestCase; +import java.util.HashMap; +import java.util.Map; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + + +public class ClangHeaderMapTest extends FoundationTestCase { + @Test + public void testNamespacedHeaderMap() { + Map paths = new HashMap(); + paths.put("MyHeader.h", "include/MyHeader.h"); + paths.put("X/MyHeader2.h", "include/MyHeader2.h"); + paths.put("X/MyHeader3.h", "include/MyHeader3.h"); + + ClangHeaderMap hmap = new ClangHeaderMap(paths); + assertThat(hmap.get("MyHeader.h")).isEqualTo("include/MyHeader.h"); + assertThat(hmap.get("X/MyHeader2.h")).isEqualTo("include/MyHeader2.h"); + assertThat(hmap.get("X/MyHeader3.h")).isEqualTo("include/MyHeader3.h"); + } +} + diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java index 8bb93d66f8e019..aba9d0362addb4 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java @@ -1792,6 +1792,41 @@ public void testLegacyObjcProtoLibraryDoesNotCrash() throws Exception { assertThat(getConfiguredTarget("//x:objc")).isNotNull(); } + @Test + public void testObjcNamespacedImports() throws Exception { + useConfiguration( + "--crosstool_top=" + MockObjcSupport.DEFAULT_OSX_CROSSTOOL, + "--experimental_objc_crosstool=all", + "--experimental_enable_implicit_headermaps=true"); + scratch.file( + "x/importer.m", + // By default, we would be able to import this header + // under the namespace of dep_liba. + "#import \"dep_liba/header.h\"", + // This header has a custom namespace + "#import \"some/headerb.h\""); + scratch.file( + "x/headerb.h"); + scratch.file( + "x/BUILD", + "objc_library(", + " name = 'objc',", + " srcs = ['importer.m'],", + " deps = [':dep_liba', ':dep_libb'],", + ")", + "objc_library(", + " name = 'dep_liba',", + " hdrs = ['header.h'],", + ")", + "objc_library(", + " name = 'dep_libb',", + " header_namespace = 'some',", + " hdrs = ['headerb.h'],", + ")"); + + assertThat(getConfiguredTarget("//x:objc")).isNotNull(); + } + @Test public void testObjcImportDoesNotCrash() throws Exception { useConfiguration("--crosstool_top=" + MockObjcSupport.DEFAULT_OSX_CROSSTOOL); From 56d7784f9099602582110c9734ae95cffc13a551 Mon Sep 17 00:00:00 2001 From: Jerry Marino Date: Mon, 19 Mar 2018 15:13:21 -0700 Subject: [PATCH 2/3] Start integrating include prefix headers for Objc --- .../lib/rules/cpp/CcCompilationHelper.java | 22 ++++++------ .../build/lib/rules/cpp/HeaderMapInfo.java | 35 +++++++++++-------- .../build/lib/rules/objc/ObjcCommon.java | 14 ++++---- .../build/lib/rules/objc/ObjcRuleClasses.java | 11 +++--- 4 files changed, 47 insertions(+), 35 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java index a3454e466098bf..10ae96cc51c867 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java @@ -1058,25 +1058,27 @@ public CcCompilationInfo initializeCcCompilationInfo() { ccCompilationInfoBuilder.addQuoteIncludeDir(internalHeaderMap.getExecPath()); headerMapsBuilder.add(internalHeaderMap); - String namespace; - if (ruleContext.attributes().has("header_namespace")) { - namespace = ruleContext.attributes().get("header_namespace", Type.STRING); + String includePrefix; + if (ruleContext.attributes().has("include_prefix")) { + includePrefix = ruleContext.attributes().get("include_prefix", Type.STRING); } else { - namespace = ruleContext.getRule().getName(); + includePrefix = ruleContext.getRule().getName(); } // Construct the dep headermap. - // This header map additionally contains namespaced headers so that a user + // This header map additionally contains include prefixed headers so that a user // can import headers of the form Namespace/Header.h from headers within // the current target. HeaderMapInfo.Builder depHeaderMapInfo = new HeaderMapInfo.Builder(); - depHeaderMapInfo.setNamespace(namespace); - depHeaderMapInfo.addNamespacedHeaders(publicHeaders.getHeaders()); + depHeaderMapInfo.setIncludePrefix(includePrefix); + depHeaderMapInfo.addIncludePrefixdHeaders(publicHeaders.getHeaders()); + depHeaderMapInfo.addIncludePrefixdHeaders(privateHeaders); + depHeaderMapInfo.addIncludePrefixdHeaders(publicTextualHeaders); + + // TODO flatten_virtual_headers + depHeaderMapInfo.addHeaders(publicTextualHeaders); depHeaderMapInfo.addHeaders(publicHeaders.getHeaders()); - depHeaderMapInfo.addNamespacedHeaders(privateHeaders); depHeaderMapInfo.addHeaders(privateHeaders); - depHeaderMapInfo.addNamespacedHeaders(publicTextualHeaders); - depHeaderMapInfo.addHeaders(publicTextualHeaders); // Merge all of the header map info from deps. The headers within a given // target have precedence over over dep headers ( See diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfo.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfo.java index 6d9dd99bd3d948..7387951b354486 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfo.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfo.java @@ -25,25 +25,30 @@ public ImmutableMap getSources() { /** Builder for HeaderMapInfo */ public static class Builder { private final ImmutableList.Builder basicHeaders = ImmutableList.builder(); - private final ImmutableList.Builder namespacedHeaders = ImmutableList.builder(); + private final ImmutableList.Builder includePrefixdHeaders = ImmutableList.builder(); private final ImmutableList.Builder mergedHeaderMapInfos = ImmutableList.builder(); - private String namespace = ""; + private String includePrefix = ""; - /** Set the namespace. */ - public Builder setNamespace(String namespace) { - this.namespace = namespace; + /** Set include prefix. */ + public Builder setIncludePrefix(String includePrefix) { + this.includePrefix = includePrefix; return this; } - /** Signals that the build uses headers. */ + /** + * Signals that the build uses headers. + * + * If `flatten_virtual_headers` is set, the headers will be mapped to + * "Header.h" -> path/to/Header. + */ public Builder addHeaders(Iterable headers) { this.basicHeaders.addAll(headers); return this; } - /** Signals that the build uses headers under the namespace. */ - public Builder addNamespacedHeaders(Iterable headers) { - this.namespacedHeaders.addAll(headers); + /** Signals that the build uses headers under the includePrefix. */ + public Builder addIncludePrefixdHeaders(Iterable headers) { + this.includePrefixdHeaders.addAll(headers); return this; } @@ -69,14 +74,14 @@ public HeaderMapInfo build() { inputMap.put(hdr.getPath().getBaseName(), hdr.getExecPath().getPathString()); } - // If there is no namespace, don't add a slash - if (namespace.equals("") == false) { - for (Artifact hdr : namespacedHeaders.build()) { - String namespacedKey = namespace + "/" + hdr.getPath().getBaseName(); - inputMap.put(namespacedKey, hdr.getExecPath().getPathString()); + // If there is no includePrefix, don't add a slash + if (includePrefix.equals("") == false) { + for (Artifact hdr : includePrefixdHeaders.build()) { + String includePrefixdKey = includePrefix + "/" + hdr.getPath().getBaseName(); + inputMap.put(includePrefixdKey, hdr.getExecPath().getPathString()); } } else { - for (Artifact hdr : namespacedHeaders.build()) { + for (Artifact hdr : includePrefixdHeaders.build()) { inputMap.put(hdr.getPath().getBaseName(), hdr.getExecPath().getPathString()); } } 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 d681ce24e83be5..5ecd043894ac60 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 @@ -366,16 +366,18 @@ Builder setAlwayslink(boolean alwayslink) { public HeaderMapInfoProvider getHeaderMapInfoProvider(RuleContext ruleContext, ImmutableListhdrs){ HeaderMapInfo.Builder headerMapInfo = new HeaderMapInfo.Builder(); - String namespace; - if (ruleContext.attributes().has("header_namespace")) { - namespace = ruleContext.attributes().get("header_namespace", Type.STRING); + String includePrefix; + if (ruleContext.attributes().has("include_prefix")) { + includePrefix = ruleContext.attributes().get("include_prefix", Type.STRING); } else { - namespace = ruleContext.getRule().getName(); + includePrefix = ruleContext.getRule().getName(); } - headerMapInfo.setNamespace(namespace); + headerMapInfo.setIncludePrefix(includePrefix); + headerMapInfo.addIncludePrefixdHeaders(hdrs); + + // TODO flatten_virtual_headers headerMapInfo.addHeaders(hdrs); - headerMapInfo.addNamespacedHeaders(hdrs); if (ruleContext.attributes().has("deps")){ // Propagate all of the dep sources 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 391c7cff1a4a8e..c6ae8b17b1a217 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 @@ -705,11 +705,14 @@ Enables clang module support (via -fmodules). .add(attr("module_map", LABEL).allowedFileTypes(FileType.of(".modulemap"))) /* - The namespace that headers in this rule may be imported as. - If not set, it defaults to the name of the target. + * #BLAZE_RULE($objc_compiling_rule).ATTRIBUTE(include_prefix) --> */ - .add(attr("header_namespace", Type.STRING)) + .add(attr("include_prefix", Type.STRING)) + + /* + */ + .add(attr("flatten_virtual_headers", BOOLEAN)) /* Provides the label for header_scanner tool that is used to scan inclusions for ObjC sources and provide a list of required headers via a .header_list file. From 91f275af901d40a6978d386bcbddc0257be07bdf Mon Sep 17 00:00:00 2001 From: Jerry Marino Date: Mon, 19 Mar 2018 15:41:23 -0700 Subject: [PATCH 3/3] Integrate flatten_virtual_headers When flatten virtual headers is set, we add all headers to the headermap. This allows a user to include a Header, "path/to/header" as "header.h" Together with include_prefix, this allows bazel to support Xcode style includes out of the box. As a followup, we'll want to make this work without headermaps. + misc cleanups from rebasing --- .../lib/rules/cpp/CcCompilationHelper.java | 24 +++++++++-------- .../build/lib/rules/cpp/ClangHeaderMap.java | 1 - .../build/lib/rules/cpp/CppOptions.java | 2 +- .../build/lib/rules/cpp/HeaderMapInfo.java | 10 +++---- .../build/lib/rules/objc/ObjcCommon.java | 27 ++++++++----------- .../build/lib/rules/objc/ObjcRuleClasses.java | 13 +++++---- .../build/lib/rules/objc/ObjcLibraryTest.java | 2 +- 7 files changed, 38 insertions(+), 41 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java index 10ae96cc51c867..b255848fc69879 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java @@ -24,6 +24,8 @@ 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.common.collect.Streams; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.analysis.AnalysisEnvironment; @@ -71,10 +73,6 @@ import java.util.TreeMap; import javax.annotation.Nullable; - -import com.google.common.collect.Sets; -import com.google.common.collect.Streams; - /** * A class to create C/C++ compile actions in a way that is consistent with cc_library. Rules that * generate source files and emulate cc_library on top of that should use this class instead of the @@ -1039,6 +1037,7 @@ public CcCompilationInfo initializeCcCompilationInfo() { } } + // Setup Experimental implicit header maps if needed if (ruleContext.getFragment(CppConfiguration.class).experimentalEnableImplicitHeaderMaps()) { ImmutableList.Builder headerMapsBuilder = ImmutableList.builder(); String targetName = ruleContext.getTarget().getName(); @@ -1067,7 +1066,7 @@ public CcCompilationInfo initializeCcCompilationInfo() { // Construct the dep headermap. // This header map additionally contains include prefixed headers so that a user - // can import headers of the form Namespace/Header.h from headers within + // can import headers of the form IncludePrefix/Header.h from headers within // the current target. HeaderMapInfo.Builder depHeaderMapInfo = new HeaderMapInfo.Builder(); depHeaderMapInfo.setIncludePrefix(includePrefix); @@ -1075,10 +1074,14 @@ public CcCompilationInfo initializeCcCompilationInfo() { depHeaderMapInfo.addIncludePrefixdHeaders(privateHeaders); depHeaderMapInfo.addIncludePrefixdHeaders(publicTextualHeaders); - // TODO flatten_virtual_headers - depHeaderMapInfo.addHeaders(publicTextualHeaders); - depHeaderMapInfo.addHeaders(publicHeaders.getHeaders()); - depHeaderMapInfo.addHeaders(privateHeaders); + // Flatten virtual headers into the headermap. + boolean flattenVirtualHeaders = ruleContext.attributes().has("flatten_virtual_headers") && + ruleContext.attributes().get("flatten_virtual_headers", Type.BOOLEAN); + if (flattenVirtualHeaders) { + depHeaderMapInfo.addHeaders(publicTextualHeaders); + depHeaderMapInfo.addHeaders(publicHeaders.getHeaders()); + depHeaderMapInfo.addHeaders(privateHeaders); + } // Merge all of the header map info from deps. The headers within a given // target have precedence over over dep headers ( See @@ -1103,11 +1106,10 @@ public CcCompilationInfo initializeCcCompilationInfo() { // the working directory ( i.e. exec root ) in this form // and it must be after including the header map files ccCompilationInfoBuilder.addIncludeDir(PathFragment.create(".")); - ImmutableList headerMaps = headerMapsBuilder.build(); + ImmutableList headerMaps = headerMapsBuilder.build(); ccCompilationInfoBuilder.setHeaderMaps(headerMaps); } - if (featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAPS)) { if (cppModuleMap == null) { cppModuleMap = CppHelper.createDefaultCppModuleMap(ruleContext, /*suffix=*/ ""); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMap.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMap.java index 66510cb614f08c..d0791d6f6b024b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMap.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMap.java @@ -304,7 +304,6 @@ private static int clangKeyHash(String key) { return hash; } - // Utils from Guava ( FIXME: use those utils ) public static String toLowerCaseAscii(String string) { int length = string.length(); StringBuilder builder = new StringBuilder(length); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java index 5c943758a93bfd..67edd066f5eb3f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java @@ -322,7 +322,7 @@ public LipoModeConverter() { documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = {OptionEffectTag.UNKNOWN}, help = - "When building a target //a:a, generate a header map containing all of the headrs that //a:a depends " + "When building a target //a:a, generate a header map containing all of the headers that //a:a depends " + "on (if header processing is enabled for the toolchain)." ) public boolean experimentalEnableImplicitHeaderMaps; diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfo.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfo.java index 7387951b354486..cd0ca125460110 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfo.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderMapInfo.java @@ -38,8 +38,8 @@ public Builder setIncludePrefix(String includePrefix) { /** * Signals that the build uses headers. * - * If `flatten_virtual_headers` is set, the headers will be mapped to - * "Header.h" -> path/to/Header. + * This is used when `flatten_virtual_headers` is set: these headers will be + * mapped to "Header.h" -> path/to/Header. */ public Builder addHeaders(Iterable headers) { this.basicHeaders.addAll(headers); @@ -77,13 +77,11 @@ public HeaderMapInfo build() { // If there is no includePrefix, don't add a slash if (includePrefix.equals("") == false) { for (Artifact hdr : includePrefixdHeaders.build()) { + // Set the include prefix: + // IncludePrefix/Header.h -> Path/To/Header.h String includePrefixdKey = includePrefix + "/" + hdr.getPath().getBaseName(); inputMap.put(includePrefixdKey, hdr.getExecPath().getPathString()); } - } else { - for (Artifact hdr : includePrefixdHeaders.build()) { - inputMap.put(hdr.getPath().getBaseName(), hdr.getExecPath().getPathString()); - } } return new HeaderMapInfo(ImmutableMap.copyOf(inputMap)); } 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 5ecd043894ac60..060b182ca8e49d 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 @@ -68,34 +68,26 @@ import com.google.devtools.build.lib.packages.NativeProvider; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.rules.apple.AppleToolchain; +import com.google.devtools.build.lib.rules.cpp.CppConfiguration; import com.google.devtools.build.lib.rules.cpp.CcCompilationInfo; import com.google.devtools.build.lib.rules.cpp.CcLinkParams; import com.google.devtools.build.lib.rules.cpp.CcLinkParamsInfo; - -import com.google.devtools.build.lib.rules.cpp.CppFileTypes; -import com.google.devtools.build.lib.rules.cpp.CppModuleMap; -import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; - -import com.google.devtools.build.lib.rules.cpp.CppConfiguration; import com.google.devtools.build.lib.rules.cpp.CppFileTypes; import com.google.devtools.build.lib.rules.cpp.CppModuleMap; import com.google.devtools.build.lib.rules.cpp.HeaderMapAction; import com.google.devtools.build.lib.rules.cpp.HeaderMapInfo; import com.google.devtools.build.lib.rules.cpp.HeaderMapInfoProvider; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; import com.google.devtools.build.lib.syntax.Type; - import com.google.devtools.build.lib.util.FileType; import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.HashMap; import java.util.HashSet; import java.util.List; -import java.util.Set; -import java.util.HashMap; import java.util.Map; -/** -import com.google.devtools.build.lib.actions.Root; -*/ +import java.util.Set; /** * Contains information common to multiple objc_* rules, and provides a unified API for extracting @@ -368,16 +360,19 @@ public HeaderMapInfoProvider getHeaderMapInfoProvider(RuleContext ruleContext, I HeaderMapInfo.Builder headerMapInfo = new HeaderMapInfo.Builder(); String includePrefix; if (ruleContext.attributes().has("include_prefix")) { - includePrefix = ruleContext.attributes().get("include_prefix", Type.STRING); + includePrefix = ruleContext.attributes().get("include_prefix", Type.STRING); } else { - includePrefix = ruleContext.getRule().getName(); + includePrefix = ruleContext.getRule().getName(); } headerMapInfo.setIncludePrefix(includePrefix); headerMapInfo.addIncludePrefixdHeaders(hdrs); - // TODO flatten_virtual_headers - headerMapInfo.addHeaders(hdrs); + boolean flattenVirtualHeaders = ruleContext.attributes().has("flatten_virtual_headers") && + ruleContext.attributes().get("flatten_virtual_headers", Type.BOOLEAN); + if (flattenVirtualHeaders) { + headerMapInfo.addHeaders(hdrs); + } if (ruleContext.attributes().has("deps")){ // Propagate all of the dep sources 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 c6ae8b17b1a217..217316d18d0822 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 @@ -704,13 +704,16 @@ Enables clang module support (via -fmodules). */ .add(attr("module_map", LABEL).allowedFileTypes(FileType.of(".modulemap"))) - /* + /* */ - .add(attr("include_prefix", Type.STRING)) + .add(attr("include_prefix", STRING)) - /* + /* + When flatten virtual headers is set, we add all headers to the headermap. + + This allows a user to include a Header, "path/to/header" as "header.h" + + Together with include_prefix, this allows bazel to support Xcode style includes. */ .add(attr("flatten_virtual_headers", BOOLEAN)) diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java index aba9d0362addb4..7285d17141c70f 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java @@ -1820,7 +1820,7 @@ public void testObjcNamespacedImports() throws Exception { ")", "objc_library(", " name = 'dep_libb',", - " header_namespace = 'some',", + " include_prefix = 'some',", " hdrs = ['headerb.h'],", ")");