Permalink
Browse files

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=133606309
  • Loading branch information...
1 parent 35b50d2 commit 82d43279f93d95e4c41b4bc598a3cc05ddd1ae1a @kchodorow kchodorow committed with laszlocsomor Sep 19, 2016
Showing with 571 additions and 422 deletions.
  1. +1 −0 .gitignore
  2. +1 −1 src/java_tools/buildjar/java/com/google/devtools/build/java/bazel/BUILD
  3. +42 −18 src/main/java/com/google/devtools/build/lib/actions/Artifact.java
  4. +17 −10 src/main/java/com/google/devtools/build/lib/actions/ArtifactFactory.java
  5. +5 −3 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. +4 −3 src/main/java/com/google/devtools/build/lib/analysis/CompilationHelper.java
  8. +2 −4 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. +39 −15 src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
  11. +65 −118 src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
  12. +4 −3 src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRule.java
  13. +2 −1 src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
  14. +8 −2 src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
  15. +154 −56 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. +1 −9 src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
  18. +2 −11 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. +3 −2 src/main/java/com/google/devtools/build/lib/packages/Package.java
  21. +1 −2 src/main/java/com/google/devtools/build/lib/rules/android/LocalResourceContainer.java
  22. +6 −2 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. +25 −7 src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
  25. +3 −2 src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
  26. +15 −6 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. +6 −12 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. +5 −1 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. +11 −40 src/test/java/com/google/devtools/build/lib/analysis/RunfilesTest.java
  36. +3 −0 src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
  37. +51 −15 src/test/java/com/google/devtools/build/lib/buildtool/SymlinkForestTest.java
  38. +9 −10 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. +13 −11 src/test/shell/bazel/external_correctness_test.sh
  47. +14 −8 src/test/shell/bazel/external_integration_test.sh
  48. +14 −10 src/test/shell/bazel/git_repository_test.sh
  49. +8 −5 src/test/shell/bazel/local_repository_test.sh
  50. +2 −1 src/test/shell/bazel/skylark_repository_test.sh
  51. +3 −2 src/test/shell/bazel/workspace_test.sh
  52. +1 −1 src/tools/singlejar/BUILD
View
@@ -8,6 +8,7 @@
/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) '' $(SRCS)",
+ cmd = "$(location javac-bootclasspath-locations.sh) $@ $(GENDIR) 'bazel_tools/' $(SRCS)",
tools = ["javac-bootclasspath-locations.sh"],
)
@@ -40,17 +40,19 @@
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.packages</code> API: for example,
+ * FileTarget object in the <code>build.lib.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 Builder#buildArtifacts(), no two Artifacts in the
- * action graph may refer to the same path.
+ * <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>Artifacts generally fall into two classifications, source and derived, but
* there exist a few other cases that are fuzzy and difficult to classify. The
@@ -128,7 +130,6 @@ public int compareTo(Object o) {
return EvalUtils.compareByClass(this, o);
}
-
/** An object that can expand middleman artifacts. */
public interface ArtifactExpander {
@@ -182,10 +183,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 of the execRoot.
+ * be below the execRoot.
* <pre>
* [path] == [/root][pathTail] == [/execRoot][execPath] == [/execRoot][rootPrefix][pathTail]
- * <pre>
+ * </pre>
*/
@VisibleForTesting
public Artifact(Path path, Root root, PathFragment execPath, ArtifactOwner owner) {
@@ -200,7 +201,6 @@ 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,6 +210,7 @@ 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);
}
@@ -350,7 +351,12 @@ 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() {
- return execPath == rootRelativePath;
+ // 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();
}
/**
@@ -515,15 +521,9 @@ 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() {
- 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;
+ return externalfy(rootRelativePath);
}
@SkylarkCallable(
@@ -557,7 +557,31 @@ public final String getRunfilesPathString() {
+ "runfiles of a binary."
)
public final String getExecPathString() {
- return getExecPath().getPathString();
+ 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();
}
/*
@@ -585,7 +609,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 this.path.equals(that.path);
+ return Objects.equals(this.path, that.path);
}
@Override
@@ -287,8 +287,9 @@ 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.
*/
- public synchronized Artifact resolveSourceArtifactWithAncestor(
- PathFragment relativePath, PathFragment baseExecPath, Root baseRoot) {
+ private synchronized Artifact resolveSourceArtifactWithAncestor(
+ PathFragment relativePath, PathFragment baseExecPath, Root baseRoot,
+ RepositoryName repositoryName) {
Preconditions.checkState(
(baseExecPath == null) == (baseRoot == null),
"%s %s %s",
@@ -313,7 +314,15 @@ public synchronized Artifact resolveSourceArtifactWithAncestor(
return null;
}
- return createArtifactIfNotValid(findSourceRoot(execPath, baseExecPath, baseRoot), execPath);
+ 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);
}
/**
@@ -322,21 +331,20 @@ public synchronized Artifact resolveSourceArtifactWithAncestor(
*/
@Nullable
private Root findSourceRoot(
- PathFragment execPath, @Nullable PathFragment baseExecPath, @Nullable Root baseRoot) {
+ PathFragment execPath, @Nullable PathFragment baseExecPath, @Nullable Root baseRoot,
+ RepositoryName repoName) {
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 (dir != null && !dir.equals(baseExecPath)) {
+ while (packageRoots != null && dir != null && !dir.equals(baseExecPath)) {
Root sourceRoot = packageRoots.get(PackageIdentifier.create(repoName, dir));
if (sourceRoot != null) {
return sourceRoot;
@@ -348,9 +356,8 @@ private Root findSourceRoot(
}
@Override
- public Artifact resolveSourceArtifact(PathFragment execPath,
- @SuppressWarnings("unused") RepositoryName repositoryName) {
- return resolveSourceArtifactWithAncestor(execPath, null, null);
+ public Artifact resolveSourceArtifact(PathFragment execPath, RepositoryName repositoryName) {
+ return resolveSourceArtifactWithAncestor(execPath, null, null, repositoryName);
}
@Override
@@ -170,7 +170,7 @@ boolean isMiddlemanRoot() {
return isMiddlemanRoot;
}
- public boolean isMainRepo() {
+ boolean isMainRepo() {
return isMainRepo;
}
@@ -181,7 +181,7 @@ public int compareTo(Root o) {
@Override
public int hashCode() {
- return Objects.hash(execRoot, path.hashCode());
+ return Objects.hash(execRoot, path.hashCode(), isMainRepo);
}
@Override
@@ -193,7 +193,9 @@ public boolean equals(Object o) {
return false;
}
Root r = (Root) o;
- return path.equals(r.path) && Objects.equals(execRoot, r.execRoot);
+ return path.equals(r.path)
+ && Objects.equals(execRoot, r.execRoot)
+ && Objects.equals(isMainRepo, r.isMainRepo);
}
@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().getSourceRoot().getRelative(fragment)
+ return label.getPackageIdentifier().getPackageFragment().getRelative(fragment)
.getRelative(label.getName());
}
@@ -54,8 +54,8 @@
}
MiddlemanFactory factory = env.getMiddlemanFactory();
return ImmutableList.of(factory.createMiddlemanAllowMultiple(
- env, actionOwner, ruleContext.getPackageDirectory(), purpose, filesToBuild,
- ruleContext.getConfiguration().getMiddlemanDirectory(
+ env, actionOwner, ruleContext.getRule().getLabel().getPackageIdentifier().getSourceRoot(),
+ purpose, filesToBuild, ruleContext.getConfiguration().getMiddlemanDirectory(
ruleContext.getRule().getRepository())));
}
@@ -87,7 +87,8 @@
MiddlemanFactory factory = env.getMiddlemanFactory();
Iterable<Artifact> artifacts = dep.getProvider(FileProvider.class).getFilesToBuild();
return ImmutableList.of(
- factory.createMiddlemanAllowMultiple(env, actionOwner, ruleContext.getPackageDirectory(),
+ factory.createMiddlemanAllowMultiple(env, actionOwner,
+ ruleContext.getRule().getLabel().getPackageIdentifier().getSourceRoot(),
purpose, artifacts, ruleContext.getConfiguration().getMiddlemanDirectory(
ruleContext.getRule().getRepository())));
}
@@ -143,9 +143,7 @@ private Artifact getOutputArtifact(OutputFile outputFile, BuildConfiguration con
: configuration.getGenfilesDirectory(rule.getRepository());
ArtifactOwner owner =
new ConfiguredTargetKey(rule.getLabel(), configuration.getArtifactOwnerConfiguration());
- PathFragment rootRelativePath =
- outputFile.getLabel().getPackageIdentifier().getSourceRoot().getRelative(
- outputFile.getLabel().getName());
+ PathFragment rootRelativePath = outputFile.getLabel().toPathFragment();
Artifact result = isFileset
? artifactFactory.getFilesetArtifact(rootRelativePath, root, owner)
: artifactFactory.getDerivedArtifact(rootRelativePath, root, owner);
@@ -196,7 +194,7 @@ public final ConfiguredTarget createConfiguredTarget(AnalysisEnvironment analysi
} else if (target instanceof InputFile) {
InputFile inputFile = (InputFile) target;
Artifact artifact = artifactFactory.getSourceArtifact(
- inputFile.getExecPath(),
+ inputFile.getLabel().toPathFragment(),
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().getSourceRoot();
+ return getLabel().getPackageIdentifier().getPackageFragment();
}
/**
Oops, something went wrong.

0 comments on commit 82d4327

Please sign in to comment.