From 54e24df757464ba7c4d6d579d2251e26eb7d34c4 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 28 Mar 2016 19:11:39 +0000 Subject: Provide descriptive error messages on external mutable source files encountered during a build Currently when evaluating a file or symlink leading to an external mutable object, Blaze throws an exception with unclear messages. The message does not contain the actual path but rather [/]/[] instead. This change updates FileFunction to allow bubbling up the error with the accurate path. -- MOS_MIGRATED_REVID=118381323 --- .../devtools/build/lib/skyframe/FileFunction.java | 69 ++++++++++++++++++---- .../skyframe/FileOutsidePackageRootsException.java | 17 +++--- .../SymlinkOutsidePackageRootsException.java | 35 +++++++++++ 3 files changed, 103 insertions(+), 18 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/SymlinkOutsidePackageRootsException.java (limited to 'src/main/java/com') diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java index da6b9d989a..6eb962114c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java @@ -29,6 +29,7 @@ import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import java.io.IOException; import java.util.ArrayList; import java.util.TreeSet; import java.util.concurrent.atomic.AtomicReference; @@ -62,8 +63,19 @@ public class FileFunction implements SkyFunction { // symlink cycle, we want to detect that quickly as it gives a more informative error message // than we'd get doing bogus filesystem operations. if (!relativePath.equals(PathFragment.EMPTY_FRAGMENT)) { - Pair resolvedState = - resolveFromAncestors(rootedPath, env); + Pair resolvedState = null; + + try { + resolvedState = resolveFromAncestors(rootedPath, env); + } catch (FileOutsidePackageRootsException e) { + // When getting a FileOutsidePackageRootsException caused by an external file symlink + // somewhere in this file's path, rethrow an exception with this file's path, so the error + // message mentions this file instead of the first ancestor path where the external file + // error is observed + throw new FileFunctionException( + new FileOutsidePackageRootsException(rootedPath), Transience.PERSISTENT); + } + if (resolvedState == null) { return null; } @@ -71,7 +83,18 @@ public class FileFunction implements SkyFunction { realFileStateValue = resolvedState.getSecond(); } - FileStateValue fileStateValue = (FileStateValue) env.getValue(FileStateValue.key(rootedPath)); + FileStateValue fileStateValue = null; + + try { + fileStateValue = + (FileStateValue) + env.getValueOrThrow( + FileStateValue.key(rootedPath), FileOutsidePackageRootsException.class); + } catch (FileOutsidePackageRootsException e) { + throw new FileFunctionException( + new FileOutsidePackageRootsException(rootedPath), Transience.PERSISTENT); + } + if (fileStateValue == null) { return null; } @@ -109,15 +132,21 @@ public class FileFunction implements SkyFunction { * {@code null} if there was a missing dep. */ @Nullable - private Pair resolveFromAncestors(RootedPath rootedPath, - Environment env) throws FileFunctionException { + private Pair resolveFromAncestors( + RootedPath rootedPath, Environment env) + throws FileFunctionException, FileOutsidePackageRootsException { PathFragment relativePath = rootedPath.getRelativePath(); RootedPath realRootedPath = rootedPath; FileValue parentFileValue = null; if (!relativePath.equals(PathFragment.EMPTY_FRAGMENT)) { RootedPath parentRootedPath = RootedPath.toRootedPath(rootedPath.getRoot(), relativePath.getParentDirectory()); - parentFileValue = (FileValue) env.getValue(FileValue.key(parentRootedPath)); + + parentFileValue = + (FileValue) + env.getValueOrThrow( + FileValue.key(parentRootedPath), FileOutsidePackageRootsException.class); + if (parentFileValue == null) { return null; } @@ -125,13 +154,17 @@ public class FileFunction implements SkyFunction { RootedPath parentRealRootedPath = parentFileValue.realRootedPath(); realRootedPath = RootedPath.toRootedPath(parentRealRootedPath.getRoot(), parentRealRootedPath.getRelativePath().getRelative(baseName)); + if (!parentFileValue.exists()) { return Pair.of( realRootedPath, FileStateValue.NONEXISTENT_FILE_STATE_NODE); } } FileStateValue realFileStateValue = - (FileStateValue) env.getValue(FileStateValue.key(realRootedPath)); + (FileStateValue) + env.getValueOrThrow( + FileStateValue.key(realRootedPath), FileOutsidePackageRootsException.class); + if (realFileStateValue == null) { return null; } @@ -230,8 +263,9 @@ public class FileFunction implements SkyFunction { ImmutableList.copyOf( Iterables.concat(symlinkChain, ImmutableList.of(symlinkTargetRootedPath)))); uniquenessKey = FileSymlinkInfiniteExpansionUniquenessFunction.key(pathAndChain.getSecond()); - fse = new FileSymlinkInfiniteExpansionException( - pathAndChain.getFirst(), pathAndChain.getSecond()); + fse = + new FileSymlinkInfiniteExpansionException( + pathAndChain.getFirst(), pathAndChain.getSecond()); } if (uniquenessKey != null) { if (env.getValue(uniquenessKey) == null) { @@ -241,7 +275,18 @@ public class FileFunction implements SkyFunction { } throw new FileFunctionException(Preconditions.checkNotNull(fse, rootedPath)); } - return resolveFromAncestors(symlinkTargetRootedPath, env); + + try { + return resolveFromAncestors(symlinkTargetRootedPath, env); + } catch (FileOutsidePackageRootsException e) { + // At this point we know this file node is a symlink leading to an external file. Mark the + // exception to be a specific SymlinkOutsidePackageRootsException. The error will be bubbled + // up further but no path information will be updated again. This allows preserving the + // information about the symlink crossing the internal/external boundary. + throw new FileFunctionException( + new SymlinkOutsidePackageRootsException(rootedPath, symlinkTargetRootedPath), + Transience.PERSISTENT); + } } private static final Predicate isPathPredicate(final Path path) { @@ -272,5 +317,9 @@ public class FileFunction implements SkyFunction { public FileFunctionException(FileSymlinkException e) { super(e, Transience.PERSISTENT); } + + public FileFunctionException(IOException e, Transience transience) { + super(e, transience); + } } } 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 index 319957bfba..ce9e27c0e7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileOutsidePackageRootsException.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileOutsidePackageRootsException.java @@ -18,17 +18,18 @@ 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. + *

This is an implementation detail of {@link FileFunction} to signal attempt to evaluate a file + * outside of known/allowed directory structures. + * + *

Extends {@link IOException} to ensure it is properly handled by consumers of + * {@link FileValue}. */ // TODO(bazel-team): Don't piggyback on existing handling of IOExceptions and instead implement // the desired semantics. -public class FileOutsidePackageRootsException extends IOException { +class FileOutsidePackageRootsException extends IOException { - /** - * @param outsideRoots the {@link RootedPath} that triggered this exception. - */ - public FileOutsidePackageRootsException(RootedPath outsideRoots) { - super("Encountered reference to external mutable " + outsideRoots); + /** @param outsidePath the {@link RootedPath} that triggered this exception. */ + public FileOutsidePackageRootsException(RootedPath outsidePath) { + super("Encountered reference to external mutable " + outsidePath); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SymlinkOutsidePackageRootsException.java b/src/main/java/com/google/devtools/build/lib/skyframe/SymlinkOutsidePackageRootsException.java new file mode 100644 index 0000000000..3f20ce0a48 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SymlinkOutsidePackageRootsException.java @@ -0,0 +1,35 @@ +// Copyright 2016 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.vfs.RootedPath; + +import java.io.IOException; + +/** + *

This is an implementation detail of {@link FileFunction} to to evaluate a symlink linking to + * file outside of known/allowed directory structures. + * + *

Extends {@link IOException} to ensure it is properly handled by consumers of + * {@link FileValue}. + */ +class SymlinkOutsidePackageRootsException extends IOException { + /** + * @param symlinkPath the {@link RootedPath} that links to an outside path. + * @param outsidePath the {@link RootedPath} that triggered this exception. + */ + public SymlinkOutsidePackageRootsException(RootedPath symlinkPath, RootedPath outsidePath) { + super("Encountered symlink " + symlinkPath + " linking to external mutable " + outsidePath); + } +} -- cgit v1.2.3