Skip to content

Commit

Permalink
Add --incompatible_objc_provider_remove_compile_info flag
Browse files Browse the repository at this point in the history
When set to true, the flag turns off the compile info/merge_zip
Starlark APIs in ObjcProvider.

RELNOTES: --incompatible_objc_provider_remove_compile_info turns off
the compile info/mege_zip Starlark APIs in ObjcProvider.  See #11359.
PiperOrigin-RevId: 316568134
  • Loading branch information
googlewalt authored and copybara-github committed Jun 15, 2020
1 parent afec50e commit 38aee31
Show file tree
Hide file tree
Showing 8 changed files with 319 additions and 100 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,18 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
+ "instead of linkopts to cc_toolchain_config")
public boolean incompatibleLinkoptsToLinkLibs;

@Option(
name = "incompatible_objc_provider_remove_compile_info",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
help = "If set to true, the ObjcProvider's APIs for compile info/merge_zip will be removed.")
public boolean incompatibleObjcProviderRemoveCompileInfo;

@Option(
name = "max_computation_steps",
defaultValue = "0",
Expand Down Expand Up @@ -697,6 +709,7 @@ public StarlarkSemantics toStarlarkSemantics() {
.incompatibleRequireLinkerInputCcApi(incompatibleRequireLinkerInputCcApi)
.incompatibleRestrictStringEscapes(incompatibleRestrictStringEscapes)
.incompatibleLinkoptsToLinkLibs(incompatibleLinkoptsToLinkLibs)
.incompatibleObjcProviderRemoveCompileInfo(incompatibleObjcProviderRemoveCompileInfo)
.maxComputationSteps(maxComputationSteps)
.recordRuleInstantiationCallstack(recordRuleInstantiationCallstack)
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import com.google.devtools.build.lib.syntax.Location;
import com.google.devtools.build.lib.syntax.Sequence;
import com.google.devtools.build.lib.syntax.Starlark;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
import com.google.devtools.build.lib.syntax.StarlarkValue;
import java.util.Map;
Expand All @@ -62,6 +63,15 @@ public class AppleStarlarkCommon
XcodeConfigInfo,
ApplePlatform> {

@VisibleForTesting
public static final String DEPRECATED_KEY_ERROR =
"Key '%s' no longer supported in ObjcProvider (use CcInfo instead).";

@VisibleForTesting
public static final String BAD_DIRECT_DEP_PROVIDERS_ERROR =
"Argument 'direct_dep_providers' no longer supported. Please use 'strict_include' field "
+ "in ObjcProvider.";

@VisibleForTesting
public static final String BAD_KEY_ERROR =
"Argument %s not a recognized key,"
Expand Down Expand Up @@ -188,8 +198,8 @@ public SplitTransitionProviderApi getMultiArchSplitProvider() {
// This method is registered statically for Starlark, and never called directly.
public ObjcProvider newObjcProvider(Boolean usesSwift, Dict<?, ?> kwargs, StarlarkThread thread)
throws EvalException {
ObjcProvider.StarlarkBuilder resultBuilder =
new ObjcProvider.StarlarkBuilder(thread.getSemantics());
StarlarkSemantics semantics = thread.getSemantics();
ObjcProvider.StarlarkBuilder resultBuilder = new ObjcProvider.StarlarkBuilder(semantics);
if (usesSwift) {
resultBuilder.add(ObjcProvider.FLAG, ObjcProvider.Flag.USES_SWIFT);
}
Expand All @@ -202,6 +212,9 @@ public ObjcProvider newObjcProvider(Boolean usesSwift, Dict<?, ?> kwargs, Starla
} else if (entry.getKey().equals("providers")) {
resultBuilder.addProvidersFromStarlark(entry.getValue());
} else if (entry.getKey().equals("direct_dep_providers")) {
if (semantics.incompatibleObjcProviderRemoveCompileInfo()) {
throw new EvalException(null, BAD_DIRECT_DEP_PROVIDERS_ERROR);
}
resultBuilder.addDirectDepProvidersFromStarlark(entry.getValue());
} else {
throw new EvalException(null, String.format(BAD_KEY_ERROR, entry.getKey()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,10 +318,13 @@ public enum Flag {

private final CcCompilationContext ccCompilationContext;

/** Keys corresponding to compile information that has been migrated to CcCompilationContext. */
static final ImmutableSet<Key<?>> KEYS_FOR_COMPILE_INFO =
/**
* Keys that are deprecated and will be removed. These include compile information that has been
* migrated to CcCompilationContext, plus MERGE_ZIP.
*/
static final ImmutableSet<Key<?>> DEPRECATED_KEYS =
ImmutableSet.<Key<?>>of(
DEFINE, FRAMEWORK_SEARCH_PATHS, HEADER, INCLUDE, INCLUDE_SYSTEM, IQUOTE);
DEFINE, FRAMEWORK_SEARCH_PATHS, HEADER, INCLUDE, INCLUDE_SYSTEM, IQUOTE, MERGE_ZIP);

/** All keys in ObjcProvider that will be passed in the corresponding Starlark provider. */
static final ImmutableList<Key<?>> KEYS_FOR_STARLARK =
Expand Down Expand Up @@ -990,6 +993,10 @@ public Builder(StarlarkSemantics semantics) {
this.starlarkSemantics = semantics;
}

public StarlarkSemantics getStarlarkSemantics() {
return starlarkSemantics;
}

private static void maybeAddEmptyBuilder(Map<Key<?>, NestedSetBuilder<?>> set, Key<?> key) {
set.computeIfAbsent(key, k -> new NestedSetBuilder<>(k.order));
}
Expand Down Expand Up @@ -1185,53 +1192,17 @@ public StarlarkBuilder(StarlarkSemantics semantics) {
*/
void addElementsFromStarlark(Key<?> key, Object starlarkToAdd) throws EvalException {
NestedSet<?> toAdd = ObjcProviderStarlarkConverters.convertToJava(key, starlarkToAdd);
if (KEYS_FOR_COMPILE_INFO.contains(key)) {
String keyName = key.getStarlarkKeyName();

if (key == DEFINE) {
ccCompilationContextBuilder.addDefines(
Depset.noneableCast(starlarkToAdd, String.class, keyName));
} else if (key == FRAMEWORK_SEARCH_PATHS) {
// Due to legacy reasons, There is a mismatch between the starlark interface for the
// framework search path, and the internal representation. The interface specifies that
// framework_search_paths include the framework directories, but internally we only store
// their parents. We will eventually clean up the interface, but for now we need to do
// this ugly conversion.

ImmutableList<PathFragment> frameworks =
Depset.noneableCast(starlarkToAdd, String.class, keyName).toList().stream()
.map(x -> PathFragment.create(x))
.collect(ImmutableList.toImmutableList());

ImmutableList.Builder<PathFragment> frameworkSearchPaths = ImmutableList.builder();
for (PathFragment framework : frameworks) {
if (!framework.getSafePathString().endsWith(FRAMEWORK_SUFFIX)) {
throw new EvalException(
null, String.format(AppleStarlarkCommon.BAD_FRAMEWORK_PATH_ERROR, framework));
}
frameworkSearchPaths.add(framework.getParentDirectory());
if (DEPRECATED_KEYS.contains(key)) {
if (getStarlarkSemantics().incompatibleObjcProviderRemoveCompileInfo()) {
if (!KEYS_FOR_DIRECT.contains(key)) {
throw new EvalException(
null,
String.format(AppleStarlarkCommon.DEPRECATED_KEY_ERROR, key.getStarlarkKeyName()));
}
} else {
if (!addCompileElementsFromStarlark(key, starlarkToAdd)) {
uncheckedAddTransitive(key, toAdd);
}
ccCompilationContextBuilder.addFrameworkIncludeDirs(frameworkSearchPaths.build());
} else if (key == HEADER) {
ImmutableList<Artifact> hdrs =
Depset.noneableCast(starlarkToAdd, Artifact.class, keyName).toList();
ccCompilationContextBuilder.addDeclaredIncludeSrcs(hdrs);
ccCompilationContextBuilder.addTextualHdrs(hdrs);
} else if (key == INCLUDE) {
ccCompilationContextBuilder.addIncludeDirs(
Depset.noneableCast(starlarkToAdd, String.class, keyName).toList().stream()
.map(x -> PathFragment.create(x))
.collect(ImmutableList.toImmutableList()));
} else if (key == INCLUDE_SYSTEM) {
ccCompilationContextBuilder.addSystemIncludeDirs(
Depset.noneableCast(starlarkToAdd, String.class, keyName).toList().stream()
.map(x -> PathFragment.create(x))
.collect(ImmutableList.toImmutableList()));
} else if (key == IQUOTE) {
ccCompilationContextBuilder.addQuoteIncludeDirs(
Depset.noneableCast(starlarkToAdd, String.class, keyName).toList().stream()
.map(x -> PathFragment.create(x))
.collect(ImmutableList.toImmutableList()));
}
} else {
uncheckedAddTransitive(key, toAdd);
Expand All @@ -1242,6 +1213,64 @@ void addElementsFromStarlark(Key<?> key, Object starlarkToAdd) throws EvalExcept
}
}

private boolean addCompileElementsFromStarlark(Key<?> key, Object starlarkToAdd)
throws EvalException {
String keyName = key.getStarlarkKeyName();

if (key == DEFINE) {
ccCompilationContextBuilder.addDefines(
Depset.noneableCast(starlarkToAdd, String.class, keyName));
return true;
} else if (key == FRAMEWORK_SEARCH_PATHS) {
// Due to legacy reasons, There is a mismatch between the starlark interface for the
// framework search path, and the internal representation. The interface specifies that
// framework_search_paths include the framework directories, but internally we only store
// their parents. We will eventually clean up the interface, but for now we need to do
// this ugly conversion.

ImmutableList<PathFragment> frameworks =
Depset.noneableCast(starlarkToAdd, String.class, keyName).toList().stream()
.map(PathFragment::create)
.collect(ImmutableList.toImmutableList());

ImmutableList.Builder<PathFragment> frameworkSearchPaths = ImmutableList.builder();
for (PathFragment framework : frameworks) {
if (!framework.getSafePathString().endsWith(FRAMEWORK_SUFFIX)) {
throw new EvalException(
null, String.format(AppleStarlarkCommon.BAD_FRAMEWORK_PATH_ERROR, framework));
}
frameworkSearchPaths.add(framework.getParentDirectory());
}
ccCompilationContextBuilder.addFrameworkIncludeDirs(frameworkSearchPaths.build());
return true;
} else if (key == HEADER) {
ImmutableList<Artifact> hdrs =
Depset.noneableCast(starlarkToAdd, Artifact.class, keyName).toList();
ccCompilationContextBuilder.addDeclaredIncludeSrcs(hdrs);
ccCompilationContextBuilder.addTextualHdrs(hdrs);
return true;
} else if (key == INCLUDE) {
ccCompilationContextBuilder.addIncludeDirs(
Depset.noneableCast(starlarkToAdd, String.class, keyName).toList().stream()
.map(PathFragment::create)
.collect(ImmutableList.toImmutableList()));
return true;
} else if (key == INCLUDE_SYSTEM) {
ccCompilationContextBuilder.addSystemIncludeDirs(
Depset.noneableCast(starlarkToAdd, String.class, keyName).toList().stream()
.map(PathFragment::create)
.collect(ImmutableList.toImmutableList()));
return true;
} else if (key == IQUOTE) {
ccCompilationContextBuilder.addQuoteIncludeDirs(
Depset.noneableCast(starlarkToAdd, String.class, keyName).toList().stream()
.map(PathFragment::create)
.collect(ImmutableList.toImmutableList()));
return true;
}
return false;
}

/**
* Adds the given providers from Starlark. An error is thrown if toAdd is not an iterable of
* ObjcProvider instances.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.devtools.build.lib.skylarkbuildapi.FileApi;
import com.google.devtools.build.lib.skylarkbuildapi.cpp.CcCompilationContextApi;
import com.google.devtools.build.lib.syntax.Sequence;
import com.google.devtools.build.lib.syntax.StarlarkSemantics.FlagIdentifier;
import com.google.devtools.build.lib.syntax.StarlarkValue;
import net.starlark.java.annot.StarlarkBuiltin;
import net.starlark.java.annot.StarlarkDocumentationCategory;
Expand All @@ -38,7 +39,8 @@ public interface ObjcProviderApi<FileApiT extends FileApi> extends StarlarkValue
structField = true,
doc =
"A set of strings from 'defines' attributes. These are to be passed as '-D' flags to "
+ "all invocations of the compiler for this target and all depending targets.")
+ "all invocations of the compiler for this target and all depending targets.",
disableWithFlag = FlagIdentifier.INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO)
Depset /*<String>*/ defineForStarlark();

@StarlarkMethod(
Expand All @@ -60,7 +62,8 @@ public interface ObjcProviderApi<FileApiT extends FileApi> extends StarlarkValue
structField = true,
doc =
"Exec paths of .framework directories corresponding to frameworks to include "
+ "in search paths, but not to link.")
+ "in search paths, but not to link.",
disableWithFlag = FlagIdentifier.INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO)
Depset /*<String>*/ frameworkIncludeForStarlark();

@StarlarkMethod(
Expand All @@ -72,7 +75,8 @@ public interface ObjcProviderApi<FileApiT extends FileApi> extends StarlarkValue
@StarlarkMethod(
name = "header",
structField = true,
doc = "All header files. These may be either public or private headers.")
doc = "All header files. These may be either public or private headers.",
disableWithFlag = FlagIdentifier.INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO)
Depset /*<FileApiT>*/ headerForStarlark();

@StarlarkMethod(
Expand All @@ -94,7 +98,8 @@ public interface ObjcProviderApi<FileApiT extends FileApi> extends StarlarkValue
structField = true,
doc =
"Include search paths specified with '-I' on the command line. Also known as "
+ "header search paths (and distinct from <em>user</em> header search paths).")
+ "header search paths (and distinct from <em>user</em> header search paths).",
disableWithFlag = FlagIdentifier.INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO)
Depset includeForStarlark();

@StarlarkMethod(
Expand All @@ -108,13 +113,15 @@ public interface ObjcProviderApi<FileApiT extends FileApi> extends StarlarkValue
@StarlarkMethod(
name = "include_system",
structField = true,
doc = "System include search paths (typically specified with -isystem).")
doc = "System include search paths (typically specified with -isystem).",
disableWithFlag = FlagIdentifier.INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO)
Depset systemIncludeForStarlark();

@StarlarkMethod(
name = "iquote",
structField = true,
doc = "User header search paths (typically specified with -iquote).")
doc = "User header search paths (typically specified with -iquote).",
disableWithFlag = FlagIdentifier.INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO)
Depset quoteIncludeForStarlark();

@StarlarkMethod(
Expand Down Expand Up @@ -168,7 +175,8 @@ public interface ObjcProviderApi<FileApiT extends FileApi> extends StarlarkValue
doc =
"Merge zips to include in the bundle. The entries of these zip files are included "
+ "in the final bundle with the same path. The entries in the merge zips should not "
+ "include the bundle root path (e.g. 'Foo.app').")
+ "include the bundle root path (e.g. 'Foo.app').",
disableWithFlag = FlagIdentifier.INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO)
Depset /*<FileApiT>*/ mergeZip();

@StarlarkMethod(
Expand Down Expand Up @@ -276,6 +284,7 @@ public interface ObjcProviderApi<FileApiT extends FileApi> extends StarlarkValue
doc =
"Returns the embedded <code>CcCompilationContext</code> that contains the"
+ "provider's compilation information.",
structField = true)
structField = true,
disableWithFlag = FlagIdentifier.INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO)
CcCompilationContextApi<FileApiT> getCcCompilationContext();
}
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ private FlagIdentifier() {} // uninstantiable
public static final String INCOMPATIBLE_NO_ATTR_LICENSE = "incompatible_no_attr_license";
public static final String INCOMPATIBLE_ALLOW_TAGS_PROPAGATION =
"incompatible_allow_tags_propagation";
public static final String INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO =
"incompatible_objc_provider_remove_compile_info";
public static final String INCOMPATIBLE_REQUIRE_LINKER_INPUT_CC_API =
"incompatible_require_linker_input_cc_api";
public static final String INCOMPATIBLE_LINKOPTS_TO_LINKLIBS =
Expand Down Expand Up @@ -147,6 +149,8 @@ public boolean flagValue(String flag) {
return incompatibleNoAttrLicense();
case FlagIdentifier.INCOMPATIBLE_ALLOW_TAGS_PROPAGATION:
return experimentalAllowTagsPropagation();
case FlagIdentifier.INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO:
return incompatibleObjcProviderRemoveCompileInfo();
case FlagIdentifier.INCOMPATIBLE_REQUIRE_LINKER_INPUT_CC_API:
return incompatibleRequireLinkerInputCcApi();
case FlagIdentifier.INCOMPATIBLE_LINKOPTS_TO_LINKLIBS:
Expand Down Expand Up @@ -264,6 +268,8 @@ boolean isFeatureEnabledBasedOnTogglingFlags(String enablingFlag, String disabli

public abstract boolean incompatibleDepsetForLibrariesToLinkGetter();

public abstract boolean incompatibleObjcProviderRemoveCompileInfo();

public abstract boolean incompatibleRequireLinkerInputCcApi();

public abstract boolean incompatibleRestrictStringEscapes();
Expand Down Expand Up @@ -347,6 +353,7 @@ public static Builder builderWithDefaults() {
.internalStarlarkFlagTestCanary(false)
.incompatibleDoNotSplitLinkingCmdline(true)
.incompatibleDepsetForLibrariesToLinkGetter(true)
.incompatibleObjcProviderRemoveCompileInfo(false)
.incompatibleRequireLinkerInputCcApi(false)
.incompatibleRestrictStringEscapes(false)
.incompatibleUseCcConfigureFromRulesCc(false)
Expand Down Expand Up @@ -432,6 +439,8 @@ public abstract static class Builder {

public abstract Builder incompatibleDepsetForLibrariesToLinkGetter(boolean value);

public abstract Builder incompatibleObjcProviderRemoveCompileInfo(boolean value);

public abstract Builder incompatibleRequireLinkerInputCcApi(boolean value);

public abstract Builder incompatibleRestrictStringEscapes(boolean value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E
"--incompatible_no_implicit_file_export=" + rand.nextBoolean(),
"--incompatible_no_rule_outputs_param=" + rand.nextBoolean(),
"--incompatible_no_support_tools_in_action_inputs=" + rand.nextBoolean(),
"--incompatible_objc_provider_remove_compile_info=" + rand.nextBoolean(),
"--incompatible_run_shell_command_string=" + rand.nextBoolean(),
"--incompatible_string_replace_count=" + rand.nextBoolean(),
"--incompatible_visibility_private_attributes_at_definition=" + rand.nextBoolean(),
Expand Down Expand Up @@ -207,6 +208,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
.incompatibleNoImplicitFileExport(rand.nextBoolean())
.incompatibleNoRuleOutputsParam(rand.nextBoolean())
.incompatibleNoSupportToolsInActionInputs(rand.nextBoolean())
.incompatibleObjcProviderRemoveCompileInfo(rand.nextBoolean())
.incompatibleRunShellCommandString(rand.nextBoolean())
.incompatibleStringReplaceCount(rand.nextBoolean())
.incompatibleVisibilityPrivateAttributesAtDefinition(rand.nextBoolean())
Expand Down
Loading

0 comments on commit 38aee31

Please sign in to comment.