Permalink
Browse files

Rollback of commit 82d4327.

*** Reason for rollback ***

Breaks TensorFlow and other Bazel jobs on ci.bazel.io

*** Original change description ***

Change execution root for external repositories to be ../repo

Some of the important aspect of this change:

* Remote repos in the execution root are under output_base/execroot/repo_name, so the prefix is ../repo_name (to escape the local workspace name).
* Package roots for external repos were previously "output_base/", they are now output_base/external/repo_name (which means source artifacts always have a relative path from their repository).
* Outputs are under bazel-bin/external/repo_name/ (or similarly under genfiles). Note that this is a bit of a change from how this was implemented in the previous cl.

Fixes #1262.

RELNOTES[INC]: Previously, an external repository would be symlinked into the
execution root at execroot/local_repo/external/remote_repo. This changes it to
be at execroot/remote_repo. This may break genrules/Skylark actions that
hardcode execution root paths. If this causes breakages for you, ensure that
genrules are using $(location :target) to access files and Skylark rules are
using http://bazel.io/docs/skylark/lib/File.html's path, dirname, etc.
functions.

Roll forward of bdfd58a.

--
MOS_MIGRATED_REVID=133709658
  • Loading branch information...
1 parent 6301025 commit 8539a1215bb58211c7c643005d2389ecafa6f580 @laszlocsomor laszlocsomor committed Sep 20, 2016
Showing with 416 additions and 565 deletions.
  1. +0 −1 .gitignore
  2. +1 −1 src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BUILD
  3. +18 −42 src/main/java/com/google/devtools/build/lib/actions/Artifact.java
  4. +10 −17 src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
  5. +3 −5 src/main/java/com/google/devtools/build/lib/actions/Root.java
  6. +1 −1 src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
  7. +3 −4 src/main/java/com/google/devtools/build/lib/analysis/CompilationHelper.java
  8. +4 −2 src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
  9. +1 −1 src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
  10. +15 −39 src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
  11. +118 −65 src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
  12. +3 −4 src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRule.java
  13. +1 −2 src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
  14. +2 −8 src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
  15. +56 −154 src/main/java/com/google/devtools/build/lib/buildtool/SymlinkForest.java
  16. +2 −2 src/main/java/com/google/devtools/build/lib/cmdline/Label.java
  17. +9 −1 src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
  18. +11 −2 src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
  19. +2 −2 src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
  20. +2 −3 src/main/java/com/google/devtools/build/lib/packages/Package.java
  21. +2 −1 src/main/java/com/google/devtools/build/lib/rules/android/LocalResourceContainer.java
  22. +2 −6 src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
  23. +1 −1 src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
  24. +7 −25 src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
  25. +2 −3 src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
  26. +6 −15 src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
  27. +1 −1 src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
  28. +12 −6 src/main/java/com/google/devtools/build/lib/rules/java/Jvm.java
  29. +1 −1 src/main/java/com/google/devtools/build/lib/rules/java/JvmConfigurationLoader.java
  30. +5 −5 src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedStrategy.java
  31. +2 −2 src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
  32. +1 −5 src/main/java/com/google/devtools/build/lib/sandbox/SandboxStrategy.java
  33. +1 −1 src/main/java/com/google/devtools/build/lib/sandbox/SymlinkedExecRoot.java
  34. +1 −1 src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageLoaderWithValueEnvironment.java
  35. +37 −8 src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java
  36. +0 −3 src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
  37. +15 −51 src/test/java/com/google/devtools/build/lib/buildtool/SymlinkForestTest.java
  38. +7 −6 src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java
  39. +4 −4 src/test/java/com/google/devtools/build/lib/cmdline/RepositoryNameTest.java
  40. +1 −1 src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
  41. +2 −2 src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
  42. +1 −1 src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
  43. +1 −1 src/test/java/com/google/devtools/build/lib/testutil/TestConstants.java
  44. +2 −2 src/test/shell/bazel/bazel_rules_test.sh
  45. +2 −2 src/test/shell/bazel/bazel_sandboxing_test.sh
  46. +11 −13 src/test/shell/bazel/external_correctness_test.sh
  47. +8 −14 src/test/shell/bazel/external_integration_test.sh
  48. +10 −14 src/test/shell/bazel/git_repository_test.sh
  49. +5 −8 src/test/shell/bazel/local_repository_test.sh
  50. +1 −2 src/test/shell/bazel/skylark_repository_test.sh
  51. +2 −3 src/test/shell/bazel/workspace_test.sh
  52. +1 −1 src/tools/singlejar/BUILD
View
@@ -8,7 +8,6 @@
/WORKSPACE.user.bzl
/bazel-bazel
/bazel-bin
-/bazel-external
/bazel-genfiles
/bazel-io_bazel
/bazel-out
@@ -12,7 +12,7 @@ genrule(
name = "javac-bootclasspath-locations",
srcs = ["@bazel_tools//tools/jdk:bootclasspath"],
outs = ["JavacBootclasspathLocations.java"],
- cmd = "$(location javac-bootclasspath-locations.sh) $@ $(GENDIR) 'bazel_tools/' $(SRCS)",
+ cmd = "$(location javac-bootclasspath-locations.sh) $@ $(GENDIR) '' $(SRCS)",
tools = ["javac-bootclasspath-locations.sh"],
)
@@ -40,19 +40,17 @@
import java.util.Collection;
import java.util.Comparator;
import java.util.List;
-import java.util.Objects;
import javax.annotation.Nullable;
/**
* An Artifact represents a file used by the build system, whether it's a source
* file or a derived (output) file. Not all Artifacts have a corresponding
- * FileTarget object in the <code>build.lib.packages</code> API: for example,
+ * FileTarget object in the <code>build.packages</code> API: for example,
* low-level intermediaries internal to a given rule, such as a Java class files
* or C++ object files. However all FileTargets have a corresponding Artifact.
*
- * <p>In any given call to
- * {@link com.google.devtools.build.lib.skyframe.SkyframeExecutor#buildArtifacts}, no two Artifacts
- * in the action graph may refer to the same path.
+ * <p>In any given call to Builder#buildArtifacts(), no two Artifacts in the
+ * action graph may refer to the same path.
*
* <p>Artifacts generally fall into two classifications, source and derived, but
* there exist a few other cases that are fuzzy and difficult to classify. The
@@ -130,6 +128,7 @@ public int compareTo(Object o) {
return EvalUtils.compareByClass(this, o);
}
+
/** An object that can expand middleman artifacts. */
public interface ArtifactExpander {
@@ -183,10 +182,10 @@ public boolean apply(Artifact input) {
* </pre>
*
* <p>In a derived Artifact, the execPath will overlap with part of the root, which in turn will
- * be below the execRoot.
+ * be below of the execRoot.
* <pre>
* [path] == [/root][pathTail] == [/execRoot][execPath] == [/execRoot][rootPrefix][pathTail]
- * </pre>
+ * <pre>
*/
@VisibleForTesting
public Artifact(Path path, Root root, PathFragment execPath, ArtifactOwner owner) {
@@ -201,6 +200,7 @@ public Artifact(Path path, Root root, PathFragment execPath, ArtifactOwner owner
this.hashCode = path.hashCode();
this.path = path;
this.root = root;
+ this.execPath = execPath;
// These two lines establish the invariant that
// execPath == rootRelativePath <=> execPath.equals(rootRelativePath)
// This is important for isSourceArtifact.
@@ -210,7 +210,6 @@ public Artifact(Path path, Root root, PathFragment execPath, ArtifactOwner owner
+ rootRel + " at " + path + " with root " + root);
}
this.rootRelativePath = rootRel.equals(execPath) ? execPath : rootRel;
- this.execPath = externalfy(execPath);
this.owner = Preconditions.checkNotNull(owner, path);
}
@@ -351,12 +350,7 @@ public PathFragment getParentRelativePath() {
@SkylarkCallable(name = "is_source", structField = true,
doc = "Returns true if this is a source file, i.e. it is not generated")
public final boolean isSourceArtifact() {
- // All source roots should, you know, point to sources. However, for embedded binaries, they
- // are actually created as derived artifacts, so we have to special-case isSourceArtifact to
- // treat derived roots in the main repo where execPath==rootRelPath as source roots.
- // Source artifacts have reference-identical execPaths and rootRelativePaths, so use
- // of == instead of .equals() is intentional here.
- return execPath == rootRelativePath || root.isSourceRoot();
+ return execPath == rootRelativePath;
}
/**
@@ -521,9 +515,15 @@ public final PathFragment getRootRelativePath() {
* For targets in external repositories, this returns the path the artifact live at in the
* runfiles tree. For local targets, it returns the rootRelativePath.
*/
- // TODO(kchodorow): remove.
public final PathFragment getRunfilesPath() {
- return externalfy(rootRelativePath);
+ PathFragment relativePath = rootRelativePath;
+ if (relativePath.segmentCount() > 1
+ && relativePath.getSegment(0).equals(Label.EXTERNAL_PATH_PREFIX)) {
+ // Turn external/repo/foo into ../repo/foo.
+ relativePath = relativePath.relativeTo(Label.EXTERNAL_PATH_PREFIX);
+ relativePath = new PathFragment("..").getRelative(relativePath);
+ }
+ return relativePath;
}
@SkylarkCallable(
@@ -557,31 +557,7 @@ public final String getRunfilesPathString() {
+ "runfiles of a binary."
)
public final String getExecPathString() {
- return getExecPath().toString();
- }
-
- private PathFragment externalfy(PathFragment relativePath) {
- if (root.isMainRepo()) {
- return relativePath;
- }
-
- PathFragment prefix;
- if (root.isSourceRoot()) {
- prefix = new PathFragment(Label.EXTERNAL_PATH_PREFIX)
- .getRelative(root.getPath().getBaseName());
- } else {
- prefix = new PathFragment(Label.EXTERNAL_PATH_PREFIX)
- .getRelative(root.getExecRoot().getBaseName());
- }
- return prefix.getRelative(relativePath);
- }
-
- /**
- * Returns if this artifact is in the main repository or not.
- */
- // TODO(kchodorow): remove once execroot path is correct.
- public boolean isInMainRepo() {
- return root.isMainRepo();
+ return getExecPath().getPathString();
}
/*
@@ -609,7 +585,7 @@ public final boolean equals(Object other) {
// We don't bother to check root in the equivalence relation, because we
// assume that no root is an ancestor of another one.
Artifact that = (Artifact) other;
- return Objects.equals(this.path, that.path);
+ return this.path.equals(that.path);
}
@Override
@@ -287,9 +287,8 @@ private Artifact createArtifact(Path path, Root root, PathFragment execPath, Art
* not null). That Artifact will have root determined by the package roots of this factory if it
* lives in a subpackage distinct from that of baseExecPath, and {@code baseRoot} otherwise.
*/
- private synchronized Artifact resolveSourceArtifactWithAncestor(
- PathFragment relativePath, PathFragment baseExecPath, Root baseRoot,
- RepositoryName repositoryName) {
+ public synchronized Artifact resolveSourceArtifactWithAncestor(
+ PathFragment relativePath, PathFragment baseExecPath, Root baseRoot) {
Preconditions.checkState(
(baseExecPath == null) == (baseRoot == null),
"%s %s %s",
@@ -314,15 +313,7 @@ private synchronized Artifact resolveSourceArtifactWithAncestor(
return null;
}
- return createArtifactIfNotValid(
- findSourceRoot(execPath, baseExecPath, baseRoot, repositoryName), execPath);
- }
-
- // TODO(kchodorow): make remote repositories work with include scanning.
- public synchronized Artifact resolveSourceArtifactWithAncestor(
- PathFragment relativePath, PathFragment baseExecPath, Root baseRoot) {
- return resolveSourceArtifactWithAncestor(
- relativePath, baseExecPath, baseRoot, RepositoryName.MAIN);
+ return createArtifactIfNotValid(findSourceRoot(execPath, baseExecPath, baseRoot), execPath);
}
/**
@@ -331,20 +322,21 @@ public synchronized Artifact resolveSourceArtifactWithAncestor(
*/
@Nullable
private Root findSourceRoot(
- PathFragment execPath, @Nullable PathFragment baseExecPath, @Nullable Root baseRoot,
- RepositoryName repoName) {
+ PathFragment execPath, @Nullable PathFragment baseExecPath, @Nullable Root baseRoot) {
PathFragment dir = execPath.getParentDirectory();
if (dir == null) {
return null;
}
+ RepositoryName repoName = RepositoryName.MAIN;
+
Pair<RepositoryName, PathFragment> repo = RepositoryName.fromPathFragment(dir);
if (repo != null) {
repoName = repo.getFirst();
dir = repo.getSecond();
}
- while (packageRoots != null && dir != null && !dir.equals(baseExecPath)) {
+ while (dir != null && !dir.equals(baseExecPath)) {
Root sourceRoot = packageRoots.get(PackageIdentifier.create(repoName, dir));
if (sourceRoot != null) {
return sourceRoot;
@@ -356,8 +348,9 @@ private Root findSourceRoot(
}
@Override
- public Artifact resolveSourceArtifact(PathFragment execPath, RepositoryName repositoryName) {
- return resolveSourceArtifactWithAncestor(execPath, null, null, repositoryName);
+ public Artifact resolveSourceArtifact(PathFragment execPath,
+ @SuppressWarnings("unused") RepositoryName repositoryName) {
+ return resolveSourceArtifactWithAncestor(execPath, null, null);
}
@Override
@@ -170,7 +170,7 @@ boolean isMiddlemanRoot() {
return isMiddlemanRoot;
}
- boolean isMainRepo() {
+ public boolean isMainRepo() {
return isMainRepo;
}
@@ -181,7 +181,7 @@ public int compareTo(Root o) {
@Override
public int hashCode() {
- return Objects.hash(execRoot, path.hashCode(), isMainRepo);
+ return Objects.hash(execRoot, path.hashCode());
}
@Override
@@ -193,9 +193,7 @@ public boolean equals(Object o) {
return false;
}
Root r = (Root) o;
- return path.equals(r.path)
- && Objects.equals(execRoot, r.execRoot)
- && Objects.equals(isMainRepo, r.isMainRepo);
+ return path.equals(r.path) && Objects.equals(execRoot, r.execRoot);
}
@Override
@@ -123,7 +123,7 @@ public static PathFragment getManifestPathFromFilesetPath(PathFragment filesetDi
* <p>For example "//pkg:target" -> "pkg/&lt;fragment&gt;/target.
*/
public static PathFragment getUniqueDirectory(Label label, PathFragment fragment) {
- return label.getPackageIdentifier().getPackageFragment().getRelative(fragment)
+ return label.getPackageIdentifier().getSourceRoot().getRelative(fragment)
.getRelative(label.getName());
}
@@ -54,8 +54,8 @@
}
MiddlemanFactory factory = env.getMiddlemanFactory();
return ImmutableList.of(factory.createMiddlemanAllowMultiple(
- env, actionOwner, ruleContext.getRule().getLabel().getPackageIdentifier().getSourceRoot(),
- purpose, filesToBuild, ruleContext.getConfiguration().getMiddlemanDirectory(
+ env, actionOwner, ruleContext.getPackageDirectory(), purpose, filesToBuild,
+ ruleContext.getConfiguration().getMiddlemanDirectory(
ruleContext.getRule().getRepository())));
}
@@ -87,8 +87,7 @@
MiddlemanFactory factory = env.getMiddlemanFactory();
Iterable<Artifact> artifacts = dep.getProvider(FileProvider.class).getFilesToBuild();
return ImmutableList.of(
- factory.createMiddlemanAllowMultiple(env, actionOwner,
- ruleContext.getRule().getLabel().getPackageIdentifier().getSourceRoot(),
+ factory.createMiddlemanAllowMultiple(env, actionOwner, ruleContext.getPackageDirectory(),
purpose, artifacts, ruleContext.getConfiguration().getMiddlemanDirectory(
ruleContext.getRule().getRepository())));
}
@@ -143,7 +143,9 @@ private Artifact getOutputArtifact(OutputFile outputFile, BuildConfiguration con
: configuration.getGenfilesDirectory(rule.getRepository());
ArtifactOwner owner =
new ConfiguredTargetKey(rule.getLabel(), configuration.getArtifactOwnerConfiguration());
- PathFragment rootRelativePath = outputFile.getLabel().toPathFragment();
+ PathFragment rootRelativePath =
+ outputFile.getLabel().getPackageIdentifier().getSourceRoot().getRelative(
+ outputFile.getLabel().getName());
Artifact result = isFileset
? artifactFactory.getFilesetArtifact(rootRelativePath, root, owner)
: artifactFactory.getDerivedArtifact(rootRelativePath, root, owner);
@@ -194,7 +196,7 @@ public final ConfiguredTarget createConfiguredTarget(AnalysisEnvironment analysi
} else if (target instanceof InputFile) {
InputFile inputFile = (InputFile) target;
Artifact artifact = artifactFactory.getSourceArtifact(
- inputFile.getLabel().toPathFragment(),
+ inputFile.getExecPath(),
Root.asSourceRoot(inputFile.getPackage().getSourceRoot(),
inputFile.getPackage().getPackageIdentifier().getRepository().isMain()),
new ConfiguredTargetKey(target.getLabel(), config));
@@ -609,7 +609,7 @@ public Artifact getPackageRelativeArtifact(PathFragment relative, Root root) {
* {@link #getUniqueDirectoryArtifact(String, PathFragment, Root)}) ensures that this is the case.
*/
public PathFragment getPackageDirectory() {
- return getLabel().getPackageIdentifier().getPackageFragment();
+ return getLabel().getPackageIdentifier().getSourceRoot();
}
/**
Oops, something went wrong.

0 comments on commit 8539a12

Please sign in to comment.