From 2270c990938b170cecee96ef5985921771676bfd Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 29 Jul 2019 13:22:41 -0400 Subject: [PATCH] Project import generated by Copybara. PiperOrigin-RevId: 260527280 --- src/BUILD | 1 - .../templates/attributes/common/tags.html | 5 +- .../java/com/google/devtools/build/lib/BUILD | 1 - .../build/lib/actions/ActionCacheChecker.java | 5 - .../lib/actions/ArtifactFileMetadata.java | 202 ++++++++++++++++ .../build/lib/actions/FileArtifactValue.java | 211 +++++------------ .../build/lib/actionsketch/ActionSketch.java | 3 +- .../devtools/build/lib/actionsketch/BUILD | 21 -- .../lib/analysis/BashCommandConstructor.java | 50 ---- .../lib/analysis/CommandConstructor.java | 37 --- .../build/lib/analysis/CommandHelper.java | 80 +++++-- .../AbstractConfiguredTarget.java | 50 +--- .../lib/analysis/extra/ExtraActionSpec.java | 12 +- .../lib/analysis/platform/PlatformInfo.java | 38 +-- .../lib/analysis/platform/PlatformUtils.java | 14 +- .../skylark/SkylarkActionFactory.java | 9 +- .../analysis/skylark/SkylarkRuleContext.java | 13 +- .../analysis/test/TestTargetProperties.java | 5 +- .../build/lib/buildtool/ExecutionTool.java | 1 - .../build/lib/buildtool/SkyframeBuilder.java | 6 - .../build/lib/packages/SkylarkInfo.java | 8 - .../packages/StarlarkSemanticsOptions.java | 18 -- .../build/lib/remote/GrpcRemoteCache.java | 14 +- .../build/lib/rules/genrule/GenRuleBase.java | 14 +- .../build/lib/rules/platform/Platform.java | 10 - .../lib/rules/platform/PlatformRule.java | 16 -- .../build/lib/rules/python/PyCommon.java | 44 ++++ .../lib/rules/python/PythonConfiguration.java | 16 ++ .../python/PythonConfigurationLoader.java | 1 + .../build/lib/rules/python/PythonOptions.java | 16 ++ .../build/lib/runtime/BlazeModule.java | 12 - .../build/lib/runtime/CommandEnvironment.java | 21 -- .../lib/skyframe/ActionExecutionFunction.java | 18 -- .../lib/skyframe/ActionExecutionValue.java | 33 +-- .../lib/skyframe/ActionMetadataHandler.java | 99 ++++---- .../lib/skyframe/ActionSketchFunction.java | 142 ------------ .../build/lib/skyframe/ArtifactFunction.java | 12 +- .../lib/skyframe/FilesystemValueChecker.java | 24 +- .../lib/skyframe/MinimalOutputStore.java | 3 +- .../build/lib/skyframe/OutputStore.java | 26 ++- .../build/lib/skyframe/SkyFunctions.java | 1 - .../lib/skyframe/SkyframeActionExecutor.java | 8 - .../build/lib/skyframe/SkyframeExecutor.java | 16 +- .../lib/skyframe/TopDownActionCache.java | 33 --- .../platform/PlatformInfoApi.java | 8 - .../build/lib/syntax/DotExpression.java | 2 +- .../build/lib/syntax/SkylarkClassObject.java | 16 -- .../build/lib/syntax/StarlarkSemantics.java | 5 - .../devtools/build/skydoc/SkydocMain.java | 86 ++++++- .../devtools/build/skydoc/SkydocOptions.java | 2 +- .../build/lib/actions/ActionInputMapTest.java | 15 -- .../analysis/platform/PlatformUtilsTest.java | 32 +-- .../SkylarkSemanticsConsistencyTest.java | 2 - .../packages/util/BazelMockPythonSupport.java | 3 + .../lib/packages/util/MockPythonSupport.java | 59 +++++ .../build/lib/remote/GrpcRemoteCacheTest.java | 107 +++++++++ .../rules/platform/PlatformInfoApiTest.java | 94 -------- .../lib/rules/platform/PlatformTestCase.java | 15 -- .../devtools/build/lib/rules/python/BUILD | 1 + .../PyBaseConfiguredTargetTestBase.java | 43 ++++ .../rules/test/TestTargetPropertiesTest.java | 20 -- .../lib/skyframe/ArtifactFunctionTest.java | 18 +- .../google/devtools/build/lib/skyframe/BUILD | 1 - .../skyframe/FilesystemValueCheckerTest.java | 15 +- .../skyframe/TimestampBuilderTestCase.java | 12 +- .../lib/skyframe/TopDownActionCacheTest.java | 216 ------------------ .../skyframe/TreeArtifactMetadataTest.java | 10 +- .../lib/skylark/SkylarkIntegrationTest.java | 92 -------- .../bazel/bazel_embedded_skylark_test.sh | 12 +- src/test/shell/bazel/platforms_test.sh | 37 --- .../bazel/remote/remote_execution_test.sh | 28 --- .../integration/modify_execution_info_test.sh | 8 + tools/python/private/defs.bzl | 20 +- 73 files changed, 906 insertions(+), 1442 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/actions/ArtifactFileMetadata.java delete mode 100644 src/main/java/com/google/devtools/build/lib/actionsketch/BUILD delete mode 100644 src/main/java/com/google/devtools/build/lib/analysis/BashCommandConstructor.java delete mode 100644 src/main/java/com/google/devtools/build/lib/analysis/CommandConstructor.java delete mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/ActionSketchFunction.java delete mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/TopDownActionCache.java delete mode 100644 src/test/java/com/google/devtools/build/lib/skyframe/TopDownActionCacheTest.java diff --git a/src/BUILD b/src/BUILD index d9d5184fff0e34..b8f1a7d38ad893 100644 --- a/src/BUILD +++ b/src/BUILD @@ -430,7 +430,6 @@ filegroup( "//src/java_tools/singlejar:srcs", "//src/main/cpp:srcs", "//src/main/java/com/google/devtools/build/docgen:srcs", - "//src/main/java/com/google/devtools/build/lib/actionsketch:srcs", "//src/main/java/com/google/devtools/build/lib:srcs", "//src/main/java/com/google/devtools/build/lib/includescanning:srcs", "//src/main/java/com/google/devtools/build/lib/network:srcs", diff --git a/src/main/java/com/google/devtools/build/docgen/templates/attributes/common/tags.html b/src/main/java/com/google/devtools/build/docgen/templates/attributes/common/tags.html index 2af4ae112c8f19..4011a0f6969fb6 100644 --- a/src/main/java/com/google/devtools/build/docgen/templates/attributes/common/tags.html +++ b/src/main/java/com/google/devtools/build/docgen/templates/attributes/common/tags.html @@ -88,9 +88,8 @@
  • exclusive keyword will force the test to be run in the "exclusive" mode, ensuring that no other tests are running at the same time. Such tests will be executed in serial fashion after all build - activity and non-exclusive tests have been completed. Remote execution is - disabled for such tests because Bazel doesn't have control over what's - running on a remote machine. + activity and non-exclusive tests have been completed. They will also always + run locally and thus without sandboxing.
  • manual keyword will force the test target to not be included in target pattern diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index 3a0101855e03f9..625ef89750a019 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -679,7 +679,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", - "//src/main/java/com/google/devtools/build/lib/actionsketch:action_sketch", "//src/main/java/com/google/devtools/build/lib/analysis/platform", "//src/main/java/com/google/devtools/build/lib/analysis/platform:platform_utils", "//src/main/java/com/google/devtools/build/lib/analysis/platform:utils", diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index 5c204bd67bfcc0..4abf6684015b3f 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -552,11 +552,6 @@ public byte[] getDigest() { return EMPTY_DIGEST; } - @Override - public FileContentsProxy getContentsProxy() { - throw new UnsupportedOperationException(); - } - @Override public long getSize() { return 0; diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactFileMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFileMetadata.java new file mode 100644 index 00000000000000..7756ae26def09b --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactFileMetadata.java @@ -0,0 +1,202 @@ +// Copyright 2014 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.actions; + +import com.google.common.base.Preconditions; +import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; +import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; +import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; +import com.google.devtools.build.lib.util.BigIntegerFingerprint; +import com.google.devtools.build.lib.vfs.PathFragment; +import java.math.BigInteger; +import java.util.Objects; +import javax.annotation.Nullable; + +/** + * A value that represents a file for the purposes of up-to-dateness checks of actions. + * + *

    It always stands for an actual file. In particular, tree artifacts and middlemen do not have a + * corresponding {@link ArtifactFileMetadata}. However, the file is not necessarily present in the + * file system; this happens when intermediate build outputs are not downloaded (and maybe when an + * input artifact of an action is missing?) + * + *

    It makes its main appearance in {@code ActionExecutionValue.artifactData}. It has two main + * uses: + * + *

    + * + *

    It would probably be possible to unify this and {@link FileArtifactValue} since they contain + * much the same data. However, {@link FileArtifactValue} has a few other uses that are do not map + * easily to {@link ArtifactFileMetadata}, mostly relating to ActionFS. + */ +@Immutable +@ThreadSafe +public abstract class ArtifactFileMetadata { + /** + * Used as as placeholder in {@code OutputStore.artifactData} for artifacts that have entries in + * {@code OutputStore.additionalOutputData}. + */ + @AutoCodec public static final ArtifactFileMetadata PLACEHOLDER = new PlaceholderFileValue(); + + // No implementations outside this class. + private ArtifactFileMetadata() {} + + public boolean exists() { + return realFileStateValue().getType() != FileStateType.NONEXISTENT; + } + + /** + * Returns true if this value corresponds to a file or symlink to an existing regular or special + * file. If so, its parent directory is guaranteed to exist. + */ + public boolean isFile() { + return realFileStateValue().getType() == FileStateType.REGULAR_FILE; + } + + /** + * Returns true if the file is a directory or a symlink to an existing directory. If so, its + * parent directory is guaranteed to exist. + */ + public boolean isDirectory() { + return realFileStateValue().getType() == FileStateType.DIRECTORY; + } + + protected abstract FileStateValue realFileStateValue(); + + public FileContentsProxy getContentsProxy() { + return realFileStateValue().getContentsProxy(); + } + + public long getSize() { + Preconditions.checkState(isFile(), this); + return realFileStateValue().getSize(); + } + + @Nullable + public byte[] getDigest() { + Preconditions.checkState(isFile(), this); + return realFileStateValue().getDigest(); + } + + /** Returns a quick fingerprint via a BigInteger */ + public BigInteger getFingerprint() { + BigIntegerFingerprint fp = new BigIntegerFingerprint(); + + // TODO(lberki): This could be replaced with addLong(getType().ordinal()) + // at the cost of making the order of elements in the enum affecting the fingerprint. + fp.addBoolean(realFileStateValue().getType() == FileStateType.NONEXISTENT); + fp.addBoolean(realFileStateValue().getType() == FileStateType.SPECIAL_FILE); + fp.addBoolean(realFileStateValue().getType() == FileStateType.DIRECTORY); + fp.addBoolean(realFileStateValue().getType() == FileStateType.REGULAR_FILE); + + if (isFile()) { + fp.addLong(getSize()); + fp.addDigestedBytes(getDigest()); + } + return fp.getFingerprint(); + } + + public static ArtifactFileMetadata forRegularFile( + PathFragment pathFragment, FileStateValue fileStateValue) { + return new Regular(pathFragment, fileStateValue); + } + + /** Non-stub implementation of {@link ArtifactFileMetadata}. */ + private static final class Regular extends ArtifactFileMetadata { + private final PathFragment realPath; + private final FileStateValue realFileStateValue; + + Regular(PathFragment realPath, FileStateValue realFileStateValue) { + this.realPath = Preconditions.checkNotNull(realPath); + this.realFileStateValue = Preconditions.checkNotNull(realFileStateValue); + } + + @Override + public FileStateValue realFileStateValue() { + return realFileStateValue; + } + + @Override + public FileContentsProxy getContentsProxy() { + return realFileStateValue.getContentsProxy(); + } + + @Override + public boolean equals(Object obj) { + if (obj == null) { + return false; + } + if (obj.getClass() != Regular.class) { + return false; + } + Regular other = (Regular) obj; + return realPath.equals(other.realPath) && realFileStateValue.equals(other.realFileStateValue); + } + + @Override + public int hashCode() { + return Objects.hash(realPath, realFileStateValue); + } + + @Override + public String toString() { + return realPath + ", " + realFileStateValue; + } + + @Override + public BigInteger getFingerprint() { + BigInteger original = super.getFingerprint(); + BigIntegerFingerprint fp = new BigIntegerFingerprint(); + fp.addBigIntegerOrdered(original); + fp.addString(getClass().getCanonicalName()); + fp.addPath(realPath); + fp.addBigIntegerOrdered(realFileStateValue.getValueFingerprint()); + return fp.getFingerprint(); + } + } + + private static final class PlaceholderFileValue extends ArtifactFileMetadata { + private static final BigInteger FINGERPRINT = + new BigIntegerFingerprint().addString("PlaceholderFileValue").getFingerprint(); + + private PlaceholderFileValue() {} + + @Override + public FileStateValue realFileStateValue() { + throw new UnsupportedOperationException(); + } + + @Override + public FileContentsProxy getContentsProxy() { + throw new UnsupportedOperationException(); + } + + @Override + public BigInteger getFingerprint() { + return FINGERPRINT; + } + + @Override + public String toString() { + return "PlaceholderFileValue:Singleton"; + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index d7cbc284b2f62d..6a7899810f18f8 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java @@ -36,32 +36,44 @@ import javax.annotation.Nullable; /** - * A value that represents a file for the purposes of up-to-dateness checks of actions. + * State of a file system object for the execution phase. * - *

    It always stands for an actual file. In particular, tree artifacts and middlemen do not have a - * corresponding {@link FileArtifactValue}. However, the file is not necessarily present in the file - * system; this happens when intermediate build outputs are not downloaded (and maybe when an input - * artifact of an action is missing?) + *

    This is not used by Skyframe for invalidation, it is primarily used by the action cache and + * the various {@link com.google.devtools.build.lib.exec.SpawnRunner} implementations. * - *

    It makes its main appearance in {@code ActionExecutionValue.artifactData}. It has two main - * uses: + *

    We have the following cases: * *

    */ @Immutable @ThreadSafe public abstract class FileArtifactValue implements SkyValue, HasDigest { + @AutoCodec public static final FileArtifactValue DEFAULT_MIDDLEMAN = new SingletonMarkerValue(); + /** Data that marks that a file is not present on the filesystem. */ + @AutoCodec public static final FileArtifactValue MISSING_FILE_MARKER = new SingletonMarkerValue(); + + /** + * Represents an omitted file -- we are aware of it but it doesn't exist. All access methods are + * unsupported. + */ + @AutoCodec public static final FileArtifactValue OMITTED_FILE_MARKER = new OmittedFileValue(); /** - * Used as as placeholder in {@code OutputStore.artifactData} for artifacts that have entries in - * {@code OutputStore.additionalOutputData}. + * Marker interface for singleton implementations of this class. + * + *

    Needed for a correct implementation of {@code equals}. */ - @AutoCodec public static final FileArtifactValue PLACEHOLDER = new PlaceholderFileValue(); + public interface Singleton {} /** * The type of the underlying file system object. If it is a regular file, then it is guaranteed @@ -80,7 +92,7 @@ public abstract class FileArtifactValue implements SkyValue, HasDigest { * *

    The return value is owned by this object and must not be modified. */ - @Override + @Nullable public abstract byte[] getDigest(); /** Returns the file's size, or 0 if the underlying file system object is not a file. */ @@ -104,7 +116,6 @@ public BigInteger getValueFingerprint() { return null; } - /** * Index used to resolve remote files. * @@ -116,7 +127,7 @@ public int getLocationIndex() { } /** Returns {@code true} if this is a special marker as opposed to a representing a real file. */ - public boolean isMarkerValue() { + public final boolean isMarkerValue() { return this instanceof Singleton; } @@ -169,24 +180,6 @@ public int hashCode() { } } - public abstract FileContentsProxy getContentsProxy(); - - /** - * Marker interface for singleton implementations of this class. - * - *

    Needed for a correct implementation of {@code equals}. - */ - interface Singleton {} - - @AutoCodec public static final FileArtifactValue DEFAULT_MIDDLEMAN = new SingletonMarkerValue(); - /** Data that marks that a file is not present on the filesystem. */ - @AutoCodec public static final FileArtifactValue MISSING_FILE_MARKER = new SingletonMarkerValue(); - /** - * Represents an omitted file -- we are aware of it but it doesn't exist. All access methods are - * unsupported. - */ - @AutoCodec public static final FileArtifactValue OMITTED_FILE_MARKER = new OmittedFileValue(); - public static FileArtifactValue createForSourceArtifact(Artifact artifact, FileValue fileValue) throws IOException { Preconditions.checkState(artifact.isSourceArtifact()); @@ -202,21 +195,20 @@ public static FileArtifactValue createForSourceArtifact(Artifact artifact, FileV } @VisibleForTesting - public static FileArtifactValue injectDigestForTesting( - Artifact artifact, FileArtifactValue source) throws IOException { - boolean isFile = source.getType() == FileStateType.REGULAR_FILE; - FileContentsProxy proxy = source.getContentsProxy(); + public static FileArtifactValue createForTesting(Artifact artifact, ArtifactFileMetadata metadata) + throws IOException { + boolean isFile = metadata.isFile(); return create( artifact.getPath(), isFile, - isFile ? source.getSize() : 0, - proxy, - isFile ? source.getDigest() : null, + isFile ? metadata.getSize() : 0, + isFile ? metadata.getContentsProxy() : null, + isFile ? metadata.getDigest() : null, !artifact.isConstantMetadata()); } public static FileArtifactValue createFromInjectedDigest( - FileArtifactValue metadata, @Nullable byte[] digest, boolean isShareable) { + ArtifactFileMetadata metadata, @Nullable byte[] digest, boolean isShareable) { return createForNormalFile( digest, metadata.getContentsProxy(), metadata.getSize(), isShareable); } @@ -277,6 +269,15 @@ public static FileArtifactValue createForNormalFile( : new UnshareableRegularFileArtifactValue(digest, proxy, size); } + public static FileArtifactValue createFromMetadata( + ArtifactFileMetadata artifactMetadata, boolean isShareable) { + return createForNormalFile( + artifactMetadata.getDigest(), + artifactMetadata.getContentsProxy(), + artifactMetadata.getSize(), + isShareable); + } + public static FileArtifactValue createForDirectoryWithHash(byte[] digest) { return new HashedDirectoryArtifactValue(digest); } @@ -287,17 +288,13 @@ public static FileArtifactValue createForDirectoryWithMtime(long mtime) { /** * Creates a FileArtifactValue used as a 'proxy' input for other ArtifactValues. These are used in - * {@link ActionCacheChecker}. + * {@link com.google.devtools.build.lib.actions.ActionCacheChecker}. */ public static FileArtifactValue createProxy(byte[] digest) { Preconditions.checkNotNull(digest); return createForNormalFile(digest, /*proxy=*/ null, /*size=*/ 0, /*isShareable=*/ true); } - private static String bytesToString(byte[] bytes) { - return "0x" + BaseEncoding.base16().omitPadding().encode(bytes); - } - private static final class DirectoryArtifactValue extends FileArtifactValue { private final long mtime; @@ -316,11 +313,6 @@ public byte[] getDigest() { return null; } - @Override - public FileContentsProxy getContentsProxy() { - throw new UnsupportedOperationException(); - } - @Override public BigInteger getValueFingerprint() { BigIntegerFingerprint fp = new BigIntegerFingerprint(); @@ -368,11 +360,6 @@ public byte[] getDigest() { return digest; } - @Override - public FileContentsProxy getContentsProxy() { - throw new UnsupportedOperationException(); - } - @Override public long getModifiedTime() { return 0; @@ -419,9 +406,8 @@ private static class RegularFileArtifactValue extends FileArtifactValue { @Nullable private final FileContentsProxy proxy; private final long size; - private RegularFileArtifactValue( - @Nullable byte[] digest, @Nullable FileContentsProxy proxy, long size) { - this.digest = digest; + private RegularFileArtifactValue(byte[] digest, @Nullable FileContentsProxy proxy, long size) { + this.digest = Preconditions.checkNotNull(digest); this.proxy = proxy; this.size = size; } @@ -436,11 +422,6 @@ public byte[] getDigest() { return digest; } - @Override - public FileContentsProxy getContentsProxy() { - return proxy; - } - @Override public long getSize() { return size; @@ -466,8 +447,7 @@ public String toString() { return MoreObjects.toStringHelper(this) .add("digest", BaseEncoding.base16().lowerCase().encode(digest)) .add("size", size) - .add("proxy", proxy) - .toString(); + .add("proxy", proxy).toString(); } @Override @@ -488,8 +468,7 @@ public boolean equals(Object o) { @Override public int hashCode() { return (proxy != null ? 127 * proxy.hashCode() : 0) - + 37 * Long.hashCode(getSize()) - + Arrays.hashCode(getDigest()); + + 37 * Long.hashCode(getSize()) + Arrays.hashCode(getDigest()); } } @@ -527,11 +506,6 @@ public byte[] getDigest() { return digest; } - @Override - public FileContentsProxy getContentsProxy() { - throw new UnsupportedOperationException(); - } - @Override public long getSize() { return size; @@ -568,6 +542,10 @@ public String toString() { } } + private static String bytesToString(byte[] bytes) { + return "0x" + BaseEncoding.base16().omitPadding().encode(bytes); + } + /** File stored inline in metadata. */ public static class InlineFileArtifactValue extends FileArtifactValue { private final byte[] data; @@ -579,8 +557,7 @@ private InlineFileArtifactValue(byte[] data, byte[] digest) { } private InlineFileArtifactValue(byte[] bytes) { - this( - bytes, + this(bytes, DigestHashFunction.getDefaultUnchecked().getHashFunction().hashBytes(bytes).asBytes()); } @@ -604,11 +581,6 @@ public byte[] getDigest() { return digest; } - @Override - public FileContentsProxy getContentsProxy() { - throw new UnsupportedOperationException(); - } - @Override public long getSize() { return data.length; @@ -649,7 +621,8 @@ public static final class SourceFileArtifactValue extends FileArtifactValue { private final byte[] digest; private final long size; - public SourceFileArtifactValue(PathFragment execPath, byte[] digest, long size) { + public SourceFileArtifactValue( + PathFragment execPath, byte[] digest, long size) { this.execPath = Preconditions.checkNotNull(execPath); this.digest = Preconditions.checkNotNull(digest); this.size = size; @@ -669,11 +642,6 @@ public byte[] getDigest() { return digest; } - @Override - public FileContentsProxy getContentsProxy() { - throw new UnsupportedOperationException(); - } - @Override public long getSize() { return size; @@ -702,11 +670,6 @@ public byte[] getDigest() { return null; } - @Override - public FileContentsProxy getContentsProxy() { - throw new UnsupportedOperationException(); - } - @Override public long getSize() { return 0; @@ -745,11 +708,6 @@ public byte[] getDigest() { throw new UnsupportedOperationException(); } - @Override - public FileContentsProxy getContentsProxy() { - throw new UnsupportedOperationException(); - } - @Override public long getSize() { throw new UnsupportedOperationException(); @@ -770,61 +728,4 @@ public String toString() { return "OMITTED_FILE_MARKER"; } } - - private static final class PlaceholderFileValue extends FileArtifactValue implements Singleton { - private static final BigInteger FINGERPRINT = - new BigIntegerFingerprint().addString("PlaceholderFileValue").getFingerprint(); - - private PlaceholderFileValue() {} - - @Override - public long getSize() { - throw new UnsupportedOperationException(); - } - - @Override - public byte[] getDigest() { - throw new UnsupportedOperationException(); - } - - @Override - public FileContentsProxy getContentsProxy() { - throw new UnsupportedOperationException(); - } - - @Override - public BigInteger getValueFingerprint() { - return FINGERPRINT; - } - - @Override - public String toString() { - return "PlaceholderFileValue:Singleton"; - } - - @Override - public FileStateType getType() { - throw new UnsupportedOperationException(); - } - - @Override - public long getModifiedTime() { - throw new UnsupportedOperationException(); - } - - @Override - public boolean wasModifiedSinceDigest(Path path) { - throw new UnsupportedOperationException(); - } - - @Override - public boolean isRemote() { - throw new UnsupportedOperationException(); - } - - @Override - public boolean isMarkerValue() { - throw new UnsupportedOperationException(); - } - } } diff --git a/src/main/java/com/google/devtools/build/lib/actionsketch/ActionSketch.java b/src/main/java/com/google/devtools/build/lib/actionsketch/ActionSketch.java index f936ae7e509b2c..16ddbe8c308888 100644 --- a/src/main/java/com/google/devtools/build/lib/actionsketch/ActionSketch.java +++ b/src/main/java/com/google/devtools/build/lib/actionsketch/ActionSketch.java @@ -16,7 +16,6 @@ import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; -import com.google.devtools.build.skyframe.SkyValue; import com.google.protobuf.ByteString; import java.math.BigInteger; import java.nio.ByteBuffer; @@ -28,7 +27,7 @@ * all transitively consumed input files, as well as a transitive hash of all action keys. */ @AutoValue -public abstract class ActionSketch implements SkyValue { +public abstract class ActionSketch { public static final int BIGINTEGER_ENCODED_LENGTH = /*length=*/ 1 + /*payload=*/ 17; public static final int MAX_BYTES = /*hashes=*/ 2 * BIGINTEGER_ENCODED_LENGTH; diff --git a/src/main/java/com/google/devtools/build/lib/actionsketch/BUILD b/src/main/java/com/google/devtools/build/lib/actionsketch/BUILD deleted file mode 100644 index 0b9fa0816d2087..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/actionsketch/BUILD +++ /dev/null @@ -1,21 +0,0 @@ -package( - default_visibility = ["//src:__subpackages__"], -) - -filegroup( - name = "srcs", - srcs = glob(["**"]), -) - -java_library( - name = "action_sketch", - srcs = glob(["*.java"]), - deps = [ - "//src/main/java/com/google/devtools/build/lib/actions", - "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", - "//third_party:auto_value", - "//third_party:guava", - "//third_party:jsr305", - "//third_party/protobuf:protobuf_java", - ], -) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BashCommandConstructor.java b/src/main/java/com/google/devtools/build/lib/analysis/BashCommandConstructor.java deleted file mode 100644 index 41b2488152c3cd..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/analysis/BashCommandConstructor.java +++ /dev/null @@ -1,50 +0,0 @@ -// Copyright 2019 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.analysis; - -import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.analysis.actions.FileWriteAction; -import com.google.devtools.build.lib.vfs.PathFragment; - -/** The class for constructing command line for Bash. */ -public final class BashCommandConstructor implements CommandConstructor { - - private final PathFragment shellPath; - private final String scriptNameSuffix; - - BashCommandConstructor(PathFragment shellPath, String scriptNameSuffix) { - this.shellPath = shellPath; - this.scriptNameSuffix = scriptNameSuffix; - } - - @Override - public ImmutableList asExecArgv(Artifact scriptFileArtifact) { - return ImmutableList.of(shellPath.getPathString(), scriptFileArtifact.getExecPathString()); - } - - @Override - public ImmutableList asExecArgv(String command) { - return ImmutableList.of(shellPath.getPathString(), "-c", command); - } - - @Override - public Artifact commandAsScript(RuleContext ruleContext, String command) { - String scriptFileName = ruleContext.getTarget().getName() + scriptNameSuffix; - String scriptFileContents = "#!/bin/bash\n" + command; - return FileWriteAction.createFile( - ruleContext, scriptFileName, scriptFileContents, /*executable=*/ true); - } -} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CommandConstructor.java b/src/main/java/com/google/devtools/build/lib/analysis/CommandConstructor.java deleted file mode 100644 index 6fd2023c8539c6..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/analysis/CommandConstructor.java +++ /dev/null @@ -1,37 +0,0 @@ -// Copyright 2019 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.analysis; - -import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.actions.Artifact; - -/** - * The interface to construct command line for different shells (Bash, Batch, Powershell). Used in - * {@link com.google.devtools.build.lib.analysis.CommandHelper} - */ -public interface CommandConstructor { - - /** - * Given a string of command, return the arguments to run the command. eg. For Bash command, - * asExecArgv("foo bar") -> ["/bin/bash", "-c", "foo bar"] - */ - ImmutableList asExecArgv(String command); - - /** Given an artifact of a script, return the arguments to run this command. */ - ImmutableList asExecArgv(Artifact scriptFileArtifact); - - /** Write the command to a script and return the artifact of the script. */ - Artifact commandAsScript(RuleContext ruleContext, String command); -} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java index 4572e1153fe37b..2cd8184bc929d7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CommandHelper.java @@ -23,6 +23,7 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.RunfilesSupplier; +import com.google.devtools.build.lib.analysis.actions.FileWriteAction; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -282,19 +283,38 @@ public String expandLabelsHeuristically(String expr) { } private static Pair, Artifact> buildCommandLineMaybeWithScriptFile( - RuleContext ruleContext, String command, CommandConstructor constructor) { + RuleContext ruleContext, String command, String scriptPostFix, PathFragment shellPath) { List argv; Artifact scriptFileArtifact = null; if (command.length() <= maxCommandLength) { - argv = constructor.asExecArgv(command); + argv = buildCommandLineSimpleArgv(command, shellPath); } else { // Use script file. - scriptFileArtifact = constructor.commandAsScript(ruleContext, command); - argv = constructor.asExecArgv(scriptFileArtifact); + scriptFileArtifact = buildCommandLineArtifact(ruleContext, command, scriptPostFix); + argv = buildCommandLineArgvWithArtifact(scriptFileArtifact, shellPath); } return Pair.of(argv, scriptFileArtifact); } + private static ImmutableList buildCommandLineArgvWithArtifact(Artifact scriptFileArtifact, + PathFragment shellPath) { + return ImmutableList.of(shellPath.getPathString(), scriptFileArtifact.getExecPathString()); + } + + private static Artifact buildCommandLineArtifact(RuleContext ruleContext, String command, + String scriptPostFix) { + String scriptFileName = ruleContext.getTarget().getName() + scriptPostFix; + String scriptFileContents = "#!/bin/bash\n" + command; + Artifact scriptFileArtifact = FileWriteAction.createFile( + ruleContext, scriptFileName, scriptFileContents, /*executable=*/true); + return scriptFileArtifact; + } + + private static ImmutableList buildCommandLineSimpleArgv(String command, + PathFragment shellPath) { + return ImmutableList.of(shellPath.getPathString(), "-c", command); + } + /** * If {@code command} is too long, creates a helper shell script that runs that command. * @@ -304,24 +324,49 @@ private static Pair, Artifact> buildCommandLineMaybeWithScriptFile( * this method does nothing and returns null. */ @Nullable - public static Artifact commandHelperScriptMaybe( - RuleContext ruleCtx, String command, CommandConstructor constructor) { + public static Artifact shellCommandHelperScriptMaybe( + RuleContext ruleCtx, + String command, + String scriptPostFix, + Map executionInfo) { if (command.length() <= maxCommandLength) { return null; } else { - return constructor.commandAsScript(ruleCtx, command); + return buildCommandLineArtifact(ruleCtx, command, scriptPostFix); } } + /** + * Builds the set of command-line arguments. Creates a bash script if the command line is longer + * than the allowed maximum {@link #maxCommandLength}. Fixes up the input artifact list with the + * created bash script when required. + */ + public List buildCommandLine( + PathFragment shExecutable, + String command, + NestedSetBuilder inputs, + String scriptPostFix) { + return buildCommandLine( + shExecutable, command, inputs, scriptPostFix, ImmutableMap.of()); + } + /** * Builds the set of command-line arguments using the specified shell path. Creates a bash script * if the command line is longer than the allowed maximum {@link #maxCommandLength}. Fixes up the * input artifact list with the created bash script when required. + * + * @param executionInfo an execution info map of the action associated with the command line to be + * built. */ public List buildCommandLine( - String command, NestedSetBuilder inputs, CommandConstructor constructor) { + PathFragment shExecutable, + String command, + NestedSetBuilder inputs, + String scriptPostFix, + Map executionInfo) { Pair, Artifact> argvAndScriptFile = - buildCommandLineMaybeWithScriptFile(ruleContext, command, constructor); + buildCommandLineMaybeWithScriptFile( + ruleContext, command, scriptPostFix, shellPath(executionInfo, shExecutable)); if (argvAndScriptFile.second != null) { inputs.add(argvAndScriptFile.second); } @@ -334,9 +379,14 @@ public List buildCommandLine( * created bash script when required. */ public List buildCommandLine( - String command, List inputs, CommandConstructor constructor) { + PathFragment shExecutable, + String command, + List inputs, + String scriptPostFix, + Map executionInfo) { Pair, Artifact> argvAndScriptFile = - buildCommandLineMaybeWithScriptFile(ruleContext, command, constructor); + buildCommandLineMaybeWithScriptFile( + ruleContext, command, scriptPostFix, shellPath(executionInfo, shExecutable)); if (argvAndScriptFile.second != null) { inputs.add(argvAndScriptFile.second); } @@ -344,16 +394,10 @@ public List buildCommandLine( } /** Returns the path to the shell for an action with the given execution requirements. */ - private static PathFragment shellPath( - Map executionInfo, PathFragment shExecutable) { + private PathFragment shellPath(Map executionInfo, PathFragment shExecutable) { // Use vanilla /bin/bash for actions running on mac machines. return executionInfo.containsKey(ExecutionRequirements.REQUIRES_DARWIN) ? PathFragment.create("/bin/bash") : shExecutable; } - - public static BashCommandConstructor buildBashCommandConstructor( - Map executionInfo, PathFragment shExecutable, String scriptPostFix) { - return new BashCommandConstructor(shellPath(executionInfo, shExecutable), scriptPostFix); - } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java index 799b6d1f9838ba..3027a99344ab8d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java @@ -16,7 +16,6 @@ import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.AnalysisUtils; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.DefaultInfo; @@ -37,23 +36,20 @@ import com.google.devtools.build.lib.skyframe.BuildConfigurationValue; import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter; import com.google.devtools.build.lib.skylarkinterface.StarlarkContext; +import com.google.devtools.build.lib.syntax.ClassObject; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.EvalUtils; -import com.google.devtools.build.lib.syntax.Mutability; import com.google.devtools.build.lib.syntax.Printer; -import com.google.devtools.build.lib.syntax.SkylarkClassObject; -import com.google.devtools.build.lib.syntax.SkylarkType; -import com.google.devtools.build.lib.syntax.StarlarkSemantics; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import javax.annotation.Nullable; /** - * An abstract implementation of ConfiguredTarget in which all properties are assigned trivial - * default values. + * An abstract implementation of ConfiguredTarget in which all properties are + * assigned trivial default values. */ public abstract class AbstractConfiguredTarget - implements ConfiguredTarget, VisibilityProvider, SkylarkClassObject { + implements ConfiguredTarget, VisibilityProvider, ClassObject { private final Label label; private final BuildConfigurationValue.Key configurationKey; @@ -66,18 +62,6 @@ public abstract class AbstractConfiguredTarget private static final String DATA_RUNFILES_FIELD = "data_runfiles"; private static final String DEFAULT_RUNFILES_FIELD = "default_runfiles"; - // A set containing all field names which may be specially handled (and thus may not be - // attributed to normal user-specified providers). - private static final ImmutableSet SPECIAL_FIELD_NAMES = - ImmutableSet.of( - LABEL_FIELD, - FILES_FIELD, - DEFAULT_RUNFILES_FIELD, - DATA_RUNFILES_FIELD, - FilesToRunProvider.SKYLARK_NAME, - OutputGroupInfo.SKYLARK_NAME, - RuleConfiguredTarget.ACTIONS_FIELD_NAME); - public AbstractConfiguredTarget(Label label, BuildConfigurationValue.Key configurationKey) { this(label, configurationKey, NestedSetBuilder.emptySet(Order.STABLE_ORDER)); } @@ -121,34 +105,11 @@ public

    P getProvider(Class

    provider) { } } - @Override - public Object getValue(Location loc, StarlarkSemantics semantics, String name) - throws EvalException { - if (semantics.incompatibleDisableTargetProviderFields() - && !SPECIAL_FIELD_NAMES.contains(name)) { - throw new EvalException( - loc, - "Accessing providers via the field syntax on structs is " - + "deprecated and will be removed soon. It may be temporarily re-enabled by setting " - + "--incompatible_disable_target_provider_fields=false. See " - + "https://github.com/bazelbuild/bazel/issues/9014 for details."); - } - return getValue(name); - } - @Override public Object getValue(String name) { switch (name) { case LABEL_FIELD: return getLabel(); - case RuleConfiguredTarget.ACTIONS_FIELD_NAME: - // Depending on subclass, the 'actions' field will either be unsupported or of type - // java.util.List, which needs to be converted to SkylarkList before being returned. - Object result = get(name); - if (result != null) { - result = SkylarkType.convertToSkylark(result, (Mutability) null); - } - return result; default: return get(name); } @@ -246,6 +207,9 @@ public String getRuleClassString() { */ @Override public final Object get(String providerKey) { + if (OutputGroupInfo.SKYLARK_NAME.equals(providerKey)) { + return get(OutputGroupInfo.SKYLARK_CONSTRUCTOR); + } switch (providerKey) { case FILES_FIELD: return getDefaultProvider().getFiles(); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraActionSpec.java b/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraActionSpec.java index 56b17864aa3f1d..5c17eff20a4195 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraActionSpec.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/extra/ExtraActionSpec.java @@ -23,7 +23,6 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.RunfilesSupplier; -import com.google.devtools.build.lib.analysis.BashCommandConstructor; import com.google.devtools.build.lib.analysis.CommandHelper; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; @@ -130,10 +129,13 @@ public Collection addExtraAction( actionToShadow.getPrimaryOutput().getExecPath().getBaseName() + "." + actionToShadow.getKey(owningRule.getActionKeyContext()); - BashCommandConstructor constructor = - CommandHelper.buildBashCommandConstructor( - executionInfo, shExecutable, "." + actionUniquifier + ".extra_action_script.sh"); - List argv = commandHelper.buildCommandLine(command, extraActionInputs, constructor); + List argv = + commandHelper.buildCommandLine( + shExecutable, + command, + extraActionInputs, + "." + actionUniquifier + ".extra_action_script.sh", + executionInfo); String commandMessage = String.format("Executing extra_action %s on %s", label, ownerLabel); owningRule.registerAction( diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java index 1d09ae7f34fed5..1373987a2b8ce3 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java @@ -90,7 +90,7 @@ public String remoteExecutionProperties() { return remoteExecutionProperties; } - @Override + // TODO(agoulti): expose this in the Starlark API public ImmutableMap execProperties() { return execProperties; } @@ -237,40 +237,13 @@ public Builder setLocation(Location location) { return this; } - private void checkRemoteExecutionProperties() throws ExecPropertiesException { - if (execProperties != null && !Strings.isNullOrEmpty(remoteExecutionProperties)) { - throw new ExecPropertiesException( - "Platform contains both remote_execution_properties and exec_properties. Prefer" - + " exec_properties over the deprecated remote_execution_properties."); - } - if (execProperties != null - && parent != null - && !Strings.isNullOrEmpty(parent.remoteExecutionProperties())) { - throw new ExecPropertiesException( - "Platform specifies exec_properties but its parent " - + parent.label() - + " specifies remote_execution_properties. Prefer exec_properties over the" - + " deprecated remote_execution_properties."); - } - if (!Strings.isNullOrEmpty(remoteExecutionProperties) - && parent != null - && !parent.execProperties().isEmpty()) { - throw new ExecPropertiesException( - "Platform specifies remote_execution_properties but its parent specifies" - + " exec_properties. Prefer exec_properties over the deprecated" - + " remote_execution_properties."); - } - } - /** * Returns the new {@link PlatformInfo} instance. * * @throws DuplicateConstraintException if more than one constraint value exists for the same * constraint setting */ - public PlatformInfo build() throws DuplicateConstraintException, ExecPropertiesException { - checkRemoteExecutionProperties(); - + public PlatformInfo build() throws DuplicateConstraintException { // Merge the remote execution properties. String remoteExecutionProperties = mergeRemoteExecutionProperties(parent, this.remoteExecutionProperties); @@ -342,11 +315,4 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(label, constraints, remoteExecutionProperties); } - - /** Exception that indicates something is wrong in exec_properties configuration. */ - public static class ExecPropertiesException extends Exception { - ExecPropertiesException(String message) { - super(message); - } - } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java index 765294ead8fb09..739b25661343b7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java @@ -23,7 +23,6 @@ import com.google.protobuf.TextFormat.ParseException; import java.util.Comparator; import java.util.List; -import java.util.Map; import javax.annotation.Nullable; /** Utilities for accessing platform properties. */ @@ -44,11 +43,7 @@ public static Platform getPlatformProto( Platform.Builder platformBuilder = Platform.newBuilder(); - if (executionPlatform != null && !executionPlatform.execProperties().isEmpty()) { - for (Map.Entry entry : executionPlatform.execProperties().entrySet()) { - platformBuilder.addPropertiesBuilder().setName(entry.getKey()).setValue(entry.getValue()); - } - } else if (executionPlatform != null + if (executionPlatform != null && !Strings.isNullOrEmpty(executionPlatform.remoteExecutionProperties())) { // Try and get the platform info from the execution properties. try { @@ -75,11 +70,10 @@ public static Platform getPlatformProto( } // Sort the properties. - List properties = - Ordering.from(Comparator.comparing(Platform.Property::getName)) - .sortedCopy(platformBuilder.getPropertiesList()); + List properties = platformBuilder.getPropertiesList(); platformBuilder.clearProperties(); - platformBuilder.addAllProperties(properties); + platformBuilder.addAllProperties( + Ordering.from(Comparator.comparing(Platform.Property::getName)).sortedCopy(properties)); return platformBuilder.build(); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java index 9cfd02e8587b71..16dea75ab02648 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java @@ -34,7 +34,6 @@ import com.google.devtools.build.lib.actions.SingleStringArgFormatter; import com.google.devtools.build.lib.actions.extra.ExtraActionInfo; import com.google.devtools.build.lib.actions.extra.SpawnInfo; -import com.google.devtools.build.lib.analysis.BashCommandConstructor; import com.google.devtools.build.lib.analysis.CommandHelper; import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.PseudoAction; @@ -325,12 +324,10 @@ public void runShell( ImmutableMap.copyOf(TargetUtils.getExecutionInfo(ruleContext.getRule())); String helperScriptSuffix = String.format(".run_shell_%d.sh", runShellOutputCounter++); String command = (String) commandUnchecked; - PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext); - BashCommandConstructor constructor = - CommandHelper.buildBashCommandConstructor( - executionInfo, shExecutable, helperScriptSuffix); Artifact helperScript = - CommandHelper.commandHelperScriptMaybe(ruleContext, command, constructor); + CommandHelper.shellCommandHelperScriptMaybe( + ruleContext, command, helperScriptSuffix, executionInfo); + PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext); if (helperScript == null) { builder.setShellCommand(shExecutable, command); } else { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java index 51e1980cc891cb..d9c821e8658593 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java @@ -30,7 +30,6 @@ import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.analysis.ActionsProvider; import com.google.devtools.build.lib.analysis.AliasProvider; -import com.google.devtools.build.lib.analysis.BashCommandConstructor; import com.google.devtools.build.lib.analysis.CommandHelper; import com.google.devtools.build.lib.analysis.ConfigurationMakeVariableContext; import com.google.devtools.build.lib.analysis.DefaultInfo; @@ -1068,15 +1067,15 @@ public Tuple resolveCommand( String.class, "execution_requirements")); PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext); - - BashCommandConstructor constructor = - CommandHelper.buildBashCommandConstructor( - executionRequirements, + List argv = + helper.buildCommandLine( shExecutable, + command, + inputs, // Hash the command-line to prevent multiple actions from the same rule invocation // conflicting with each other. - "." + Hashing.murmur3_32().hashUnencodedChars(command).toString() + SCRIPT_SUFFIX); - List argv = helper.buildCommandLine(command, inputs, constructor); + "." + Hashing.murmur3_32().hashUnencodedChars(command).toString() + SCRIPT_SUFFIX, + executionRequirements); return Tuple.of( MutableList.copyOf(env, inputs), MutableList.copyOf(env, argv), diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java index b5f51ceb0c7d85..e7a83540995e89 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java @@ -88,12 +88,9 @@ private static ResourceSet getResourceSetFromSize(TestSize size) { Map executionInfo = Maps.newLinkedHashMap(); executionInfo.putAll(TargetUtils.getExecutionInfo(rule)); - if (TargetUtils.isLocalTestRule(rule)) { + if (TargetUtils.isLocalTestRule(rule) || TargetUtils.isExclusiveTestRule(rule)) { executionInfo.put(ExecutionRequirements.LOCAL, ""); } - if (TargetUtils.isExclusiveTestRule(rule)) { - executionInfo.put(ExecutionRequirements.NO_REMOTE_EXEC, ""); - } if (executionRequirements != null) { // This will overwrite whatever TargetUtils put there, which might be confusing. diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index c8d0ba8edd81af..aa7034065657e8 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -628,7 +628,6 @@ private Builder createBuilder( .setEnabled(options.useActionCache) .setVerboseExplanations(options.verboseExplanations) .build()), - env.getTopDownActionCache(), request.getPackageCacheOptions().checkOutputFiles ? modifiedOutputFiles : ModifiedFileSet.NOTHING_MODIFIED, diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java index f45b673e8b047e..0a8f4cec83e274 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java @@ -45,7 +45,6 @@ import com.google.devtools.build.lib.skyframe.Builder; import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; import com.google.devtools.build.lib.skyframe.SkyframeExecutor; -import com.google.devtools.build.lib.skyframe.TopDownActionCache; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.ExitCode; import com.google.devtools.build.lib.util.LoggingUtil; @@ -77,19 +76,16 @@ public class SkyframeBuilder implements Builder { private final MetadataProvider fileCache; private final ActionInputPrefetcher actionInputPrefetcher; private final ActionCacheChecker actionCacheChecker; - private final TopDownActionCache topDownActionCache; @VisibleForTesting public SkyframeBuilder( SkyframeExecutor skyframeExecutor, ActionCacheChecker actionCacheChecker, - TopDownActionCache topDownActionCache, ModifiedFileSet modifiedOutputFiles, MetadataProvider fileCache, ActionInputPrefetcher actionInputPrefetcher) { this.skyframeExecutor = skyframeExecutor; this.actionCacheChecker = actionCacheChecker; - this.topDownActionCache = topDownActionCache; this.modifiedOutputFiles = modifiedOutputFiles; this.fileCache = fileCache; this.actionInputPrefetcher = actionInputPrefetcher; @@ -160,7 +156,6 @@ public void buildArtifacts( exclusiveTests, options, actionCacheChecker, - topDownActionCache, executionProgressReceiver, topLevelArtifactContext); // progressReceiver is finished, so unsynchronized access to builtTargets is now safe. @@ -188,7 +183,6 @@ public void buildArtifacts( exclusiveTest, options, actionCacheChecker, - topDownActionCache, null, topLevelArtifactContext); exitCode = diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java index c18438fdc9c544..0837b1f5d83b71 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java @@ -32,7 +32,6 @@ import com.google.devtools.build.lib.syntax.EvalUtils; import com.google.devtools.build.lib.syntax.SkylarkClassObject; import com.google.devtools.build.lib.syntax.SkylarkType; -import com.google.devtools.build.lib.syntax.StarlarkSemantics; import java.util.Arrays; import java.util.List; import java.util.Map; @@ -98,13 +97,6 @@ public boolean isCompact() { return getLayout() != null; } - @Override - public Object getValue(Location loc, StarlarkSemantics starlarkSemantics, String name) - throws EvalException { - // By default, a SkylarkInfo's field values are not affected by the Starlark semantics. - return getValue(name); - } - /** * Creates a schemaless (map-based) provider instance with the given provider type and field * values. diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java index f588a4e22460e0..65c65e47461166 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java @@ -232,23 +232,6 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl + "convert to a list.") public boolean incompatibleDepsetIsNotIterable; - @Option( - name = "incompatible_disable_target_provider_fields", - 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, disable the ability to access providers on 'target' objects via field " - + "syntax. Use provider-key syntax instead. For example, instead of using " - + "`ctx.attr.dep.my_info` to access `my_info` from inside a rule implementation " - + "function, use `ctx.attr.dep[MyInfo]`. See " - + "https://github.com/bazelbuild/bazel/issues/9014 for details.") - public boolean incompatibleDisableTargetProviderFields; - @Option( name = "incompatible_disable_deprecated_attr_params", defaultValue = "true", @@ -719,7 +702,6 @@ public StarlarkSemantics toSkylarkSemantics() { .incompatibleBzlDisallowLoadAfterStatement(incompatibleBzlDisallowLoadAfterStatement) .incompatibleDepsetIsNotIterable(incompatibleDepsetIsNotIterable) .incompatibleDepsetUnion(incompatibleDepsetUnion) - .incompatibleDisableTargetProviderFields(incompatibleDisableTargetProviderFields) .incompatibleDisableThirdPartyLicenseChecking( incompatibleDisableThirdPartyLicenseChecking) .incompatibleDisableDeprecatedAttrParams(incompatibleDisableDeprecatedAttrParams) diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java index 110e8f2aff27b9..cd4b6051d6f2e9 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java @@ -477,7 +477,19 @@ private Digest uploadFileContents(Path file) throws IOException, InterruptedExce } return digest; } - + + Digest uploadBlob(byte[] blob) throws IOException, InterruptedException { + Digest digest = digestUtil.compute(blob); + ImmutableSet missing = getMissingDigests(ImmutableList.of(digest)); + if (!missing.isEmpty()) { + uploader.uploadBlob( + HashCode.fromString(digest.getHash()), + Chunker.builder().setInput(blob).build(), + /* forceUpload=*/ true); + } + return digest; + } + // Execution Cache API @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java index 15576573cef05d..37f048c46f853d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java @@ -23,7 +23,6 @@ import com.google.devtools.build.lib.actions.CompositeRunfilesSupplier; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.AliasProvider; -import com.google.devtools.build.lib.analysis.CommandConstructor; import com.google.devtools.build.lib.analysis.CommandHelper; import com.google.devtools.build.lib.analysis.ConfigurationMakeVariableContext; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -168,12 +167,13 @@ public String toString() { if (ruleContext.hasErrors()) { return null; } - - CommandConstructor constructor = - CommandHelper.buildBashCommandConstructor( - executionInfo, shExecutable, ".genrule_script.sh"); - - List argv = commandHelper.buildCommandLine(command, inputs, constructor); + List argv = + commandHelper.buildCommandLine( + shExecutable, + command, + inputs, + ".genrule_script.sh", + ImmutableMap.copyOf(executionInfo)); if (isStampingEnabled(ruleContext)) { inputs.add(ruleContext.getAnalysisEnvironment().getStableWorkspaceStatusArtifact()); diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java b/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java index 991e952551b184..c73d5f83e5ebf0 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java +++ b/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.rules.platform; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; @@ -37,7 +36,6 @@ import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.util.Pair; import java.util.List; -import java.util.Map; /** Defines a platform for execution contexts. */ public class Platform implements RuleConfiguredTargetFactory { @@ -79,20 +77,12 @@ public ConfiguredTarget create(RuleContext ruleContext) ruleContext.attributes().get(PlatformRule.REMOTE_EXECUTION_PROPS_ATTR, Type.STRING); platformBuilder.setRemoteExecutionProperties(remoteExecutionProperties); - Map execProperties = - ruleContext.attributes().get(PlatformRule.EXEC_PROPS_ATTR, Type.STRING_DICT); - if (execProperties != null && !execProperties.isEmpty()) { - platformBuilder.setExecProperties(ImmutableMap.copyOf(execProperties)); - } - PlatformInfo platformInfo; try { platformInfo = platformBuilder.build(); } catch (ConstraintCollection.DuplicateConstraintException e) { throw ruleContext.throwWithAttributeError( PlatformRule.CONSTRAINT_VALUES_ATTR, e.getMessage()); - } catch (PlatformInfo.ExecPropertiesException e) { - throw ruleContext.throwWithAttributeError(PlatformRule.EXEC_PROPS_ATTR, e.getMessage()); } return new RuleConfiguredTargetBuilder(ruleContext) diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformRule.java b/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformRule.java index 994854d6a162bb..bafdb7b0e7e9b4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformRule.java @@ -31,7 +31,6 @@ public class PlatformRule implements RuleDefinition { public static final String CONSTRAINT_VALUES_ATTR = "constraint_values"; public static final String PARENTS_PLATFORM_ATTR = "parents"; public static final String REMOTE_EXECUTION_PROPS_ATTR = "remote_execution_properties"; - public static final String EXEC_PROPS_ATTR = "exec_properties"; static final String HOST_PLATFORM_ATTR = "host_platform"; static final String TARGET_PLATFORM_ATTR = "target_platform"; static final String CPU_CONSTRAINTS_ATTR = "cpu_constraints"; @@ -70,8 +69,6 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) .mandatoryProviders(PlatformInfo.PROVIDER.id())) /* - DEPRECATED. Please use exec_properties attribute instead. - A string used to configure a remote execution platform. Actual builds make no attempt to interpret this, it is treated as opaque data that can be used by a specific SpawnRunner. This can include data from the parent platform's "remote_execution_properties" attribute, @@ -80,19 +77,6 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) */ .add(attr(REMOTE_EXECUTION_PROPS_ATTR, Type.STRING)) - /* - A map of strings used to configure a remote execution platform. Bazel makes no attempt - to interpret this, it is treated as opaque data that's forwarded via the remote execution - protocol. - - This includes any data from the parent platform's exec_properties attributes. - If the child and parent platform define the same keys, the child's values are kept. - - This attribute is a full replacement for the deprecated - remote_execution_properties. - */ - .add(attr(EXEC_PROPS_ATTR, Type.STRING_DICT)) - // Undocumented. Indicates that this platform should auto-configure the platform constraints // based on the current host OS and CPU settings. .add( diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java index cf49eb13cd8d00..a90cb1532a3e02 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java @@ -67,9 +67,23 @@ /** A helper class for analyzing a Python configured target. */ public final class PyCommon { + /** Deprecated name of the version attribute. */ public static final String DEFAULT_PYTHON_VERSION_ATTRIBUTE = "default_python_version"; + /** Name of the version attribute. */ public static final String PYTHON_VERSION_ATTRIBUTE = "python_version"; + /** + * Name of the tag used by bazelbuild/rules_python to signal that a rule was instantiated through + * that repo. + */ + private static final String MAGIC_TAG = "__PYTHON_RULES_MIGRATION_DO_NOT_USE_WILL_BREAK__"; + /** + * Names of native rules that must be instantiated through bazelbuild/rules_python when {@code + * --incompatible_load_python_rules_from_bzl} is enabled. + */ + private static final ImmutableList RULES_REQUIRING_MAGIC_TAG = + ImmutableList.of("py_library", "py_binary", "py_test", "py_runtime"); + /** * Returns the Python version based on the {@code python_version} and {@code * default_python_version} attributes of the given {@code AttributeMap}. @@ -207,6 +221,7 @@ public PyCommon(RuleContext ruleContext, PythonSemantics semantics) { validateTargetPythonVersionAttr(PYTHON_VERSION_ATTRIBUTE); validateOldVersionAttrNotUsedIfDisabled(); validateLegacyProviderNotUsedIfDisabled(); + maybeValidateLoadedFromBzl(); } /** Returns the parsed value of the "srcs_version" attribute. */ @@ -600,6 +615,35 @@ private void validateLegacyProviderNotUsedIfDisabled() { } } + /** + * If {@code --incompatible_load_python_rules_from_bzl} is enabled, reports a rule error if the + * rule is one of the ones that has a redirect in bazelbuild/rules_python, and either 1) the magic + * tag {@code __PYTHON_RULES_MIGRATION_DO_NOT_USE_WILL_BREAK__} is not present, or 2) the target + * was not created in a macro. + * + *

    No-op otherwise. + */ + private void maybeValidateLoadedFromBzl() { + if (!ruleContext.getFragment(PythonConfiguration.class).loadPythonRulesFromBzl()) { + return; + } + String ruleName = ruleContext.getRule().getRuleClass(); + if (!RULES_REQUIRING_MAGIC_TAG.contains(ruleName)) { + return; + } + + boolean hasMagicTag = + ruleContext.attributes().get("tags", Type.STRING_LIST).contains(MAGIC_TAG); + if (!hasMagicTag || !ruleContext.getRule().wasCreatedByMacro()) { + ruleContext.ruleError( + "Direct access to the native Python rules is deprecated. Please load " + + ruleName + + " from the rules_python repository. See http://github.com/bazelbuild/rules_python " + + "and https://github.com/bazelbuild/bazel/issues/9006. You can temporarily bypass " + + "this error by setting --incompatible_load_python_rules_from_bzl=false."); + } + } + /** * Under the new version semantics ({@code --incompatible_allow_python_version_transitions=true}), * if the Python version (as determined by the configuration) is inconsistent with {@link diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java index 7c2291d3ea4071..761b139e25e80f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java @@ -55,6 +55,9 @@ public class PythonConfiguration extends BuildConfiguration.Fragment { // TODO(brandjon): Remove this once migration to Python toolchains is complete. private final boolean useToolchains; + // TODO(brandjon): Remove this once migration for native rule access is complete. + private final boolean loadPythonRulesFromBzl; + private final boolean windowsEscapePythonArgs; PythonConfiguration( @@ -67,6 +70,7 @@ public class PythonConfiguration extends BuildConfiguration.Fragment { boolean py2OutputsAreSuffixed, boolean disallowLegacyPyProvider, boolean useToolchains, + boolean loadPythonRulesFromBzl, boolean windowsEscapePythonArgs) { this.version = version; this.defaultVersion = defaultVersion; @@ -77,6 +81,7 @@ public class PythonConfiguration extends BuildConfiguration.Fragment { this.py2OutputsAreSuffixed = py2OutputsAreSuffixed; this.disallowLegacyPyProvider = disallowLegacyPyProvider; this.useToolchains = useToolchains; + this.loadPythonRulesFromBzl = loadPythonRulesFromBzl; this.windowsEscapePythonArgs = windowsEscapePythonArgs; } @@ -193,6 +198,17 @@ public boolean useToolchains() { return useToolchains; } + /** + * Returns true if native Python rules should fail at analysis time when the magic tag, {@code + * __PYTHON_RULES_MIGRATION_DO_NOT_USE_WILL_BREAK__}, is not present. + * + *

    This tag is set by the macros in bazelbuild/rules_python and should not be used anywhere + * else. + */ + public boolean loadPythonRulesFromBzl() { + return loadPythonRulesFromBzl; + } + public boolean windowsEscapePythonArgs() { return windowsEscapePythonArgs; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java index 8b74f2a1d4e37c..c24b16abce1fd0 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java @@ -44,6 +44,7 @@ public PythonConfiguration create(BuildOptions buildOptions) /*py2OutputsAreSuffixed=*/ pythonOptions.incompatiblePy2OutputsAreSuffixed, /*disallowLegacyPyProvider=*/ pythonOptions.incompatibleDisallowLegacyPyProvider, /*useToolchains=*/ pythonOptions.incompatibleUsePythonToolchains, + /*loadPythonRulesFromBzl=*/ pythonOptions.loadPythonRulesFromBzl, pythonOptions.windowsEscapePythonArgs); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java index ff06fdae3d7649..8a27db098699e3 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java @@ -244,6 +244,21 @@ public String getTypeDescription() { + "--python_top.") public boolean incompatibleUsePythonToolchains; + @Option( + name = "incompatible_load_python_rules_from_bzl", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES, + }, + help = + "If enabled, direct use of the native Python rules is disabled. Please use the Starlark " + + "rules instead: https://github.com/bazelbuild/rules_python. See also " + + "https://github.com/bazelbuild/bazel/issues/9006.") + public boolean loadPythonRulesFromBzl; + @Option( name = "experimental_build_transitive_python_runfiles", defaultValue = "true", @@ -392,6 +407,7 @@ public FragmentOptions getHost() { hostPythonOptions.buildPythonZip = buildPythonZip; hostPythonOptions.incompatibleDisallowLegacyPyProvider = incompatibleDisallowLegacyPyProvider; hostPythonOptions.incompatibleUsePythonToolchains = incompatibleUsePythonToolchains; + hostPythonOptions.loadPythonRulesFromBzl = loadPythonRulesFromBzl; hostPythonOptions.windowsEscapePythonArgs = windowsEscapePythonArgs; // Save host options in case of a further exec->host transition. diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java index 4fd69b29365ae4..13ed9f9c9a7cee 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java @@ -33,7 +33,6 @@ import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.skyframe.AspectValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue; -import com.google.devtools.build.lib.skyframe.TopDownActionCache; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.vfs.DigestHashFunction.DefaultHashFunctionNotSetException; @@ -94,17 +93,6 @@ public ModuleFileSystem getFileSystem( return null; } - /** - * Returns the {@link TopDownActionCache} used by Bazel. It is an error if more than one module - * returns a top-down action cache. If all modules return null, there will be no top-down caching. - * - *

    This method will be called at the beginning of Bazel startup (in-between {@link #globalInit} - * and {@link #blazeStartup}). - */ - public TopDownActionCache getTopDownActionCache() { - return null; - } - /** Tuple returned by {@link #getFileSystem}. */ @AutoValue public abstract static class ModuleFileSystem { diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java index cb29551cb4966a..668b333930db76 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java @@ -34,7 +34,6 @@ import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy; import com.google.devtools.build.lib.skyframe.SkyframeBuildView; import com.google.devtools.build.lib.skyframe.SkyframeExecutor; -import com.google.devtools.build.lib.skyframe.TopDownActionCache; import com.google.devtools.build.lib.util.AbruptExitException; import com.google.devtools.build.lib.util.ExitCode; import com.google.devtools.build.lib.util.io.OutErr; @@ -88,7 +87,6 @@ public final class CommandEnvironment { private PathFragment relativeWorkingDirectory = PathFragment.EMPTY_FRAGMENT; private long commandStartTime; private OutputService outputService; - private TopDownActionCache topDownActionCache; private Path workingDirectory; private String workspaceName; private boolean haveSetupPackageCache = false; @@ -492,11 +490,6 @@ public ActionCache getPersistentActionCache() throws IOException { return workspace.getPersistentActionCache(reporter); } - /** Returns the top-down action cache to use, or null. */ - public TopDownActionCache getTopDownActionCache() { - return topDownActionCache; - } - /** * An array of String values useful if Blaze crashes. For now, just returns the build id as soon * as it is determined. @@ -640,8 +633,6 @@ void beforeCommand(long waitTimeInMs, InvocationPolicy invocationPolicy) outputService = null; BlazeModule outputModule = null; - topDownActionCache = null; - BlazeModule topDownCachingModule = null; if (command.builds()) { for (BlazeModule module : runtime.getBlazeModules()) { OutputService moduleService = module.getOutputService(); @@ -655,18 +646,6 @@ void beforeCommand(long waitTimeInMs, InvocationPolicy invocationPolicy) outputService = moduleService; outputModule = module; } - - TopDownActionCache moduleCache = module.getTopDownActionCache(); - if (moduleCache != null) { - if (topDownActionCache != null) { - throw new IllegalStateException( - String.format( - "More than one module (%s and %s) returns a top down action cache", - module.getClass(), topDownCachingModule.getClass())); - } - topDownActionCache = moduleCache; - topDownCachingModule = module; - } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index c4eedd23431ce0..ded849a7c027cb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -45,7 +45,6 @@ import com.google.devtools.build.lib.actions.MissingInputFileException; import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit; import com.google.devtools.build.lib.actions.PackageRootResolver; -import com.google.devtools.build.lib.actionsketch.ActionSketch; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.causes.Cause; import com.google.devtools.build.lib.causes.LabelCause; @@ -101,7 +100,6 @@ * */ public class ActionExecutionFunction implements SkyFunction { - private final ActionRewindStrategy actionRewindStrategy = new ActionRewindStrategy(); private final SkyframeActionExecutor skyframeActionExecutor; private final BlazeDirectories directories; @@ -156,19 +154,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) } } - ActionSketch sketch = null; - TopDownActionCache topDownActionCache = skyframeActionExecutor.getTopDownActionCache(); - if (topDownActionCache != null) { - sketch = (ActionSketch) env.getValue(ActionSketchFunction.key(actionLookupData)); - if (sketch == null) { - return null; - } - ActionExecutionValue actionExecutionValue = topDownActionCache.get(sketch); - if (actionExecutionValue != null) { - return actionExecutionValue.transformForSharedAction(action.getOutputs()); - } - } - // For restarts of this ActionExecutionFunction we use a ContinuationState variable, below, to // avoid redoing work. // @@ -304,9 +289,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) // Remove action from state map in case it's there (won't be unless it discovers inputs). stateMap.remove(action); - if (sketch != null && result.dataIsShareable()) { - topDownActionCache.put(sketch, result); - } return result; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java index ffc043368dfa52..76fddb8c1361ed 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java @@ -23,14 +23,17 @@ import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper; +import com.google.devtools.build.lib.actions.ArtifactFileMetadata; import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.actions.FilesetOutputSymlink; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.util.BigIntegerFingerprint; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.SkyValue; import java.math.BigInteger; import java.util.Comparator; @@ -61,9 +64,9 @@ public class ActionExecutionValue implements SkyValue { * checked quickly against the actual filesystem on incremental builds. * *

    If additional metadata is not needed here over what is in {@link #additionalOutputData}, the - * value will be {@link FileArtifactValue#PLACEHOLDER}. + * value will be {@link ArtifactFileMetadata#PLACEHOLDER}. */ - private final ImmutableMap artifactData; + private final ImmutableMap artifactData; /** The TreeArtifactValue of all TreeArtifacts output by this Action. */ private final ImmutableMap treeArtifactData; @@ -84,7 +87,7 @@ public class ActionExecutionValue implements SkyValue { @Nullable private transient BigInteger valueFingerprint; /** - * @param artifactData Map from Artifacts to corresponding {@link FileArtifactValue}. + * @param artifactData Map from Artifacts to corresponding {@link ArtifactFileMetadata}. * @param treeArtifactData All tree artifact data. * @param additionalOutputData Map from Artifacts to values if the FileArtifactValue for this * artifact cannot be derived from the corresponding FileValue (see {@link @@ -94,7 +97,7 @@ public class ActionExecutionValue implements SkyValue { * @param discoveredModules cpp modules discovered */ private ActionExecutionValue( - Map artifactData, + Map artifactData, Map treeArtifactData, Map additionalOutputData, @Nullable ImmutableList outputSymlinks, @@ -121,7 +124,7 @@ static ActionExecutionValue createFromOutputStore( } static ActionExecutionValue create( - Map artifactData, + Map artifactData, Map treeArtifactData, Map additionalOutputData, @Nullable ImmutableList outputSymlinks, @@ -152,7 +155,7 @@ public FileArtifactValue getArtifactValue(Artifact artifact) { * @return The data for each non-middleman output of this action, in the form of the {@link * FileValue} that would be created for the file if it were to be read from disk. */ - FileArtifactValue getData(Artifact artifact) { + ArtifactFileMetadata getData(Artifact artifact) { Preconditions.checkState(!additionalOutputData.containsKey(artifact), "Should not be requesting data for already-constructed FileArtifactValue: %s", artifact); return artifactData.get(artifact); @@ -168,7 +171,7 @@ TreeArtifactValue getTreeArtifactValue(Artifact artifact) { * returned by {@link #getData}. Primarily needed by {@link FilesystemValueChecker}, also * called by {@link ArtifactFunction} when aggregating a {@link TreeArtifactValue}. */ - Map getAllFileValues() { + Map getAllFileValues() { return Maps.transformEntries(artifactData, this::transformIfPlaceholder); } @@ -199,7 +202,7 @@ public BigInteger getValueFingerprint() { .forEach( (entry) -> { fp.addPath(entry.getKey().getExecPath()); - fp.addBigIntegerOrdered(entry.getValue().getValueFingerprint()); + fp.addBigIntegerOrdered(entry.getValue().getFingerprint()); }); sortMapByArtifactExecPathAndStream(treeArtifactData) .forEach( @@ -262,11 +265,12 @@ public int hashCode() { } /** Transforms PLACEHOLDER values into normal instances. */ - private FileArtifactValue transformIfPlaceholder(Artifact artifact, FileArtifactValue value) { + private ArtifactFileMetadata transformIfPlaceholder( + Artifact artifact, ArtifactFileMetadata value) { Preconditions.checkState(!artifact.isTreeArtifact()); Preconditions.checkState(!artifact.isMiddlemanArtifact()); - if (value != FileArtifactValue.PLACEHOLDER) { + if (value != ArtifactFileMetadata.PLACEHOLDER) { return value; } @@ -275,8 +279,11 @@ private FileArtifactValue transformIfPlaceholder(Artifact artifact, FileArtifact additionalOutputData.get(artifact), "Placeholder without corresponding FileArtifactValue for: %s", artifact); - - return metadata; + FileStateValue.RegularFileStateValue fileStateValue = + new FileStateValue.RegularFileStateValue( + metadata.getSize(), metadata.getDigest(), /*contentsProxy=*/ null); + PathFragment pathFragment = artifact.getPath().asFragment(); + return ArtifactFileMetadata.forRegularFile(pathFragment, fileStateValue); } /** @@ -286,7 +293,7 @@ private FileArtifactValue transformIfPlaceholder(Artifact artifact, FileArtifact private static final class CrossServerUnshareableActionExecutionValue extends ActionExecutionValue { CrossServerUnshareableActionExecutionValue( - Map artifactData, + Map artifactData, Map treeArtifactData, Map additionalOutputData, @Nullable ImmutableList outputSymlinks, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 97ce7c6d3291f6..4363a68ba8bf1b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -27,10 +27,10 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.ArtifactFileMetadata; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; -import com.google.devtools.build.lib.actions.FileStateType; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.cache.DigestUtils; import com.google.devtools.build.lib.actions.cache.Md5Digest; @@ -218,7 +218,7 @@ public FileArtifactValue getMetadata(ActionInput actionInput) throws IOException // Check for existing metadata. It may have been injected. In either case, this method is called // from SkyframeActionExecutor to make sure that we have metadata for all action outputs, as the // results are then stored in Skyframe (and the action cache). - FileArtifactValue fileMetadata = store.getArtifactData(artifact); + ArtifactFileMetadata fileMetadata = store.getArtifactData(artifact); if (fileMetadata != null) { // Non-middleman artifacts should only have additionalOutputData if they have // outputArtifactData. We don't assert this because of concurrency possibilities, but at least @@ -229,10 +229,10 @@ public FileArtifactValue getMetadata(ActionInput actionInput) throws IOException if (value != null) { return metadataFromValue(value); } - if (fileMetadata.getType() == FileStateType.NONEXISTENT) { + if (!fileMetadata.exists()) { throw new FileNotFoundException(artifact.prettyPrint() + " does not exist"); } - return fileMetadata; + return FileArtifactValue.createFromMetadata(fileMetadata, !artifact.isConstantMetadata()); } // No existing metadata; this can happen if the output metadata is not injected after a spawn @@ -247,7 +247,7 @@ public FileArtifactValue getMetadata(ActionInput actionInput) throws IOException // // We only cache nonexistence here, not file system errors. It is unlikely that the file will be // requested from this cache too many times. - fileMetadata = constructFileArtifactValue(artifact, /*statNoFollow=*/ null); + fileMetadata = constructArtifactFileMetadata(artifact, /*statNoFollow=*/ null); return maybeStoreAdditionalData(artifact, fileMetadata, null); } @@ -261,22 +261,22 @@ public ActionInput getInput(String execPath) { * additional data, even for normal (non-middleman) artifacts. */ private FileArtifactValue maybeStoreAdditionalData( - Artifact artifact, FileArtifactValue data, @Nullable byte[] injectedDigest) + Artifact artifact, ArtifactFileMetadata data, @Nullable byte[] injectedDigest) throws IOException { - if (data.getType() == FileStateType.NONEXISTENT) { + if (!data.exists()) { // Nonexistent files should only occur before executing an action. throw new FileNotFoundException(artifact.prettyPrint() + " does not exist"); } - boolean isFile = data.getType() == FileStateType.REGULAR_FILE; + boolean isFile = data.isFile(); if (isFile && !artifact.hasParent() && data.getDigest() != null) { // We do not need to store the FileArtifactValue separately -- the digest is in the file value // and that is all that is needed for this file's metadata. - return data; + return FileArtifactValue.createFromMetadata(data, !artifact.isConstantMetadata()); } final FileArtifactValue value; - if (data.getType() == FileStateType.DIRECTORY) { + if (data.isDirectory()) { // This branch is taken when the output of an action is a directory: // - A Fileset (in this case, Blaze is correct) // - A directory someone created in a local action (in this case, changes under the @@ -382,14 +382,14 @@ private TreeArtifactValue constructTreeArtifactValue(Collection { - private static final LoadingCache keyCache = - CacheBuilder.newBuilder() - .weakKeys() - .concurrencyLevel(BlazeInterners.concurrencyLevel()) - .build(CacheLoader.from(SketchKey::new)); - - private SketchKey(ActionLookupData arg) { - super(arg); - } - - @AutoCodec.VisibleForSerialization - @AutoCodec.Instantiator - static SketchKey create(ActionLookupData arg) { - return keyCache.getUnchecked(arg); - } - - @Override - public SkyFunctionName functionName() { - return SkyFunctions.ACTION_SKETCH; - } - } - - @Nullable - @Override - public String extractTag(SkyKey skyKey) { - return null; - } - - @Nullable - @Override - public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { - ActionLookupData actionLookupData = (ActionLookupData) skyKey.argument(); - ActionLookupValue actionLookupValue = - ArtifactFunction.getActionLookupValue(actionLookupData.getActionLookupKey(), env); - if (actionLookupValue == null) { - return null; - } - - Action action = actionLookupValue.getAction(actionLookupData.getActionIndex()); - List srcArtifacts = new ArrayList<>(); - List depActions = new ArrayList<>(); - for (Artifact artifact : action.getInputs()) { - if (artifact.isSourceArtifact()) { - srcArtifacts.add(artifact); - } else { - depActions.add(SketchKey.create(((DerivedArtifact) artifact).getGeneratingActionKey())); - } - } - - Map srcArtifactValues = env.getValues(srcArtifacts); - Map depSketchValues = env.getValues(depActions); - if (env.valuesMissing()) { - return null; - } - - BigInteger transitiveActionKeyHash = Sketches.computeActionKey(action, actionKeyContext); - BigInteger transitiveSourceHash = BigInteger.ZERO; - - // Incorporate the direct source values. - for (SkyValue val : srcArtifactValues.values()) { - FileArtifactValue fileArtifactValue = (FileArtifactValue) val; - transitiveSourceHash = - BigIntegerFingerprintUtils.compose( - transitiveSourceHash, fileArtifactValue.getValueFingerprint()); - } - - // Incorporate the transitive action key and source values. - for (SkyValue sketchVal : depSketchValues.values()) { - ActionSketch depSketch = (ActionSketch) sketchVal; - transitiveActionKeyHash = - BigIntegerFingerprintUtils.compose( - transitiveActionKeyHash, depSketch.transitiveActionLookupHash()); - transitiveSourceHash = - BigIntegerFingerprintUtils.compose( - transitiveSourceHash, depSketch.transitiveSourceHash()); - } - - return ActionSketch.builder() - .setTransitiveActionLookupHash(transitiveActionKeyHash) - .setTransitiveSourceHash(transitiveSourceHash) - .build(); - } -} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java index 0e32d6fcfb4050..d66c1504b6325f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java @@ -27,9 +27,9 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.ArtifactFileMetadata; import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.FileArtifactValue; -import com.google.devtools.build.lib.actions.FileStateType; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.actions.FilesetTraversalParams.DirectTraversalRoot; import com.google.devtools.build.lib.actions.FilesetTraversalParams.PackageBoundaryMode; @@ -294,18 +294,14 @@ static FileArtifactValue createSimpleFileArtifactValue( if (value != null) { return value; } - FileArtifactValue data = + ArtifactFileMetadata data = Preconditions.checkNotNull(actionValue.getData(artifact), "%s %s", artifact, actionValue); Preconditions.checkNotNull( data.getDigest(), "Digest should already have been calculated for %s (%s)", artifact, data); // Directories are special-cased because their mtimes are used, so should have been constructed // during execution of the action (in ActionMetadataHandler#maybeStoreAdditionalData). - Preconditions.checkState( - data.getType() == FileStateType.REGULAR_FILE, - "Unexpected not file %s (%s)", - artifact, - data); - return data; + Preconditions.checkState(data.isFile(), "Unexpected not file %s (%s)", artifact, data); + return FileArtifactValue.createFromMetadata(data, !artifact.isConstantMetadata()); } @Nullable diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java index 40bf9fde0bd1ac..d880c6450964ec 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java @@ -25,8 +25,8 @@ import com.google.common.collect.Sets; import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.ArtifactFileMetadata; import com.google.devtools.build.lib.actions.FileArtifactValue; -import com.google.devtools.build.lib.actions.FileStateType; import com.google.devtools.build.lib.concurrent.ExecutorUtil; import com.google.devtools.build.lib.concurrent.Sharder; import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrapper; @@ -304,10 +304,10 @@ public void run() { Pair keyAndValue = fileToKeyAndValue.get(artifact); ActionExecutionValue actionValue = keyAndValue.getSecond(); SkyKey key = keyAndValue.getFirst(); - FileArtifactValue lastKnownData = actionValue.getAllFileValues().get(artifact); + ArtifactFileMetadata lastKnownData = actionValue.getAllFileValues().get(artifact); try { - FileArtifactValue newData = - ActionMetadataHandler.fileArtifactValueFromArtifact(artifact, stat, tsgm); + ArtifactFileMetadata newData = + ActionMetadataHandler.fileMetadataFromArtifact(artifact, stat, tsgm); if (!newData.equals(lastKnownData)) { updateIntraBuildModifiedCounter(stat != null ? stat.getLastChangeTime() : -1); modifiedOutputFilesCounter.getAndIncrement(); @@ -403,22 +403,20 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(ActionExecutionValue act ImmutableSet knownModifiedOutputFiles, Supplier> sortedKnownModifiedOutputFiles) { boolean isDirty = false; - for (Map.Entry entry : actionValue.getAllFileValues().entrySet()) { + for (Map.Entry entry : + actionValue.getAllFileValues().entrySet()) { Artifact file = entry.getKey(); - FileArtifactValue lastKnownData = entry.getValue(); + ArtifactFileMetadata lastKnownData = entry.getValue(); if (shouldCheckFile(knownModifiedOutputFiles, file)) { try { - FileArtifactValue fileMetadata = - ActionMetadataHandler.fileArtifactValueFromArtifact(file, null, tsgm); + ArtifactFileMetadata fileMetadata = + ActionMetadataHandler.fileMetadataFromArtifact(file, null, tsgm); FileArtifactValue fileValue = actionValue.getArtifactValue(file); boolean lastSeenRemotely = (fileValue != null) && fileValue.isRemote(); - boolean trustRemoteValue = - fileMetadata.getType() == FileStateType.NONEXISTENT && lastSeenRemotely; + boolean trustRemoteValue = !fileMetadata.exists() && lastSeenRemotely; if (!trustRemoteValue && !fileMetadata.equals(lastKnownData)) { updateIntraBuildModifiedCounter( - fileMetadata.getType() != FileStateType.NONEXISTENT - ? file.getPath().getLastModifiedTime(Symlinks.FOLLOW) - : -1); + fileMetadata.exists() ? file.getPath().getLastModifiedTime(Symlinks.FOLLOW) : -1); modifiedOutputFilesCounter.getAndIncrement(); isDirty = true; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java index 0c9b9ca6dfbbb2..7f07c14acade7d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/MinimalOutputStore.java @@ -15,6 +15,7 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.ArtifactFileMetadata; import com.google.devtools.build.lib.actions.FileArtifactValue; /** @@ -34,7 +35,7 @@ void putAdditionalOutputData(Artifact artifact, FileArtifactValue value) { } @Override - void putArtifactData(Artifact artifact, FileArtifactValue value) {} + void putArtifactData(Artifact artifact, ArtifactFileMetadata value) {} @Override void addTreeArtifactContents(Artifact artifact, TreeFileArtifact contents) {} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java index ed034bdda6503b..60a0ef297916b8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/OutputStore.java @@ -18,6 +18,7 @@ import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.ArtifactFileMetadata; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import java.util.Set; @@ -39,7 +40,8 @@ @ThreadSafe class OutputStore { - private final ConcurrentMap artifactData = new ConcurrentHashMap<>(); + private final ConcurrentMap artifactData = + new ConcurrentHashMap<>(); private final ConcurrentMap treeArtifactData = new ConcurrentHashMap<>(); @@ -53,21 +55,21 @@ class OutputStore { private final Set injectedFiles = Sets.newConcurrentHashSet(); @Nullable - final FileArtifactValue getArtifactData(Artifact artifact) { + final ArtifactFileMetadata getArtifactData(Artifact artifact) { return artifactData.get(artifact); } - void putArtifactData(Artifact artifact, FileArtifactValue value) { + void putArtifactData(Artifact artifact, ArtifactFileMetadata value) { artifactData.put(artifact, value); } /** * Returns data for output files that was computed during execution. * - *

    The value is {@link FileArtifactValue#PLACEHOLDER} if the artifact's metadata is not fully - * captured in {@link #additionalOutputData}. + *

    The value is {@link ArtifactFileMetadata#PLACEHOLDER} if the artifact's metadata is not + * fully captured in {@link #additionalOutputData}. */ - final ImmutableMap getAllArtifactData() { + final ImmutableMap getAllArtifactData() { return ImmutableMap.copyOf(artifactData); } @@ -109,7 +111,7 @@ void putAdditionalOutputData(Artifact artifact, FileArtifactValue value) { *

  • If the filesystem does not possess fast digests, then we will have additional output data * for practically every artifact, since we will need to store their digests. *
  • If we have a remote execution service injecting metadata, then we will just store that - * metadata here, and put {@link FileArtifactValue#PLACEHOLDER} objects into {@link + * metadata here, and put {@link ArtifactFileMetadata#PLACEHOLDER} objects into {@link * #outputArtifactData} if the filesystem supports fast digests, and the actual metadata if * the filesystem does not support fast digests. *
  • If the filesystem has fast digests but there is no remote execution injecting @@ -121,13 +123,13 @@ void putAdditionalOutputData(Artifact artifact, FileArtifactValue value) { * populated. Locally executed actions are the exception to this rule inside Google. * *

    Moreover, there are some artifacts that are always stored here. First, middleman artifacts - * have no corresponding {@link FileArtifactValue}. Second, directories' metadata contain their - * mtimes, which the {@link FileArtifactValue} does not expose, so that has to be stored + * have no corresponding {@link ArtifactFileMetadata}. Second, directories' metadata contain their + * mtimes, which the {@link ArtifactFileMetadata} does not expose, so that has to be stored * separately. * *

    Note that for files that need digests, we can't easily inject the digest in the {@link - * FileArtifactValue} because it would complicate equality-checking on subsequent builds -- if our - * filesystem doesn't do fast digests, the comparison value would not have a digest. + * ArtifactFileMetadata} because it would complicate equality-checking on subsequent builds -- if + * our filesystem doesn't do fast digests, the comparison value would not have a digest. */ final ImmutableMap getAllAdditionalOutputData() { return ImmutableMap.copyOf(additionalOutputData); @@ -165,7 +167,7 @@ final void injectOutputData(Artifact output, FileArtifactValue artifactValue) { // While `artifactValue` carries the important information, consumers also require an entry in // `artifactData` so a `PLACEHOLDER` is added to `artifactData`. - artifactData.put(output, FileArtifactValue.PLACEHOLDER); + artifactData.put(output, ArtifactFileMetadata.PLACEHOLDER); additionalOutputData.put(output, artifactValue); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java index fc4fc06451c8d9..12a8dab85f2b3c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java @@ -27,7 +27,6 @@ public final class SkyFunctions { SkyFunctionName.createNonHermetic("PRECOMPUTED"); public static final SkyFunctionName CLIENT_ENVIRONMENT_VARIABLE = SkyFunctionName.createNonHermetic("CLIENT_ENVIRONMENT_VARIABLE"); - static final SkyFunctionName ACTION_SKETCH = SkyFunctionName.createHermetic("ACTION_SKETCH"); public static final SkyFunctionName ACTION_ENVIRONMENT_VARIABLE = SkyFunctionName.createHermetic("ACTION_ENVIRONMENT_VARIABLE"); public static final SkyFunctionName DIRECTORY_LISTING_STATE = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index e9ce780fc9d86c..e8a340ed33b7d0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -148,7 +148,6 @@ enum ProgressEventBehavior { private ExtendedEventHandler progressSuppressingEventHandler; private ActionLogBufferPathGenerator actionLogBufferPathGenerator; private ActionCacheChecker actionCacheChecker; - @Nullable private TopDownActionCache topDownActionCache; private final Profiler profiler = Profiler.instance(); // We keep track of actions already executed this build in order to avoid executing a shared @@ -403,7 +402,6 @@ void prepareForExecution( Executor executor, OptionsProvider options, ActionCacheChecker actionCacheChecker, - TopDownActionCache topDownActionCache, OutputService outputService) { this.reporter = Preconditions.checkNotNull(reporter); this.executorEngine = Preconditions.checkNotNull(executor); @@ -415,7 +413,6 @@ void prepareForExecution( this.lostDiscoveredInputsMap = Maps.newConcurrentMap(); this.hadExecutionError = false; this.actionCacheChecker = Preconditions.checkNotNull(actionCacheChecker); - this.topDownActionCache = topDownActionCache; // Don't cache possibly stale data from the last build. this.options = options; // Cache some option values for performance, since we consult them on every action. @@ -482,7 +479,6 @@ void executionOver() { this.completedAndResetActions = null; this.lostDiscoveredInputsMap = null; this.actionCacheChecker = null; - this.topDownActionCache = null; } /** @@ -587,10 +583,6 @@ private ExtendedEventHandler selectEventHandler(ProgressEventBehavior progressEv : progressSuppressingEventHandler; } - TopDownActionCache getTopDownActionCache() { - return topDownActionCache; - } - /** * Returns an ActionExecutionContext suitable for executing a particular action. The caller should * pass the returned context to {@link #executeAction}, and any other method that needs to execute diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index c1d1747418d887..540fdd9226a932 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -607,7 +607,6 @@ private ImmutableMap skyFunctions(PackageFactory p new ActionExecutionFunction(skyframeActionExecutor, directories, tsgm); map.put(SkyFunctions.ACTION_EXECUTION, actionExecutionFunction); this.actionExecutionFunction = actionExecutionFunction; - map.put(SkyFunctions.ACTION_SKETCH, new ActionSketchFunction(actionKeyContext)); map.put( SkyFunctions.RECURSIVE_FILESYSTEM_TRAVERSAL, new RecursiveFilesystemTraversalFunction()); map.put(SkyFunctions.FILESET_ENTRY, new FilesetEntryFunction(directories::getExecRoot)); @@ -1543,7 +1542,6 @@ public EvaluationResult buildArtifacts( Set exclusiveTests, OptionsProvider options, ActionCacheChecker actionCacheChecker, - TopDownActionCache topDownActionCache, @Nullable EvaluationProgressReceiver executionProgressReceiver, TopLevelArtifactContext topLevelArtifactContext) throws InterruptedException { @@ -1553,7 +1551,7 @@ public EvaluationResult buildArtifacts( try (SilentCloseable c = Profiler.instance().profile("skyframeActionExecutor.prepareForExecution")) { skyframeActionExecutor.prepareForExecution( - reporter, executor, options, actionCacheChecker, topDownActionCache, outputService); + reporter, executor, options, actionCacheChecker, outputService); } resourceManager.resetResourceUsage(); @@ -1591,7 +1589,6 @@ public EvaluationResult runExclusiveTest( ConfiguredTarget exclusiveTest, OptionsProvider options, ActionCacheChecker actionCacheChecker, - TopDownActionCache topDownActionCache, @Nullable EvaluationProgressReceiver executionProgressReceiver, TopLevelArtifactContext topLevelArtifactContext) throws InterruptedException { @@ -1601,7 +1598,7 @@ public EvaluationResult runExclusiveTest( try (SilentCloseable c = Profiler.instance().profile("skyframeActionExecutor.prepareForExecution")) { skyframeActionExecutor.prepareForExecution( - reporter, executor, options, actionCacheChecker, topDownActionCache, outputService); + reporter, executor, options, actionCacheChecker, outputService); } resourceManager.resetResourceUsage(); @@ -1625,13 +1622,8 @@ public EvaluationResult runExclusiveTest( @VisibleForTesting public void prepareBuildingForTestingOnly( - Reporter reporter, - Executor executor, - OptionsProvider options, - ActionCacheChecker checker, - TopDownActionCache topDownActionCache) { - skyframeActionExecutor.prepareForExecution( - reporter, executor, options, checker, topDownActionCache, outputService); + Reporter reporter, Executor executor, OptionsProvider options, ActionCacheChecker checker) { + skyframeActionExecutor.prepareForExecution(reporter, executor, options, checker, outputService); } EvaluationResult targetPatterns( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TopDownActionCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/TopDownActionCache.java deleted file mode 100644 index a508e257b8804b..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TopDownActionCache.java +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright 2019 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.skyframe; - -import com.google.devtools.build.lib.actionsketch.ActionSketch; -import javax.annotation.Nullable; - -/** - * A top-down action cache is a cache of {@link ActionSketch} to {@link ActionExecutionValue}. - * - *

    Unlike {@link com.google.devtools.build.lib.actions.ActionCacheChecker}, a top-down cache can - * cull large subgraphs by computing the transitive cache key (known as the {@link ActionSketch}). - */ -public interface TopDownActionCache { - - /** Retrieves the cached value for the given action sketch, or null. */ - @Nullable - ActionExecutionValue get(ActionSketch sketch); - - /** Puts the sketch into the top-down cache. May complete asynchronously. */ - void put(ActionSketch sketch, ActionExecutionValue value); -} diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/platform/PlatformInfoApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/platform/PlatformInfoApi.java index c9a7f1033e025d..379152d3a7e486 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/platform/PlatformInfoApi.java +++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/platform/PlatformInfoApi.java @@ -20,7 +20,6 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; import com.google.devtools.build.lib.syntax.StarlarkSemantics.FlagIdentifier; -import java.util.Map; /** Info object representing data about a specific platform. */ @SkylarkModule( @@ -60,11 +59,4 @@ public interface PlatformInfoApi< structField = true, enableOnlyWithFlag = FlagIdentifier.EXPERIMENTAL_PLATFORM_API) String remoteExecutionProperties(); - - @SkylarkCallable( - name = "exec_properties", - doc = "Properties to configure a remote execution platform.", - structField = true, - enableOnlyWithFlag = FlagIdentifier.EXPERIMENTAL_PLATFORM_API) - Map execProperties(); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java index 64701b5ef7d1e9..07c00f10e84894 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java @@ -133,7 +133,7 @@ public static Object eval(Object objValue, String name, if (objValue instanceof SkylarkClassObject) { try { - return ((SkylarkClassObject) objValue).getValue(loc, env.getSemantics(), name); + return ((SkylarkClassObject) objValue).getValue(name); } catch (IllegalArgumentException ex) { throw new EvalException(loc, ex); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkClassObject.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkClassObject.java index 64e6fc6b04c246..03c9f88208fac1 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkClassObject.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkClassObject.java @@ -13,9 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.syntax; -import com.google.devtools.build.lib.events.Location; -import javax.annotation.Nullable; - /** * A marker interface for a {@link ClassObject} whose {@link #getValue} always returns a * Skylark-friendly value, with no defensive conversion required. @@ -29,17 +26,4 @@ * Skylark-friendly values. */ public interface SkylarkClassObject extends ClassObject { - - /** - * Returns the value of the field with the given name, or null if the field does not exist. - * - * @param loc the location in the current Starlark evaluation context - * @param starlarkSemantics the current starlark semantics, which may affect which fields are - * available, or the semantics of the available fields - * @param name the name of the field to return the value of - * @throws EvalException if a user-visible error occurs (other than non-existent field). - */ - @Nullable - Object getValue(Location loc, StarlarkSemantics starlarkSemantics, String name) - throws EvalException; } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java index c3e1143d0f42fd..7527c6c9273c6e 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java @@ -148,8 +148,6 @@ public boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean incompatibleDepsetUnion(); - public abstract boolean incompatibleDisableTargetProviderFields(); - public abstract boolean incompatibleDisableThirdPartyLicenseChecking(); public abstract boolean incompatibleDisableDeprecatedAttrParams(); @@ -265,7 +263,6 @@ public static Builder builderWithDefaults() { .incompatibleBzlDisallowLoadAfterStatement(true) .incompatibleDepsetIsNotIterable(true) .incompatibleDepsetUnion(true) - .incompatibleDisableTargetProviderFields(false) .incompatibleDisableThirdPartyLicenseChecking(true) .incompatibleDisableDeprecatedAttrParams(true) .incompatibleDisableObjcProviderResources(true) @@ -334,8 +331,6 @@ public abstract static class Builder { public abstract Builder incompatibleDepsetUnion(boolean value); - public abstract Builder incompatibleDisableTargetProviderFields(boolean value); - public abstract Builder incompatibleDisableThirdPartyLicenseChecking(boolean value); public abstract Builder incompatibleDisableDeprecatedAttrParams(boolean value); diff --git a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java index a31fce035c124f..9045fd456e77b8 100644 --- a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java +++ b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java @@ -112,16 +112,20 @@ import com.google.devtools.build.skydoc.fakebuildapi.test.FakeTestingModule; import com.google.devtools.build.skydoc.rendering.AspectInfoWrapper; import com.google.devtools.build.skydoc.rendering.DocstringParseException; +import com.google.devtools.build.skydoc.rendering.FunctionUtil; +import com.google.devtools.build.skydoc.rendering.MarkdownRenderer; import com.google.devtools.build.skydoc.rendering.ProtoRenderer; import com.google.devtools.build.skydoc.rendering.ProviderInfoWrapper; import com.google.devtools.build.skydoc.rendering.RuleInfoWrapper; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.AspectInfo; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.ProviderInfo; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.RuleInfo; +import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.UserDefinedFunctionInfo; import com.google.devtools.common.options.OptionsParser; import java.io.BufferedOutputStream; import java.io.FileOutputStream; import java.io.IOException; +import java.io.PrintWriter; import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.nio.file.Paths; @@ -161,6 +165,16 @@ public class SkydocMain { private final SkylarkFileAccessor fileAccessor; private final List depRoots; private final String workspaceName; + private static final String HEADER_TEMPLATE_PATH = + "com/google/devtools/build/skydoc/rendering/templates/header.vm"; + private static final String RULE_TEMPLATE_PATH = + "com/google/devtools/build/skydoc/rendering/templates/rule.vm"; + private static final String PROVIDER_TEMPLATE_PATH = + "com/google/devtools/build/skydoc/rendering/templates/provider.vm"; + private static final String FUNCTION_TEMPLATE_PATH = + "com/google/devtools/build/skydoc/rendering/templates/func.vm"; + private static final String ASPECT_TEMPLATE_PATH = + "com/google/devtools/build/skydoc/rendering/templates/aspect.vm"; public SkydocMain(SkylarkFileAccessor fileAccessor, String workspaceName, List depRoots) { this.fileAccessor = fileAccessor; @@ -251,7 +265,19 @@ public static void main(String[] args) .writeModuleInfo(out); } } else if (skydocOptions.outputFormat == OutputFormat.MARKDOWN) { - throw new IllegalArgumentException("Skydoc Binary only outputs raw proto format."); + MarkdownRenderer renderer = + new MarkdownRenderer( + HEADER_TEMPLATE_PATH, + RULE_TEMPLATE_PATH, + PROVIDER_TEMPLATE_PATH, + FUNCTION_TEMPLATE_PATH, + ASPECT_TEMPLATE_PATH); + try (PrintWriter printWriter = new PrintWriter(outputPath, "UTF-8")) { + printWriter.println(renderer.renderMarkdownHeader()); + printRuleInfos(printWriter, renderer, filteredRuleInfos); + printProviderInfos(printWriter, renderer, filteredProviderInfos); + printUserDefinedFunctions(printWriter, renderer, filteredUserDefinedFunctions); + } } } @@ -268,6 +294,64 @@ private static boolean validSymbolName(ImmutableSet symbolNames, String return false; } + private static void printRuleInfos( + PrintWriter printWriter, MarkdownRenderer renderer, Map ruleInfos) + throws IOException { + for (Entry ruleInfoEntry : ruleInfos.entrySet()) { + printRuleInfo(printWriter, renderer, ruleInfoEntry.getKey(), ruleInfoEntry.getValue()); + printWriter.println(); + } + } + + private static void printProviderInfos( + PrintWriter printWriter, MarkdownRenderer renderer, Map providerInfos) + throws IOException { + for (Entry entry : providerInfos.entrySet()) { + ProviderInfo providerInfo = entry.getValue(); + printProviderInfo(printWriter, renderer, entry.getKey(), providerInfo); + printWriter.println(); + } + } + + private static void printUserDefinedFunctions( + PrintWriter printWriter, + MarkdownRenderer renderer, + Map userDefinedFunctions) + throws IOException { + + for (Entry entry : userDefinedFunctions.entrySet()) { + try { + UserDefinedFunctionInfo functionInfo = + FunctionUtil.fromNameAndFunction(entry.getKey(), entry.getValue()); + printUserDefinedFunctionInfo(printWriter, renderer, functionInfo); + printWriter.println(); + } catch (DocstringParseException exception) { + System.err.println(exception.getMessage()); + System.err.println(); + } + } + } + + private static void printRuleInfo( + PrintWriter printWriter, MarkdownRenderer renderer, String exportedName, RuleInfo ruleInfo) + throws IOException { + printWriter.println(renderer.render(exportedName, ruleInfo)); + } + + private static void printProviderInfo( + PrintWriter printWriter, + MarkdownRenderer renderer, + String exportedName, + ProviderInfo providerInfo) + throws IOException { + printWriter.println(renderer.render(exportedName, providerInfo)); + } + + private static void printUserDefinedFunctionInfo( + PrintWriter printWriter, MarkdownRenderer renderer, UserDefinedFunctionInfo functionInfo) + throws IOException { + printWriter.println(renderer.render(functionInfo)); + } /** * Evaluates/interprets the skylark file at a given path and its transitive skylark dependencies diff --git a/src/main/java/com/google/devtools/build/skydoc/SkydocOptions.java b/src/main/java/com/google/devtools/build/skydoc/SkydocOptions.java index 900355a317f874..3cddfecc486f47 100644 --- a/src/main/java/com/google/devtools/build/skydoc/SkydocOptions.java +++ b/src/main/java/com/google/devtools/build/skydoc/SkydocOptions.java @@ -50,7 +50,7 @@ public class SkydocOptions extends OptionsBase { @Option( name = "output_format", - defaultValue = "proto", + defaultValue = "markdown", converter = OutputFormatConverter.class, documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, effectTags = OptionEffectTag.UNKNOWN, diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java index c67e7a12e3f43d..5402175ca6af55 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java @@ -192,21 +192,6 @@ public boolean wasModifiedSinceDigest(Path path) { throw new UnsupportedOperationException(); } - @Override - public boolean isRemote() { - throw new UnsupportedOperationException(); - } - - @Override - public boolean isMarkerValue() { - throw new UnsupportedOperationException(); - } - - @Override - public FileContentsProxy getContentsProxy() { - throw new UnsupportedOperationException(); - } - @Override @SuppressWarnings("EqualsHashCode") public boolean equals(Object o) { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java index 45df840a9af984..985fc272441b19 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/platform/PlatformUtilsTest.java @@ -17,7 +17,6 @@ import static com.google.common.truth.Truth.assertThat; import build.bazel.remote.execution.v2.Platform; -import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.remote.options.RemoteOptions; import org.junit.Test; import org.junit.runner.RunWith; @@ -26,9 +25,9 @@ /** Tests for {@link PlatformUtils } */ @RunWith(JUnit4.class) public final class PlatformUtilsTest { - private static RemoteOptions remoteOptions() { - RemoteOptions remoteOptions = new RemoteOptions(); - remoteOptions.remoteDefaultPlatformProperties = + @Test + public void testParsePlatformSortsProperties() throws Exception { + String s = String.join( "\n", "properties: {", @@ -39,38 +38,19 @@ private static RemoteOptions remoteOptions() { " name: \"a\"", " value: \"1\"", "}"); + RemoteOptions remoteOptions = new RemoteOptions(); + remoteOptions.remoteDefaultPlatformProperties = s; - return remoteOptions; - } - - @Test - public void testParsePlatformSortsProperties() throws Exception { Platform expected = Platform.newBuilder() .addProperties(Platform.Property.newBuilder().setName("a").setValue("1")) .addProperties(Platform.Property.newBuilder().setName("b").setValue("2")) .build(); - assertThat(PlatformUtils.getPlatformProto(null, remoteOptions())).isEqualTo(expected); + assertThat(PlatformUtils.getPlatformProto(null, remoteOptions)).isEqualTo(expected); } @Test public void testParsePlatformHandlesNull() throws Exception { assertThat(PlatformUtils.getPlatformProto(null, null)).isEqualTo(null); } - - @Test - public void testParsePlatformSortsProperties_ExecProperties() throws Exception { - // execProperties are chosen even if there are remoteOptions - ImmutableMap map = ImmutableMap.of("aa", "99", "zz", "66", "dd", "11"); - PlatformInfo platformInfo = PlatformInfo.builder().setExecProperties(map).build(); - - Platform expected = - Platform.newBuilder() - .addProperties(Platform.Property.newBuilder().setName("aa").setValue("99")) - .addProperties(Platform.Property.newBuilder().setName("dd").setValue("11")) - .addProperties(Platform.Property.newBuilder().setName("zz").setValue("66")) - .build(); - // execProperties are sorted by key - assertThat(PlatformUtils.getPlatformProto(platformInfo, remoteOptions())).isEqualTo(expected); - } } diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java index a15435486ceedf..e77ed2d97f5cd4 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java @@ -138,7 +138,6 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E "--incompatible_depset_for_libraries_to_link_getter=" + rand.nextBoolean(), "--incompatible_depset_is_not_iterable=" + rand.nextBoolean(), "--incompatible_depset_union=" + rand.nextBoolean(), - "--incompatible_disable_target_provider_fields=" + rand.nextBoolean(), "--incompatible_disable_deprecated_attr_params=" + rand.nextBoolean(), "--incompatible_disable_objc_provider_resources=" + rand.nextBoolean(), "--incompatible_disable_third_party_license_checking=" + rand.nextBoolean(), @@ -197,7 +196,6 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .incompatibleDepsetForLibrariesToLinkGetter(rand.nextBoolean()) .incompatibleDepsetIsNotIterable(rand.nextBoolean()) .incompatibleDepsetUnion(rand.nextBoolean()) - .incompatibleDisableTargetProviderFields(rand.nextBoolean()) .incompatibleDisableDeprecatedAttrParams(rand.nextBoolean()) .incompatibleDisableObjcProviderResources(rand.nextBoolean()) .incompatibleDisableThirdPartyLicenseChecking(rand.nextBoolean()) diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/BazelMockPythonSupport.java b/src/test/java/com/google/devtools/build/lib/packages/util/BazelMockPythonSupport.java index 70c94d8e6cbbdb..d4d0a55a4dbc4a 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/util/BazelMockPythonSupport.java +++ b/src/test/java/com/google/devtools/build/lib/packages/util/BazelMockPythonSupport.java @@ -30,6 +30,8 @@ private static void addTool(MockToolsConfig config, String toolRelativePath) thr @Override public void setup(MockToolsConfig config) throws IOException { + writeMacroFile(config); + addTool(config, "tools/python/python_version.bzl"); addTool(config, "tools/python/srcs_version.bzl"); addTool(config, "tools/python/toolchain.bzl"); @@ -39,6 +41,7 @@ public void setup(MockToolsConfig config) throws IOException { config.create( TestConstants.TOOLS_REPOSITORY_SCRATCH + "tools/python/BUILD", "package(default_visibility=['//visibility:public'])", + getMacroLoadStatement(), "load(':python_version.bzl', 'define_python_version_flag')", "load('//tools/python:toolchain.bzl', 'py_runtime_pair')", "define_python_version_flag(", diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/MockPythonSupport.java b/src/test/java/com/google/devtools/build/lib/packages/util/MockPythonSupport.java index b08677847fdfd3..794ee288936e06 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/util/MockPythonSupport.java +++ b/src/test/java/com/google/devtools/build/lib/packages/util/MockPythonSupport.java @@ -14,7 +14,12 @@ package com.google.devtools.build.lib.packages.util; +import com.google.common.base.Joiner; +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.testutil.TestConstants; import java.io.IOException; +import java.util.List; /** * Creates mock BUILD files required for the python rules. @@ -31,4 +36,58 @@ public abstract class MockPythonSupport { */ public abstract String createPythonTopEntryPoint(MockToolsConfig config, String pyRuntimeLabel) throws IOException; + + /** + * Defines a file simulating the part of @rules_python//python:defs.bzl that defines macros for + * native rules. + */ + public void writeMacroFile(MockToolsConfig config) throws IOException { + List ruleNames = ImmutableList.of("py_library", "py_binary", "py_test", "py_runtime"); + config.create(TestConstants.TOOLS_REPOSITORY_SCRATCH + "third_party/py_rules/macros/BUILD", ""); + + StringBuilder macros = new StringBuilder(); + for (String ruleName : ruleNames) { + Joiner.on("\n") + .appendTo( + macros, + "def " + ruleName + "(**attrs):", + " if 'tags' in attrs and attrs['tags'] != None:", + " attrs['tags'] = attrs['tags'] +" + + " ['__PYTHON_RULES_MIGRATION_DO_NOT_USE_WILL_BREAK__']", + " else:", + " attrs['tags'] = ['__PYTHON_RULES_MIGRATION_DO_NOT_USE_WILL_BREAK__']", + " native." + ruleName + "(**attrs)"); + macros.append("\n"); + } + config.create( + TestConstants.TOOLS_REPOSITORY_SCRATCH + "third_party/py_rules/macros/defs.bzl", + macros.toString()); + } + + /** + * Returns a line loading the proper wrapper macro (which adds the magic tag required by {@code + * --incompatible_load_python_rules_from_bzl}) for each of the given {@code ruleNames}. + * + *

    Otherwise returns the empty string. + */ + public static String getMacroLoadStatementFor(String... ruleNames) { + Preconditions.checkState(ruleNames.length > 0); + StringBuilder loadStatement = + new StringBuilder() + .append("load('") + .append(TestConstants.TOOLS_REPOSITORY) + .append("//third_party/py_rules/macros:defs.bzl', "); + ImmutableList.Builder quotedRuleNames = ImmutableList.builder(); + for (String ruleName : ruleNames) { + quotedRuleNames.add(String.format("'%s'", ruleName)); + } + Joiner.on(",").appendTo(loadStatement, quotedRuleNames.build()); + loadStatement.append(")"); + return loadStatement.toString(); + } + + /** Same as {@link #getMacroLoadStatementFor}, but loads all applicable macros. */ + public static String getMacroLoadStatement() { + return getMacroLoadStatementFor("py_library", "py_binary", "py_test", "py_runtime"); + } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java index a33ebdb7b1fe86..014145a4069882 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java @@ -443,6 +443,73 @@ public void testDownloadDirectoryNested() throws Exception { assertThat(execRoot.getRelative("a/bar/wobble/qux").isExecutable()).isFalse(); } + @Test + public void testUploadBlobCacheHitWithRetries() throws Exception { + final GrpcRemoteCache client = newClient(); + final Digest digest = DIGEST_UTIL.computeAsUtf8("abcdefg"); + serviceRegistry.addService( + new ContentAddressableStorageImplBase() { + private int numErrors = 4; + + @Override + public void findMissingBlobs( + FindMissingBlobsRequest request, + StreamObserver responseObserver) { + if (numErrors-- <= 0) { + responseObserver.onNext(FindMissingBlobsResponse.getDefaultInstance()); + responseObserver.onCompleted(); + } else { + responseObserver.onError(Status.UNAVAILABLE.asRuntimeException()); + } + } + }); + assertThat(client.uploadBlob("abcdefg".getBytes(UTF_8))).isEqualTo(digest); + } + + @Test + public void testUploadBlobSingleChunk() throws Exception { + final GrpcRemoteCache client = newClient(); + final Digest digest = DIGEST_UTIL.computeAsUtf8("abcdefg"); + serviceRegistry.addService( + new ContentAddressableStorageImplBase() { + @Override + public void findMissingBlobs( + FindMissingBlobsRequest request, + StreamObserver responseObserver) { + responseObserver.onNext( + FindMissingBlobsResponse.newBuilder().addMissingBlobDigests(digest).build()); + responseObserver.onCompleted(); + } + }); + serviceRegistry.addService( + new ByteStreamImplBase() { + @Override + public StreamObserver write( + final StreamObserver responseObserver) { + return new StreamObserver() { + @Override + public void onNext(WriteRequest request) { + assertThat(request.getResourceName()).contains(digest.getHash()); + assertThat(request.getFinishWrite()).isTrue(); + assertThat(request.getData().toStringUtf8()).isEqualTo("abcdefg"); + } + + @Override + public void onCompleted() { + responseObserver.onNext(WriteResponse.newBuilder().setCommittedSize(7).build()); + responseObserver.onCompleted(); + } + + @Override + public void onError(Throwable t) { + fail("An error occurred: " + t); + } + }; + } + }); + assertThat(client.uploadBlob("abcdefg".getBytes(UTF_8))).isEqualTo(digest); + } + static class TestChunkedRequestObserver implements StreamObserver { private final StreamObserver responseObserver; private final String contents; @@ -492,6 +559,46 @@ public void onError(Throwable t) { } } + private Answer> blobChunkedWriteAnswer( + final String contents, final int chunkSize) { + return new Answer>() { + @Override + @SuppressWarnings("unchecked") + public StreamObserver answer(InvocationOnMock invocation) { + return new TestChunkedRequestObserver( + (StreamObserver) invocation.getArguments()[0], contents, chunkSize); + } + }; + } + + @Test + public void testUploadBlobMultipleChunks() throws Exception { + final Digest digest = DIGEST_UTIL.computeAsUtf8("abcdef"); + serviceRegistry.addService( + new ContentAddressableStorageImplBase() { + @Override + public void findMissingBlobs( + FindMissingBlobsRequest request, + StreamObserver responseObserver) { + responseObserver.onNext( + FindMissingBlobsResponse.newBuilder().addMissingBlobDigests(digest).build()); + responseObserver.onCompleted(); + } + }); + + ByteStreamImplBase mockByteStreamImpl = Mockito.mock(ByteStreamImplBase.class); + serviceRegistry.addService(mockByteStreamImpl); + for (int chunkSize = 1; chunkSize <= 6; ++chunkSize) { + GrpcRemoteCache client = newClient(); + Chunker.setDefaultChunkSizeForTesting(chunkSize); + when(mockByteStreamImpl.write(ArgumentMatchers.>any())) + .thenAnswer(blobChunkedWriteAnswer("abcdef", chunkSize)); + assertThat(client.uploadBlob("abcdef".getBytes(UTF_8))).isEqualTo(digest); + } + Mockito.verify(mockByteStreamImpl, Mockito.times(6)) + .write(ArgumentMatchers.>any()); + } + @Test public void testUploadDirectory() throws Exception { final GrpcRemoteCache client = newClient(); diff --git a/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformInfoApiTest.java b/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformInfoApiTest.java index 39dc909c234866..a26fe81e24d944 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformInfoApiTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformInfoApiTest.java @@ -17,7 +17,6 @@ import static com.google.common.truth.Truth.assertThat; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.platform.ConstraintSettingInfo; import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; @@ -175,97 +174,4 @@ public void testPlatform_parent_tooManyParentsError() throws Exception { lines.toArray(new String[] {})); } - @Test - public void testPlatform_execProperties() throws Exception { - ImmutableMap props = ImmutableMap.of("k1", "v1", "k2", "v2"); - platformBuilder("//foo:my_platform").setExecProperties(props).write(); - assertNoEvents(); - - PlatformInfo platformInfo = fetchPlatformInfo("//foo:my_platform"); - assertThat(platformInfo).isNotNull(); - assertThat(platformInfo.execProperties()).isEqualTo(props); - } - - @Test - public void testPlatform_conflictingProperties_errorsOut() throws Exception { - ImmutableMap props = ImmutableMap.of("k1", "v1", "k2", "v2"); - PlatformBuilder builder = - platformBuilder("//foo:my_platform") - .setExecProperties(props) - .setRemoteExecutionProperties("child props"); - - checkError( - "foo", - "my_platform", - "Platform contains both remote_execution_properties and exec_properties. Prefer" - + " exec_properties over the deprecated remote_execution_properties.", - builder.lines().toArray(new String[] {})); - } - - @Test - public void testPlatform_execProperties_parent() throws Exception { - ImmutableMap props = ImmutableMap.of("k1", "v1", "k2", "v2"); - platformBuilder("//foo:parent_platform").setExecProperties(props).write(); - platformBuilder("//foo:my_platform").setParent("//foo:parent_platform").write(); - assertNoEvents(); - - PlatformInfo platformInfo = fetchPlatformInfo("//foo:my_platform"); - assertThat(platformInfo).isNotNull(); - assertThat(platformInfo.execProperties()).isEqualTo(props); - } - - @Test - public void testPlatform_execProperties_parent_mixed() throws Exception { - ImmutableMap propsParent = ImmutableMap.of("k1", "v1", "k2", "v2"); - ImmutableMap propsChild = ImmutableMap.of("k2", "child_v2", "k3", "child_v3"); - platformBuilder("//foo:parent_platform").setExecProperties(propsParent).write(); - platformBuilder("//foo:my_platform") - .setParent("//foo:parent_platform") - .setExecProperties(propsChild) - .write(); - assertNoEvents(); - - PlatformInfo platformInfo = fetchPlatformInfo("//foo:my_platform"); - assertThat(platformInfo).isNotNull(); - ImmutableMap expected = - ImmutableMap.of("k1", "v1", "k2", "child_v2", "k3", "child_v3"); - assertThat(platformInfo.execProperties()).isEqualTo(expected); - } - - @Test - public void testPlatform_execProperties_parentSpecifiesRemoteExecutionProperties_errorsOut() - throws Exception { - ImmutableMap propsParent = ImmutableMap.of("k1", "v1", "k2", "v2"); - platformBuilder("//foo:parent_platform").setExecProperties(propsParent).write(); - PlatformBuilder builder = - platformBuilder("//bar:my_platform") - .setParent("//foo:parent_platform") - .setRemoteExecutionProperties("properties"); - - checkError( - "bar", - "my_platform", - "Platform specifies remote_execution_properties but its parent specifies exec_properties." - + " Prefer exec_properties over the deprecated remote_execution_properties.", - builder.lines().toArray(new String[] {})); - } - - @Test - public void testPlatform_remoteExecutionProperties_parentSpecifiesExecProperties_errorsOut() - throws Exception { - ImmutableMap propsChild = ImmutableMap.of("k2", "child_v2", "k3", "child_v3"); - platformBuilder("//foo:parent_platform").setRemoteExecutionProperties("properties").write(); - PlatformBuilder builder = - platformBuilder("//bar:my_platform") - .setParent("//foo:parent_platform") - .setExecProperties(propsChild); - - checkError( - "bar", - "my_platform", - "Platform specifies exec_properties but its parent //foo:parent_platform specifies" - + " remote_execution_properties. Prefer exec_properties over the deprecated" - + " remote_execution_properties.", - builder.lines().toArray(new String[] {})); - } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformTestCase.java b/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformTestCase.java index d39f4c828e1579..3fdb163729127f 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformTestCase.java @@ -16,7 +16,6 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.platform.ConstraintSettingInfo; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; @@ -25,7 +24,6 @@ import com.google.devtools.build.lib.cmdline.Label; import java.util.ArrayList; import java.util.List; -import java.util.Map; import javax.annotation.Nullable; /** Base class for tests that want to use builders to create platforms and constraints. */ @@ -105,7 +103,6 @@ final class PlatformBuilder { private final List constraintValues = new ArrayList<>(); private Label parentLabel = null; private String remoteExecutionProperties = ""; - private ImmutableMap execProperties; public PlatformBuilder(String name) { this.label = Label.parseAbsoluteUnchecked(name); @@ -126,11 +123,6 @@ public PlatformBuilder setRemoteExecutionProperties(String value) { return this; } - public PlatformBuilder setExecProperties(ImmutableMap value) { - this.execProperties = value; - return this; - } - public List lines() { ImmutableList.Builder lines = ImmutableList.builder(); @@ -146,13 +138,6 @@ public List lines() { if (!Strings.isNullOrEmpty(remoteExecutionProperties)) { lines.add(" remote_execution_properties = '" + remoteExecutionProperties + "',"); } - if (execProperties != null && !execProperties.isEmpty()) { - lines.add(" exec_properties = { "); - for (Map.Entry entry : execProperties.entrySet()) { - lines.add(" \"" + entry.getKey() + "\": \"" + entry.getValue() + "\","); - } - lines.add(" }"); - } lines.add(")"); return lines.build(); diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/BUILD b/src/test/java/com/google/devtools/build/lib/rules/python/BUILD index e18a2ba308fe98..399f34415ad17a 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/python/BUILD @@ -72,6 +72,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib:build-base", "//src/main/java/com/google/devtools/build/lib:python-rules", "//src/test/java/com/google/devtools/build/lib:analysis_testutil", + "//src/test/java/com/google/devtools/build/lib:packages_testutil", "//third_party:junit4", "//third_party:truth", ], diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java index e4bcfbbdd25cd7..c7ba4da139e132 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java @@ -19,6 +19,7 @@ import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.packages.util.MockPythonSupport; import org.junit.Before; import org.junit.Test; @@ -271,4 +272,46 @@ public void requiresProvider() throws Exception { " deps = [':dep'],", ")"); } + + @Test + public void loadFromBzl_WithMagicTagPasses() throws Exception { + useConfiguration("--incompatible_load_python_rules_from_bzl=true"); + scratch.file( + "pkg/BUILD", + MockPythonSupport.getMacroLoadStatementFor(ruleName), + ruleName + "(", + " name = 'foo',", + " srcs = ['foo.py'],", + ")"); + assertThat(getConfiguredTarget("//pkg:foo")).isNotNull(); + assertNoEvents(); + } + + @Test + public void loadFromBzl_WithoutMagicTagFails() throws Exception { + useConfiguration("--incompatible_load_python_rules_from_bzl=true"); + checkError( + "pkg", + "foo", + // error: + "Direct access to the native Python rules is deprecated", + // build file: + ruleName + "(", + " name = 'foo',", + " srcs = ['foo.py'],", + ")"); + } + + @Test + public void loadFromBzl_WithoutFlagPasses() throws Exception { + useConfiguration("--incompatible_load_python_rules_from_bzl=false"); + scratch.file( + "pkg/BUILD", // + ruleName + "(", + " name = 'foo',", + " srcs = ['foo.py'],", + ")"); + assertThat(getConfiguredTarget("//pkg:foo")).isNotNull(); + assertNoEvents(); + } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/test/TestTargetPropertiesTest.java b/src/test/java/com/google/devtools/build/lib/rules/test/TestTargetPropertiesTest.java index bdb86e1c90a8f1..5571f5fcde4ec7 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/test/TestTargetPropertiesTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/test/TestTargetPropertiesTest.java @@ -16,7 +16,6 @@ import static com.google.common.truth.Truth.assertThat; -import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.test.TestProvider; @@ -51,23 +50,4 @@ public void testTestWithCpusTagHasCorrectLocalResourcesEstimate() throws Excepti .getLocalResourceUsage(testAction.getOwner().getLabel(), false); assertThat(localResourceUsage.getCpuUsage()).isEqualTo(4.0); } - - @Test - public void testTestWithExclusiveDisablesRemoteExecution() throws Exception { - scratch.file("tests/test.sh", "#!/bin/bash", "exit 0"); - scratch.file( - "tests/BUILD", - "sh_test(", - " name = 'test',", - " size = 'small',", - " srcs = ['test.sh'],", - " tags = ['exclusive'],", - ")"); - ConfiguredTarget testTarget = getConfiguredTarget("//tests:test"); - TestRunnerAction testAction = - (TestRunnerAction) - getGeneratingAction(TestProvider.getTestStatusArtifacts(testTarget).get(0)); - assertThat(testAction.getExecutionInfo()).containsKey(ExecutionRequirements.NO_REMOTE_EXEC); - assertThat(testAction.getExecutionInfo()).hasSize(1); - } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java index 9b1e112282d6c8..07a020b7352d00 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.ArtifactFileMetadata; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.BasicActionLookupValue; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -262,8 +263,8 @@ public void actionExecutionValueSerialization() throws Exception { ActionLookupData dummyData = ActionLookupData.create(ALL_OWNER, 0); Artifact.DerivedArtifact artifact1 = createDerivedArtifact("one"); Artifact.DerivedArtifact artifact2 = createDerivedArtifact("two"); - FileArtifactValue metadata1 = - ActionMetadataHandler.fileArtifactValueFromArtifact(artifact1, null, null); + ArtifactFileMetadata metadata1 = + ActionMetadataHandler.fileMetadataFromArtifact(artifact1, null, null); SpecialArtifact treeArtifact = createDerivedTreeArtifactOnly("tree"); treeArtifact.setGeneratingActionKey(dummyData); TreeFileArtifact treeFileArtifact = ActionInputHelper.treeFileArtifact(treeArtifact, "subpath"); @@ -280,7 +281,7 @@ public void actionExecutionValueSerialization() throws Exception { PathFragment.EMPTY_FRAGMENT, PathFragment.EMPTY_FRAGMENT, PathFragment.EMPTY_FRAGMENT); ActionExecutionValue actionExecutionValue = ActionExecutionValue.create( - ImmutableMap.of(artifact1, metadata1, artifact2, FileArtifactValue.PLACEHOLDER), + ImmutableMap.of(artifact1, metadata1, artifact2, ArtifactFileMetadata.PLACEHOLDER), ImmutableMap.of(treeArtifact, treeArtifactValue), ImmutableMap.of(artifact3, FileArtifactValue.DEFAULT_MIDDLEMAN), ImmutableList.of(filesetOutputSymlink), @@ -288,7 +289,7 @@ public void actionExecutionValueSerialization() throws Exception { true); ActionExecutionValue valueWithFingerprint = ActionExecutionValue.create( - ImmutableMap.of(artifact1, metadata1, artifact2, FileArtifactValue.PLACEHOLDER), + ImmutableMap.of(artifact1, metadata1, artifact2, ArtifactFileMetadata.PLACEHOLDER), ImmutableMap.of(treeArtifact, treeArtifactValue), ImmutableMap.of(artifact3, FileArtifactValue.DEFAULT_MIDDLEMAN), ImmutableList.of(filesetOutputSymlink), @@ -416,7 +417,7 @@ private EvaluationResult evaluate(SkyKey... keys) private static class SimpleActionExecutionFunction implements SkyFunction { @Override public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { - Map artifactData = new HashMap<>(); + Map artifactData = new HashMap<>(); Map treeArtifactData = new HashMap<>(); Map additionalOutputData = new HashMap<>(); ActionLookupData actionLookupData = (ActionLookupData) skyKey.argument(); @@ -438,11 +439,10 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept treeFileArtifact2, FileArtifactValue.createForTesting(treeFileArtifact2))); treeArtifactData.put(output, treeArtifactValue); } else if (action.getActionType() == MiddlemanType.NORMAL) { - FileArtifactValue fileValue = - ActionMetadataHandler.fileArtifactValueFromArtifact(output, null, null); + ArtifactFileMetadata fileValue = + ActionMetadataHandler.fileMetadataFromArtifact(output, null, null); artifactData.put(output, fileValue); - additionalOutputData.put( - output, FileArtifactValue.injectDigestForTesting(output, fileValue)); + additionalOutputData.put(output, FileArtifactValue.createForTesting(output, fileValue)); } else { additionalOutputData.put(output, FileArtifactValue.DEFAULT_MIDDLEMAN); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD index 71cba10bc38653..968c424a6e6adb 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD @@ -78,7 +78,6 @@ java_test( "//src/main/java/com/google/devtools/build/lib:util", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", - "//src/main/java/com/google/devtools/build/lib/actionsketch:action_sketch", "//src/main/java/com/google/devtools/build/lib/analysis/platform", "//src/main/java/com/google/devtools/build/lib/causes", "//src/main/java/com/google/devtools/build/lib/clock", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java index 4d9e29a97c0145..55e45242fa5fe2 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.ArtifactFileMetadata; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; @@ -728,14 +729,14 @@ public List batchStat( // Presently these appear to be untested. private ActionExecutionValue actionValue(Action action, boolean forceDigest) { - Map artifactData = new HashMap<>(); + Map artifactData = new HashMap<>(); for (Artifact output : action.getOutputs()) { try { Path path = output.getPath(); FileStatusWithDigest stat = forceDigest ? statWithDigest(path, path.statIfFound(Symlinks.NOFOLLOW)) : null; artifactData.put( - output, ActionMetadataHandler.fileArtifactValueFromArtifact(output, stat, null)); + output, ActionMetadataHandler.fileMetadataFromArtifact(output, stat, null)); } catch (IOException e) { throw new IllegalStateException(e); } @@ -763,7 +764,7 @@ private ActionExecutionValue actionValueWithEmptyDirectory(Artifact emptyDir) { } private ActionExecutionValue actionValueWithTreeArtifacts(List contents) { - Map fileData = new HashMap<>(); + Map fileData = new HashMap<>(); Map> directoryData = new HashMap<>(); for (TreeFileArtifact output : contents) { @@ -774,9 +775,9 @@ private ActionExecutionValue actionValueWithTreeArtifacts(List dirDatum = new HashMap<>(); directoryData.put(output.getParent(), dirDatum); } - FileArtifactValue fileValue = - ActionMetadataHandler.fileArtifactValueFromArtifact(output, null, null); - dirDatum.put(output, FileArtifactValue.injectDigestForTesting(output, fileValue)); + ArtifactFileMetadata fileValue = + ActionMetadataHandler.fileMetadataFromArtifact(output, null, null); + dirDatum.put(output, FileArtifactValue.createForTesting(output, fileValue)); fileData.put(output, fileValue); } catch (IOException e) { throw new IllegalStateException(e); @@ -821,7 +822,7 @@ private ActionExecutionValue actionValueWithRemoteTreeArtifact( private ActionExecutionValue actionValueWithRemoteArtifact( Artifact output, RemoteFileArtifactValue value) { return ActionExecutionValue.create( - Collections.singletonMap(output, FileArtifactValue.PLACEHOLDER), + Collections.singletonMap(output, ArtifactFileMetadata.PLACEHOLDER), ImmutableMap.of(), Collections.singletonMap(output, value), /* outputSymlinks= */ null, diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java index 50fe5a3cc5fcb9..c4b7081fff0858 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java @@ -140,7 +140,6 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase { protected OptionsParser options; protected final ActionKeyContext actionKeyContext = new ActionKeyContext(); - private TopDownActionCache topDownActionCache; @Before public final void initialize() throws Exception { @@ -154,11 +153,6 @@ public final void initialize() throws Exception { ResourceManager.instance().setAvailableResources(ResourceSet.createWithRamCpu(100, 1)); actions = new LinkedHashSet<>(); actionTemplateExpansionFunction = new ActionTemplateExpansionFunction(actionKeyContext); - topDownActionCache = initTopDownActionCache(); - } - - protected TopDownActionCache initTopDownActionCache() { - return null; } protected void clearActions() { @@ -268,7 +262,6 @@ protected Builder createBuilder( .put( SkyFunctions.ACTION_TEMPLATE_EXPANSION, new DelegatingActionTemplateExpansionFunction()) - .put(SkyFunctions.ACTION_SKETCH, new ActionSketchFunction(actionKeyContext)) .build(), differencer, evaluationProgressReceiver); @@ -317,7 +310,6 @@ public void buildArtifacts( options, new ActionCacheChecker( actionCache, null, actionKeyContext, ALWAYS_EXECUTE_FILTER, null), - topDownActionCache, null); skyframeActionExecutor.setActionExecutionProgressReportingObjects( EMPTY_PROGRESS_SUPPLIER, EMPTY_COMPLETION_RECEIVER); @@ -452,7 +444,9 @@ protected void buildArtifacts(Builder builder, Artifact... artifacts) } protected void buildArtifacts(Builder builder, Executor executor, Artifact... artifacts) - throws BuildFailedException, AbruptExitException, InterruptedException, TestExecException { + throws BuildFailedException, AbruptExitException, InterruptedException, TestExecException, + OptionsParsingException { + tsgm.setCommandStartTime(); Set artifactsToBuild = Sets.newHashSet(artifacts); Set builtTargets = new HashSet<>(); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TopDownActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TopDownActionCacheTest.java deleted file mode 100644 index e4a0f6d75f1a63..00000000000000 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TopDownActionCacheTest.java +++ /dev/null @@ -1,216 +0,0 @@ -// Copyright 2019 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.skyframe; - -import static com.google.common.truth.Truth.assertThat; - -import com.google.common.cache.Cache; -import com.google.common.cache.CacheBuilder; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; -import com.google.devtools.build.lib.actions.ActionKeyContext; -import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.util.TestAction; -import com.google.devtools.build.lib.actionsketch.ActionSketch; -import com.google.devtools.build.lib.util.Fingerprint; -import com.google.devtools.build.lib.vfs.FileSystemUtils; -import java.util.Collection; -import javax.annotation.Nullable; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Tests for top-down, transitive action caching. */ -@RunWith(JUnit4.class) -public class TopDownActionCacheTest extends TimestampBuilderTestCase { - - @Override - protected TopDownActionCache initTopDownActionCache() { - return new InMemoryTopDownActionCache(); - } - - private void buildArtifacts(Artifact... artifacts) throws Exception { - buildArtifacts(amnesiacBuilder(), artifacts); - } - - @Test - public void testAmnesiacBuilderGetsTopDownHit() throws Exception { - Artifact hello = createDerivedArtifact("hello"); - Button button = createActionButton(emptySet, Sets.newHashSet(hello)); - - button.pressed = false; - buildArtifacts(hello); - assertThat(button.pressed).isTrue(); - - button.pressed = false; - buildArtifacts(hello); - assertThat(button.pressed).isFalse(); - } - - @Test - public void testTransitiveTopDownCache() throws Exception { - Artifact hello = createDerivedArtifact("hello"); - Artifact hello2 = createDerivedArtifact("hello2"); - Button button = createActionButton(emptySet, ImmutableSet.of(hello)); - Button button2 = createActionButton(ImmutableSet.of(hello), ImmutableSet.of(hello2)); - - button.pressed = false; - button2.pressed = false; - buildArtifacts(hello2); - assertThat(button.pressed).isTrue(); - assertThat(button2.pressed).isTrue(); - - button.pressed = false; - button2.pressed = false; - buildArtifacts(hello2); - assertThat(button.pressed).isFalse(); - assertThat(button2.pressed).isFalse(); - } - - @Test - public void testActionKeyCaching() throws Exception { - Artifact hello = createDerivedArtifact("hello"); - Artifact hello2 = createDerivedArtifact("hello2"); - - ActionKeyButton button = createActionKeyButton(emptySet, ImmutableSet.of(hello), "abc"); - ActionKeyButton button2 = - createActionKeyButton(ImmutableSet.of(hello), ImmutableSet.of(hello2), "xyz"); - - button.pressed = false; - button2.pressed = false; - buildArtifacts(hello2); - assertThat(button.pressed).isTrue(); - assertThat(button2.pressed).isTrue(); - - button.pressed = false; - button2.pressed = false; - buildArtifacts(hello2); - assertThat(button.pressed).isFalse(); - assertThat(button2.pressed).isFalse(); - - clearActions(); - hello = createDerivedArtifact("hello"); - hello2 = createDerivedArtifact("hello2"); - button = createActionKeyButton(emptySet, ImmutableSet.of(hello), "abc"); - button2 = createActionKeyButton(ImmutableSet.of(hello), ImmutableSet.of(hello2), "123"); - button.pressed = false; - button2.pressed = false; - buildArtifacts(hello2); - assertThat(button.pressed).isFalse(); - assertThat(button2.pressed).isTrue(); - - button.pressed = false; - button2.pressed = false; - buildArtifacts(hello2); - assertThat(button.pressed).isFalse(); - assertThat(button2.pressed).isFalse(); - - clearActions(); - hello = createDerivedArtifact("hello"); - hello2 = createDerivedArtifact("hello2"); - button = createActionKeyButton(emptySet, ImmutableSet.of(hello), "456"); - button2 = createActionKeyButton(ImmutableSet.of(hello), ImmutableSet.of(hello2), "123"); - button.pressed = false; - button2.pressed = false; - buildArtifacts(hello2); - assertThat(button.pressed).isTrue(); - assertThat(button2.pressed).isTrue(); - - button.pressed = false; - button2.pressed = false; - buildArtifacts(hello2); - assertThat(button.pressed).isFalse(); - assertThat(button2.pressed).isFalse(); - } - - @Test - public void testSingleSourceArtifactChanged() throws Exception { - Artifact hello = createSourceArtifact("hello"); - hello.getPath().getParentDirectory().createDirectoryAndParents(); - FileSystemUtils.writeContentAsLatin1(hello.getPath(), "content1"); - - Artifact goodbye = createDerivedArtifact("goodbye"); - Button button = createActionButton(ImmutableSet.of(hello), Sets.newHashSet(goodbye)); - - button.pressed = false; - buildArtifacts(goodbye); - assertThat(button.pressed).isTrue(); - - button.pressed = false; - buildArtifacts(goodbye); - assertThat(button.pressed).isFalse(); // top-down cached - - FileSystemUtils.writeContentAsLatin1(hello.getPath(), "content1"); - button.pressed = false; - buildArtifacts(goodbye); - assertThat(button.pressed).isFalse(); // top-down cached - - FileSystemUtils.writeContentAsLatin1(hello.getPath(), "content2"); - button.pressed = false; - buildArtifacts(goodbye); - assertThat(button.pressed).isTrue(); // built - - button.pressed = false; - buildArtifacts(goodbye); - assertThat(button.pressed).isFalse(); // top-down cached - } - - private static class InMemoryTopDownActionCache implements TopDownActionCache { - private final Cache cache = - CacheBuilder.newBuilder().build(); - - @Nullable - @Override - public ActionExecutionValue get(ActionSketch sketch) { - return cache.getIfPresent(sketch); - } - - @Override - public void put(ActionSketch sketch, ActionExecutionValue value) { - cache.put(sketch, value); - } - } - - private static class MutableActionKeyAction extends TestAction { - - private final ActionKeyButton button; - - public MutableActionKeyAction( - ActionKeyButton button, Collection inputs, Collection outputs) { - super(button, inputs, outputs); - this.button = button; - } - - @Override - protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) { - super.computeKey(actionKeyContext, fp); - fp.addString(button.key); - } - } - - private static class ActionKeyButton extends Button { - private final String key; - - public ActionKeyButton(String key) { - this.key = key; - } - } - - private ActionKeyButton createActionKeyButton( - Collection inputs, Collection outputs, String key) { - ActionKeyButton button = new ActionKeyButton(key); - registerAction(new MutableActionKeyAction(button, inputs, outputs)); - return button; - } -} diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java index 76407e435ee93a..adb9788913a631 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.ArtifactFileMetadata; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.BasicActionLookupValue; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -278,7 +279,7 @@ private class TreeArtifactExecutionFunction implements SkyFunction { @Override public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException, InterruptedException { - Map fileData = new HashMap<>(); + Map fileData = new HashMap<>(); Map treeArtifactData = new HashMap<>(); ActionLookupData actionLookupData = (ActionLookupData) skyKey.argument(); ActionLookupValue actionLookupValue = @@ -288,11 +289,10 @@ public SkyValue compute(SkyKey skyKey, Environment env) for (PathFragment subpath : testTreeArtifactContents) { try { TreeFileArtifact suboutput = ActionInputHelper.treeFileArtifact(output, subpath); - FileArtifactValue fileValue = - ActionMetadataHandler.fileArtifactValueFromArtifact(suboutput, null, null); + ArtifactFileMetadata fileValue = + ActionMetadataHandler.fileMetadataFromArtifact(suboutput, null, null); fileData.put(suboutput, fileValue); - treeArtifactData.put( - suboutput, FileArtifactValue.injectDigestForTesting(suboutput, fileValue)); + treeArtifactData.put(suboutput, FileArtifactValue.createForTesting(suboutput, fileValue)); } catch (IOException e) { throw new SkyFunctionException(e, Transience.TRANSIENT) {}; } diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java index 11b0f1021f0d55..38f7a3a89b3c0e 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java @@ -2908,98 +2908,6 @@ public void testDisallowStructProviderSyntax() throws Exception { + "--incompatible_disallow_struct_provider_syntax=false"); } - @Test - public void testDisableTargetProviderFields() throws Exception { - setSkylarkSemanticsOptions("--incompatible_disable_target_provider_fields=true"); - scratch.file( - "test/skylark/rule.bzl", - "MyProvider = provider()", - "", - "def _my_rule_impl(ctx): ", - " print(ctx.attr.dep.my_info)", - "def _dep_rule_impl(ctx): ", - " my_info = MyProvider(foo = 'bar')", - " return struct(my_info = my_info, providers = [my_info])", - "my_rule = rule(", - " implementation = _my_rule_impl,", - " attrs = {", - " 'dep': attr.label(),", - " })", - "dep_rule = rule(implementation = _dep_rule_impl)"); - scratch.file( - "test/skylark/BUILD", - "load(':rule.bzl', 'my_rule', 'dep_rule')", - "", - "my_rule(name = 'r', dep = ':d')", - "dep_rule(name = 'd')"); - - reporter.removeHandler(failFastHandler); - getConfiguredTarget("//test/skylark:r"); - assertContainsEvent( - "Accessing providers via the field syntax on structs is deprecated and will be removed " - + "soon. It may be temporarily re-enabled by setting " - + "--incompatible_disable_target_provider_fields=false. " - + "See https://github.com/bazelbuild/bazel/issues/9014 for details."); - } - - // Verifies that non-provider fields on the 'target' type are still available even with - // --incompatible_disable_target_provider_fields. - @Test - public void testDisableTargetProviderFields_actionsField() throws Exception { - setSkylarkSemanticsOptions("--incompatible_disable_target_provider_fields=true"); - scratch.file( - "test/skylark/rule.bzl", - "MyProvider = provider()", - "", - "def _my_rule_impl(ctx): ", - " print(ctx.attr.dep.actions)", - "def _dep_rule_impl(ctx): ", - " my_info = MyProvider(foo = 'bar')", - " return struct(my_info = my_info, providers = [my_info])", - "my_rule = rule(", - " implementation = _my_rule_impl,", - " attrs = {", - " 'dep': attr.label(),", - " })", - "dep_rule = rule(implementation = _dep_rule_impl)"); - scratch.file( - "test/skylark/BUILD", - "load(':rule.bzl', 'my_rule', 'dep_rule')", - "", - "my_rule(name = 'r', dep = ':d')", - "dep_rule(name = 'd')"); - - assertThat(getConfiguredTarget("//test/skylark:r")).isNotNull(); - } - - @Test - public void testDisableTargetProviderFields_disabled() throws Exception { - setSkylarkSemanticsOptions("--incompatible_disable_target_provider_fields=false"); - scratch.file( - "test/skylark/rule.bzl", - "MyProvider = provider()", - "", - "def _my_rule_impl(ctx): ", - " print(ctx.attr.dep.my_info)", - "def _dep_rule_impl(ctx): ", - " my_info = MyProvider(foo = 'bar')", - " return struct(my_info = my_info, providers = [my_info])", - "my_rule = rule(", - " implementation = _my_rule_impl,", - " attrs = {", - " 'dep': attr.label(),", - " })", - "dep_rule = rule(implementation = _dep_rule_impl)"); - scratch.file( - "test/skylark/BUILD", - "load(':rule.bzl', 'my_rule', 'dep_rule')", - "", - "my_rule(name = 'r', dep = ':d')", - "dep_rule(name = 'd')"); - - assertThat(getConfiguredTarget("//test/skylark:r")).isNotNull(); - } - @Test public void testNoRuleOutputsParam() throws Exception { setSkylarkSemanticsOptions("--incompatible_no_rule_outputs_param=true"); diff --git a/src/test/shell/bazel/bazel_embedded_skylark_test.sh b/src/test/shell/bazel/bazel_embedded_skylark_test.sh index 8bff2ef92ad325..a5cf4091ed9b6f 100755 --- a/src/test/shell/bazel/bazel_embedded_skylark_test.sh +++ b/src/test/shell/bazel/bazel_embedded_skylark_test.sh @@ -53,7 +53,7 @@ pkg_tar( srcs = glob(["*.txt"]), ) EOF - bazel build --all_incompatible_changes $HOST_PY_FLAG ... \ + bazel build --all_incompatible_changes $HOST_PY_FLAG ... &> $TEST_log \ || fail "Expect success, even with all upcoming Skylark changes" grep -q 'Hello World' `bazel info bazel-bin --all_incompatible_changes $HOST_PY_FLAG`/data.tar \ || fail "Output not generated correctly" @@ -78,7 +78,8 @@ pkg_tar( symlinks = {"link_with_colons" : "some:dangling:link"}, ) EOF - bazel build --all_incompatible_changes $HOST_PY_FLAG :fancy || fail "Expected success" + bazel build --all_incompatible_changes $HOST_PY_FLAG :fancy &> $TEST_log \ + || fail "Expected success" mkdir ../out tar -C ../out -x -v -f `bazel info bazel-bin --all_incompatible_changes $HOST_PY_FLAG`/fancy.tar @@ -128,7 +129,8 @@ create_banana_directory = rule( implementation = _create_banana_directory_impl, ) EOF - bazel build --all_incompatible_changes $HOST_PY_FLAG :banana_tarball || fail "Expected success" + bazel build --all_incompatible_changes $HOST_PY_FLAG :banana_tarball &> $TEST_log \ + || fail "Expected success" mkdir ../out tar -C ../out -x -v -f `bazel info bazel-bin --all_incompatible_changes $HOST_PY_FLAG`/banana_tarball.tar @@ -167,7 +169,7 @@ genrule( executable = True, ) EOF - bazel build --all_incompatible_changes :foo \ + bazel build --all_incompatible_changes :foo &> $TEST_log \ || fail "Expected to build even with incompatible changes" bazel run :foo | grep -q dragons || fail "wrong output" } @@ -209,7 +211,7 @@ genrule( executable = True, ) EOF - bazel build --all_incompatible_changes :foo \ + bazel build --all_incompatible_changes :foo &> $TEST_log \ || fail "Expected to build even with incompatible changes" bazel run :foo | grep -q dragons || fail "wrong output" } diff --git a/src/test/shell/bazel/platforms_test.sh b/src/test/shell/bazel/platforms_test.sh index 74114f446fa5cd..d33a6c631f5bd7 100755 --- a/src/test/shell/bazel/platforms_test.sh +++ b/src/test/shell/bazel/platforms_test.sh @@ -78,42 +78,5 @@ function test_incompatible_use_platforms_repo_for_constraints() { expect_log "Constraints from @bazel_tools//platforms have been removed." } - -function test_platform_accessor() { - cat > rules.bzl <<'EOF' -def _impl(ctx): - platform = ctx.attr.platform[platform_common.PlatformInfo] - properties = platform.exec_properties - print("The properties are:", properties) - return [] - -print_props = rule( - implementation = _impl, - attrs = { - 'platform': attr.label(providers = [platform_common.PlatformInfo]), - } -) -EOF - cat > BUILD << 'EOF' -load("//:rules.bzl", "print_props") - -print_props( - name = "a", - platform = ":my_platform", -) - -platform( - name = "my_platform", - exec_properties = { - "key": "value", - "key2": "value2", - } -) -EOF - - bazel build --experimental_platforms_api=true :a &> $TEST_log || fail "Build failed" - grep 'The properties are: {"key2": "value2", "key": "value"}' $TEST_log || fail "Did not find expected properties" -} - run_suite "platform mapping test" diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index a70ce9f7d31e81..328bae19efa1c6 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -1131,34 +1131,6 @@ EOF expect_log "uri:.*bytestream://localhost" } -function test_exclusive_tag() { - # Test that the exclusive tag works with the remote cache. - mkdir -p a - cat > a/success.sh <<'EOF' -#!/bin/sh -exit 0 -EOF - chmod 755 a/success.sh - cat > a/BUILD <<'EOF' -sh_test( - name = "success_test", - srcs = ["success.sh"], - tags = ["exclusive"], -) -EOF - - bazel test \ - --remote_cache=grpc://localhost:${worker_port} \ - //a:success_test || fail "Failed to test //a:success_test" - - bazel test \ - --remote_cache=grpc://localhost:${worker_port} \ - --nocache_test_results \ - //a:success_test >& $TEST_log || fail "Failed to test //a:success_test" - - expect_log "remote cache hit" -} - # TODO(alpha): Add a test that fails remote execution when remote worker # supports sandbox. diff --git a/src/test/shell/integration/modify_execution_info_test.sh b/src/test/shell/integration/modify_execution_info_test.sh index ed29252ec54c5d..7d173a332655df 100755 --- a/src/test/shell/integration/modify_execution_info_test.sh +++ b/src/test/shell/integration/modify_execution_info_test.sh @@ -158,6 +158,14 @@ new_local_repository( path = "$(dirname $(rlocation io_bazel/third_party/protobuf/3.6.1/BUILD))", build_file = "$(rlocation io_bazel/third_party/protobuf/3.6.1/BUILD)", ) + +# TODO(#9029): May require some adjustment if/when we depend on the real +# @rules_python in the real source tree, since this third_party/ package won't +# be available. +local_repository( + name = "rules_python", + path = "$(dirname $(rlocation io_bazel/third_party/rules_python/WORKSPACE))", +) EOF fi local pkg="${FUNCNAME[0]}" diff --git a/tools/python/private/defs.bzl b/tools/python/private/defs.bzl index 88b9e169a87f62..cc6a7f7bf8835e 100644 --- a/tools/python/private/defs.bzl +++ b/tools/python/private/defs.bzl @@ -14,11 +14,21 @@ """This is a Bazel-internal file; do not load() it! -This file replicates the behavior of the macros in bazelbuild/rules_python's -`//python:defs.bzl` file, allowing the four native rules to be used even with -`--incompatible_load_python_rules_from_bzl` enabled. - -We need this file because Bazel's own codebase may not depend on rules_python. +The incompatible change `--incompatible_load_python_rules_from_bzl` (#9006) +makes it so the four native Python rules cannot be used unless a magic tag is +present. It is intended that only `@rules_python` (bazelbuild/rules_python) +uses this tag, and all other uses access the native rules via the wrapper +macros defined in `@rules_python`. + +However, `@bazel_tools` is not allowed to depend on any other repos. Therefore, +we replicate the behavior of `@rules_python`'s wrapper macros in this file, for +use by Bazel only. + +This gets a bit tricky with the third_party/ directory. Some of its +subdirectories are full-blown workspaces that cannot directly reference this +file's label. For these cases we create a mock `@rules_python` in Bazel's +WORKSPACE, and rely on those repos using the proper @rules_python-qualified +label. (#9029 tracks possibly replacing the mock with the real thing.) """ _MIGRATION_TAG = "__PYTHON_RULES_MIGRATION_DO_NOT_USE_WILL_BREAK__"