diff options
Diffstat (limited to 'src/main/java/com/google/devtools/build')
6 files changed, 122 insertions, 36 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java index 6e47a2d54b..b5a87a84ef 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateFunction.java @@ -39,12 +39,15 @@ public class DirectoryListingStateFunction implements SkyFunction { public SkyValue compute(SkyKey skyKey, Environment env) throws DirectoryListingStateFunctionException { RootedPath dirRootedPath = (RootedPath) skyKey.argument(); - externalFilesHelper.maybeAddDepOnBuildId(dirRootedPath, env); - if (env.valuesMissing()) { - return null; - } + try { + externalFilesHelper.maybeHandleExternalFile(dirRootedPath, env); + if (env.valuesMissing()) { + return null; + } return DirectoryListingStateValue.create(dirRootedPath); + } catch (FileOutsidePackageRootsException e) { + throw new DirectoryListingStateFunctionException(e); } catch (IOException e) { throw new DirectoryListingStateFunctionException(e); } @@ -64,5 +67,9 @@ public class DirectoryListingStateFunction implements SkyFunction { public DirectoryListingStateFunctionException(IOException e) { super(e, Transience.TRANSIENT); } + + public DirectoryListingStateFunctionException(FileOutsidePackageRootsException e) { + super(e, Transience.PERSISTENT); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java index 10a6e65ebe..9363111e7b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.vfs.Path; @@ -27,14 +28,32 @@ class ExternalFilesHelper { private final AtomicReference<PathPackageLocator> pkgLocator; private final Set<Path> immutableDirs; + private final boolean errorOnExternalFiles; + @VisibleForTesting ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator) { - this(pkgLocator, ImmutableSet.<Path>of()); + this(pkgLocator, ImmutableSet.<Path>of(), /*errorOnExternalFiles=*/false); } - ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator, Set<Path> immutableDirs) { + @VisibleForTesting + ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator, + boolean errorOnExternalFiles) { + this(pkgLocator, ImmutableSet.<Path>of(), errorOnExternalFiles); + } + + /** + * @param pkgLocator an {@link AtomicReference} to a {@link PathPackageLocator} used to + * determine what files are internal + * @param immutableDirs directories whose contents may be considered unchangeable + * @param errorOnExternalFiles whether or not to allow references to files outside of + * the directories provided by pkgLocator or immutableDirs. See + * {@link #maybeHandleExternalFile(RootedPath, SkyFunction.Environment)} for more details. + */ + ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator, Set<Path> immutableDirs, + boolean errorOnExternalFiles) { this.pkgLocator = pkgLocator; this.immutableDirs = immutableDirs; + this.errorOnExternalFiles = errorOnExternalFiles; } private enum FileType { @@ -69,29 +88,45 @@ class ExternalFilesHelper { return getFileType(rootedPath) == FileType.EXTERNAL_IMMUTABLE_FILE; } - public void maybeAddDepOnBuildId(RootedPath rootedPath, SkyFunction.Environment env) { - if (getFileType(rootedPath) == FileType.EXTERNAL_MUTABLE_FILE) { - // For files outside the package roots that are not assumed to be immutable, add a dependency - // on the build_id. This is sufficient for correctness; all other files will be handled by - // diff awareness of their respective package path, but these files need to be addressed - // separately. - // - // Using the build_id here seems to introduce a performance concern because the upward - // transitive closure of these external files will get eagerly invalidated on each - // incremental build (e.g. if every file had a transitive dependency on the filesystem root, - // then we'd have a big performance problem). But this a non-issue by design: - // - We don't add a dependency on the parent directory at the package root boundary, so the - // only transitive dependencies from files inside the package roots to external files are - // through symlinks. So the upwards transitive closure of external files is small. - // - The only way external source files get into the skyframe graph in the first place is - // through symlinks outside the package roots, which we neither want to encourage nor - // optimize for since it is not common. So the set of external files is small. - // - // The above reasoning doesn't hold for bazel, because external repositories - // (e.g. new_local_repository) cause lots of external symlinks to be present in the build. - // So bazel pretends that these external repositories are immutable to avoid the performance - // penalty described above. - PrecomputedValue.dependOnBuildId(env); + /** + * Potentially adds a dependency on build_id to env if this instance is configured + * with errorOnExternalFiles=false and rootedPath is an external mutable file. + * If errorOnExternalFiles=true and rootedPath is an external mutable file then + * a FileOutsidePackageRootsException is thrown. This method is a no-op for any + * rootedPaths that fall within the known package roots. + * + * @param rootedPath + * @param env + * @throws FileOutsidePackageRootsException + */ + public void maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env) + throws FileOutsidePackageRootsException { + if (getFileType(rootedPath) == FileType.EXTERNAL_MUTABLE_FILE) { + if (!errorOnExternalFiles) { + // For files outside the package roots that are not assumed to be immutable, add a + // dependency on the build_id. This is sufficient for correctness; all other files + // will be handled by diff awareness of their respective package path, but these + // files need to be addressed separately. + // + // Using the build_id here seems to introduce a performance concern because the upward + // transitive closure of these external files will get eagerly invalidated on each + // incremental build (e.g. if every file had a transitive dependency on the filesystem root, + // then we'd have a big performance problem). But this a non-issue by design: + // - We don't add a dependency on the parent directory at the package root boundary, so the + // only transitive dependencies from files inside the package roots to external files are + // through symlinks. So the upwards transitive closure of external files is small. + // - The only way external source files get into the skyframe graph in the first place is + // through symlinks outside the package roots, which we neither want to encourage nor + // optimize for since it is not common. So the set of external files is small. + // + // The above reasoning doesn't hold for bazel, because external repositories + // (e.g. new_local_repository) cause lots of external symlinks to be present in the build. + // So bazel pretends that these external repositories are immutable to avoid the performance + // penalty described above. + PrecomputedValue.dependOnBuildId(env); + } else { + throw new FileOutsidePackageRootsException(rootedPath); + } } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileOutsidePackageRootsException.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileOutsidePackageRootsException.java new file mode 100644 index 0000000000..75f2e91b86 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileOutsidePackageRootsException.java @@ -0,0 +1,34 @@ +// Copyright 2015 Google Inc. 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.vfs.RootedPath; + +import java.io.IOException; + +/** + * Exception thrown to signal reference to file outside of known/allowed directory structures. + * Extends IOException to ensure it is properly handled by skyframe. + */ +// TODO(bazel-team): Don't piggyback on existing handling of IOExceptions and instead implement +// the desired semantics. +public class FileOutsidePackageRootsException extends IOException { + + /** + * @param outsideRoots the {@link RootedPath} that triggered this exception. + */ + public FileOutsidePackageRootsException(RootedPath outsideRoots) { + super("Encountered reference to external mutable " + outsideRoots); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java index ec2e871bbd..928306c1eb 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileStateFunction.java @@ -42,12 +42,15 @@ public class FileStateFunction implements SkyFunction { @Override public SkyValue compute(SkyKey skyKey, Environment env) throws FileStateFunctionException { RootedPath rootedPath = (RootedPath) skyKey.argument(); - externalFilesHelper.maybeAddDepOnBuildId(rootedPath, env); - if (env.valuesMissing()) { - return null; - } + try { + externalFilesHelper.maybeHandleExternalFile(rootedPath, env); + if (env.valuesMissing()) { + return null; + } return FileStateValue.create(rootedPath, tsgm); + } catch (FileOutsidePackageRootsException e) { + throw new FileStateFunctionException(e); } catch (IOException e) { throw new FileStateFunctionException(e); } catch (InconsistentFilesystemException e) { @@ -72,5 +75,9 @@ public class FileStateFunction implements SkyFunction { public FileStateFunctionException(InconsistentFilesystemException e) { super(e, Transience.TRANSIENT); } + + public FileStateFunctionException(FileOutsidePackageRootsException e) { + super(e, Transience.PERSISTENT); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java index be3d190933..f8265c9101 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java @@ -101,7 +101,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { super(reporter, evaluatorSupplier, pkgFactory, tsgm, directories, workspaceStatusActionFactory, buildInfoFactories, immutableDirectories, allowedMissingInputs, preprocessorFactorySupplier, - extraSkyFunctions, extraPrecomputedValues); + extraSkyFunctions, extraPrecomputedValues, /*errorOnExternalFiles=*/false); this.diffAwarenessManager = new DiffAwarenessManager(diffAwarenessFactories, reporter); } 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 26728347b0..3fb2ee87a4 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 @@ -217,6 +217,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { protected int outputDirtyFiles; protected int modifiedFilesDuringPreviousBuild; private final Predicate<PathFragment> allowedMissingInputs; + private final boolean errorOnExternalFiles; private final ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions; private final ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues; @@ -245,7 +246,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { Predicate<PathFragment> allowedMissingInputs, Preprocessor.Factory.Supplier preprocessorFactorySupplier, ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions, - ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues) { + ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues, + boolean errorOnExternalFiles) { // Strictly speaking, these arguments are not required for initialization, but all current // callsites have them at hand, so we might as well set them during construction. this.reporter = Preconditions.checkNotNull(reporter); @@ -269,6 +271,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { this.preprocessorFactorySupplier = preprocessorFactorySupplier; this.extraSkyFunctions = extraSkyFunctions; this.extraPrecomputedValues = extraPrecomputedValues; + this.errorOnExternalFiles = errorOnExternalFiles; } private ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions( @@ -276,7 +279,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { PackageFactory pkgFactory, Predicate<PathFragment> allowedMissingInputs) { ExternalFilesHelper externalFilesHelper = new ExternalFilesHelper(pkgLocator, - immutableDirectories); + immutableDirectories, errorOnExternalFiles); // We use an immutable map builder for the nice side effect that it throws if a duplicate key // is inserted. ImmutableMap.Builder<SkyFunctionName, SkyFunction> map = ImmutableMap.builder(); |