diff options
author | Laszlo Csomor <laszlocsomor@google.com> | 2016-09-20 15:40:42 +0000 |
---|---|---|
committer | Laszlo Csomor <laszlocsomor@google.com> | 2016-09-21 07:06:32 +0000 |
commit | 8539a1215bb58211c7c643005d2389ecafa6f580 (patch) | |
tree | 87783de1c37833261051d4c540e52ec81fcbdbfe /src/main/java/com/google/devtools/build/lib/analysis | |
parent | 63010255876a81cf2b0bc4fc5d95a0e1a99df58d (diff) |
Rollback of commit 82d43279f93d95e4c41b4bc598a3cc05ddd1ae1a.
*** 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
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib/analysis')
6 files changed, 142 insertions, 112 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java index 6da8281b40..495d5ebdd4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java @@ -123,7 +123,7 @@ public final class AnalysisUtils { * <p>For example "//pkg:target" -> "pkg/<fragment>/target. */ public static PathFragment getUniqueDirectory(Label label, PathFragment fragment) { - return label.getPackageIdentifier().getPackageFragment().getRelative(fragment) + return label.getPackageIdentifier().getSourceRoot().getRelative(fragment) .getRelative(label.getName()); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CompilationHelper.java b/src/main/java/com/google/devtools/build/lib/analysis/CompilationHelper.java index a3769e3e07..8a1fddc6ff 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CompilationHelper.java @@ -54,8 +54,8 @@ public final class CompilationHelper { } 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 @@ public final class CompilationHelper { 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()))); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java index 17ab4416fd..fdd38db6de 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java @@ -143,7 +143,9 @@ public final class ConfiguredTargetFactory { : 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 class ConfiguredTargetFactory { } 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)); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index 563abe5387..f160424223 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java @@ -609,7 +609,7 @@ public final class RuleContext extends TargetContext * {@link #getUniqueDirectoryArtifact(String, PathFragment, Root)}) ensures that this is the case. */ public PathFragment getPackageDirectory() { - return getLabel().getPackageIdentifier().getPackageFragment(); + return getLabel().getPackageIdentifier().getSourceRoot(); } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java index e0bb6ce79e..0de1b2d513 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java @@ -417,19 +417,6 @@ public final class Runfiles { return newManifest; } - private static PathFragment getRootRelativePath(Artifact artifact) { - Preconditions.checkArgument(artifact != null); - if (artifact.isInMainRepo()) { - return artifact.getRootRelativePath(); - } - - return new PathFragment(Label.EXTERNAL_PATH_PREFIX) - .getRelative(artifact.isSourceArtifact() - ? artifact.getRoot().getPath().getBaseName() - : artifact.getRoot().getExecRoot().getBaseName()) - .getRelative(artifact.getRootRelativePath()); - } - /** * Returns the symlinks as a map from PathFragment to Artifact. * @@ -446,7 +433,7 @@ public final class Runfiles { Map<PathFragment, Artifact> manifest = getSymlinksAsMap(checker); // Add unconditional artifacts (committed to inclusion on construction of runfiles). for (Artifact artifact : getUnconditionalArtifactsWithoutMiddlemen()) { - checker.put(manifest, Runfiles.getRootRelativePath(artifact), artifact); + checker.put(manifest, artifact.getRootRelativePath(), artifact); } // Add conditional artifacts (only included if they appear in a pruning manifest). @@ -503,7 +490,7 @@ public final class Runfiles { // workspace. private boolean sawWorkspaceName; - ManifestBuilder( + public ManifestBuilder( PathFragment workspaceName, boolean legacyExternalRunfiles) { this.manifest = new HashMap<>(); this.workspaceName = workspaceName; @@ -514,27 +501,19 @@ public final class Runfiles { /** * Adds a map under the workspaceName. */ - void addUnderWorkspace( + public void addUnderWorkspace( Map<PathFragment, Artifact> inputManifest, ConflictChecker checker) { for (Map.Entry<PathFragment, Artifact> entry : inputManifest.entrySet()) { PathFragment path = entry.getKey(); - if (isUnderWorkspace(entry.getValue())) { - // For empty files (e.g., ../repo/__init__.py), we don't have an artifact so - // isUnderWorkspace returns true. - checker.put(manifest, workspaceName.getRelative(path).normalize(), entry.getValue()); + if (isUnderWorkspace(path)) { sawWorkspaceName = true; + checker.put(manifest, workspaceName.getRelative(path), entry.getValue()); } else { - PathFragment root = entry.getValue().getRoot().getPath().asFragment(); if (legacyExternalRunfiles) { - // external/repo - PathFragment repoDir = root.subFragment(root.segmentCount() - 2, root.segmentCount()); - // Turn ../repo/foo info wsname/external/repo/foo. - checker.put( - manifest, workspaceName.getRelative(repoDir).getRelative(path).normalize(), - entry.getValue()); + checker.put(manifest, workspaceName.getRelative(path), entry.getValue()); } - checker.put( - manifest, workspaceName.getRelative(entry.getKey()).normalize(), entry.getValue()); + // Always add the non-legacy .runfiles/repo/whatever path. + checker.put(manifest, getExternalPath(path), entry.getValue()); } } } @@ -562,20 +541,17 @@ public final class Runfiles { return manifest; } + private PathFragment getExternalPath(PathFragment path) { + return checkForWorkspace(path.relativeTo(Label.EXTERNAL_PACKAGE_NAME)); + } + private PathFragment checkForWorkspace(PathFragment path) { - sawWorkspaceName = sawWorkspaceName - || path.getSegment(0).equals(workspaceName.getPathString()); + sawWorkspaceName = sawWorkspaceName || path.getSegment(0).equals(workspaceName); return path; } - private static boolean isUnderWorkspace(@Nullable Artifact artifact) { - if (artifact == null) { - return true; - } - PathFragment root = artifact.getRoot().getPath().asFragment(); - return root.segmentCount() < 2 - || !root.getSegment(root.segmentCount() - 2).equals( - Label.EXTERNAL_PACKAGE_NAME.getPathString()); + private static boolean isUnderWorkspace(PathFragment path) { + return !path.startsWith(Label.EXTERNAL_PACKAGE_NAME); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java index df9d7666b3..272582da94 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.analysis.config; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; import com.google.common.base.Verify; import com.google.common.collect.ArrayListMultimap; @@ -23,8 +24,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; -import com.google.common.collect.Interner; -import com.google.common.collect.Interners; import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; @@ -84,6 +83,7 @@ import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Queue; import java.util.Set; import java.util.TreeMap; @@ -955,6 +955,62 @@ public final class BuildConfiguration { } } + /** + * All the output directories pertinent to a configuration. + */ + private static final class OutputRoots implements Serializable { + private final Root outputDirectory; // the configuration-specific output directory. + private final Root binDirectory; + private final Root genfilesDirectory; + private final Root coverageMetadataDirectory; // for coverage-related metadata, artifacts, etc. + private final Root testLogsDirectory; + private final Root includeDirectory; + private final Root middlemanDirectory; + + private OutputRoots(BlazeDirectories directories, String outputDirName) { + Path execRoot = directories.getExecRoot(); + // configuration-specific output tree + Path outputDir = directories.getOutputPath().getRelative(outputDirName); + this.outputDirectory = Root.asDerivedRoot(execRoot, outputDir, true); + + // specific subdirs under outputDirectory + this.binDirectory = Root + .asDerivedRoot(execRoot, outputDir.getRelative("bin"), true); + this.genfilesDirectory = Root.asDerivedRoot( + execRoot, outputDir.getRelative("genfiles"), true); + this.coverageMetadataDirectory = Root.asDerivedRoot(execRoot, + outputDir.getRelative("coverage-metadata"), true); + this.testLogsDirectory = Root.asDerivedRoot( + execRoot, outputDir.getRelative("testlogs"), true); + this.includeDirectory = Root.asDerivedRoot( + execRoot, outputDir.getRelative(BlazeDirectories.RELATIVE_INCLUDE_DIR), true); + this.middlemanDirectory = Root.middlemanRoot(execRoot, outputDir, true); + } + + @Override + public boolean equals(Object o) { + if (o == this) { + return true; + } + if (!(o instanceof OutputRoots)) { + return false; + } + OutputRoots other = (OutputRoots) o; + return outputDirectory.equals(other.outputDirectory) + && binDirectory.equals(other.binDirectory) + && genfilesDirectory.equals(other.genfilesDirectory) + && coverageMetadataDirectory.equals(other.coverageMetadataDirectory) + && testLogsDirectory.equals(other.testLogsDirectory) + && includeDirectory.equals(other.includeDirectory); + } + + @Override + public int hashCode() { + return Objects.hash(outputDirectory, binDirectory, genfilesDirectory, + coverageMetadataDirectory, testLogsDirectory, includeDirectory); + } + } + private final String checksum; private Transitions transitions; @@ -966,7 +1022,7 @@ public final class BuildConfiguration { /** * Directories in the output tree. * - * <p>The computation of the output directories should be a non-injective mapping from + * <p>The computation of the output directory should be a non-injective mapping from * BuildConfiguration instances to strings. The result should identify the aspects of the * configuration that should be reflected in the output file names. Furthermore the * returned string must not contain shell metacharacters. @@ -1003,54 +1059,7 @@ public final class BuildConfiguration { * this so that the build works even if the two configurations are too close (which is common) * and so that the path of artifacts in the host configuration is a bit more readable. */ - private enum OutputDirectory { - BIN("bin"), - GENFILES("genfiles"), - MIDDLEMAN(true), - TESTLOGS("testlogs"), - COVERAGE("coverage-metadata"), - INCLUDE(BlazeDirectories.RELATIVE_INCLUDE_DIR), - OUTPUT(false); - - private final String name; - private final boolean middleman; - - /** - * This constructor is for roots without suffixes, e.g., - * [[execroot/repo]/bazel-out/local-fastbuild]. - * @param isMiddleman whether the root should be a middleman root or a "normal" derived root. - */ - OutputDirectory(boolean isMiddleman) { - this.name = ""; - this.middleman = isMiddleman; - } - - OutputDirectory(String name) { - this.name = name; - this.middleman = false; - } - - Root getRoot( - RepositoryName repositoryName, String outputDirName, BlazeDirectories directories) { - // e.g., execroot/repo1/../repo2 -> execroot/repo2 - Path execRoot = directories.getExecRoot().getRelative(repositoryName.getPathUnderExecRoot()); - // e.g., execroot/repo2/bazel-out/config - Path outputDir = execRoot.getRelative(directories.getRelativeOutputPath()) - .getRelative(outputDirName); - if (middleman) { - return INTERNER.intern(Root.middlemanRoot(execRoot, outputDir, repositoryName.isMain())); - } - // e.g., execroot/repo2/bazel-out/config/bin - return INTERNER.intern( - Root.asDerivedRoot(execRoot, outputDir.getRelative(name), repositoryName.isMain())); - } - } - - // "Cache" of roots, so we don't keep around thousands of copies of the same root. - private static Interner<Root> INTERNER = Interners.newWeakInterner(); - - private final BlazeDirectories directories; - private final String outputDirName; + private final OutputRoots outputRoots; /** If false, AnalysisEnviroment doesn't register any actions created by the ConfiguredTarget. */ private final boolean actionsEnabled; @@ -1228,12 +1237,23 @@ public final class BuildConfiguration { /** * Constructs a new BuildConfiguration instance. */ - public BuildConfiguration( - BlazeDirectories directories, + public BuildConfiguration(BlazeDirectories directories, + Map<Class<? extends Fragment>, Fragment> fragmentsMap, + BuildOptions buildOptions, + boolean actionsDisabled) { + this(null, directories, fragmentsMap, buildOptions, actionsDisabled); + } + + /** + * Constructor variation that uses the passed in output roots if non-null, else computes them + * from the directories. + */ + public BuildConfiguration(@Nullable OutputRoots outputRoots, + @Nullable BlazeDirectories directories, Map<Class<? extends Fragment>, Fragment> fragmentsMap, BuildOptions buildOptions, boolean actionsDisabled) { - this.directories = directories; + Preconditions.checkState(outputRoots == null ^ directories == null); this.actionsEnabled = !actionsDisabled; this.fragments = ImmutableSortedMap.copyOf(fragmentsMap, lexicalFragmentSorter); @@ -1261,12 +1281,16 @@ public final class BuildConfiguration { commandLineBuildVariables = ImmutableMap.copyOf(commandLineDefinesBuilder); this.mnemonic = buildMnemonic(); - this.outputDirName = (options.outputDirectoryName != null) + String outputDirName = (options.outputDirectoryName != null) ? options.outputDirectoryName : mnemonic; this.platformName = buildPlatformName(); this.shExecutable = collectExecutables().get("sh"); + this.outputRoots = outputRoots != null + ? outputRoots + : new OutputRoots(directories, outputDirName); + Pair<ImmutableMap<String, String>, ImmutableSet<String>> shellEnvironment = setupShellEnvironment(); this.localShellEnvironment = shellEnvironment.getFirst(); @@ -1310,7 +1334,7 @@ public final class BuildConfiguration { BuildOptions options = buildOptions.trim( getOptionsClasses(fragmentsMap.keySet(), ruleClassProvider)); BuildConfiguration newConfig = - new BuildConfiguration(directories, fragmentsMap, options, !actionsEnabled); + new BuildConfiguration(outputRoots, null, fragmentsMap, options, !actionsEnabled); newConfig.setConfigurationTransitions(this.transitions); return newConfig; } @@ -1826,6 +1850,23 @@ public final class BuildConfiguration { } /** + * For an given environment, returns a subset containing all + * variables in the given list if they are defined in the given + * environment. + */ + @VisibleForTesting + static Map<String, String> getMapping(List<String> variables, + Map<String, String> environment) { + Map<String, String> result = new HashMap<>(); + for (String var : variables) { + if (environment.containsKey(var)) { + result.put(var, environment.get(var)); + } + } + return result; + } + + /** * Returns the {@link Option} class the defines the given option, null if the * option isn't recognized. * @@ -1874,7 +1915,7 @@ public final class BuildConfiguration { * Returns the output directory for this build configuration. */ public Root getOutputDirectory(RepositoryName repositoryName) { - return OutputDirectory.OUTPUT.getRoot(repositoryName, outputDirName, directories); + return outputRoots.outputDirectory; } /** @@ -1883,7 +1924,7 @@ public final class BuildConfiguration { @SkylarkCallable(name = "bin_dir", structField = true, documented = false) @Deprecated public Root getBinDirectory() { - return getBinDirectory(RepositoryName.MAIN); + return outputRoots.binDirectory; } /** @@ -1891,9 +1932,11 @@ public final class BuildConfiguration { * repositories without changes to how ArtifactFactory resolves derived roots. This is not an * issue right now because it only effects Blaze's include scanning (internal) and Bazel's * repositories (external) but will need to be fixed. + * TODO(kchodorow): Use the repository name to derive the bin directory. */ + @SuppressWarnings("unused") public Root getBinDirectory(RepositoryName repositoryName) { - return OutputDirectory.BIN.getRoot(repositoryName, outputDirName, directories); + return getBinDirectory(); } /** @@ -1905,9 +1948,11 @@ public final class BuildConfiguration { /** * Returns the include directory for this build configuration. + * TODO(kchodorow): Use the repository name to derive the include directory. */ + @SuppressWarnings("unused") public Root getIncludeDirectory(RepositoryName repositoryName) { - return OutputDirectory.INCLUDE.getRoot(repositoryName, outputDirName, directories); + return outputRoots.includeDirectory; } /** @@ -1916,27 +1961,33 @@ public final class BuildConfiguration { @SkylarkCallable(name = "genfiles_dir", structField = true, documented = false) @Deprecated public Root getGenfilesDirectory() { - return getGenfilesDirectory(RepositoryName.MAIN); + return outputRoots.genfilesDirectory; } + // TODO(kchodorow): Use the repository name to derive the genfiles directory. + @SuppressWarnings("unused") public Root getGenfilesDirectory(RepositoryName repositoryName) { - return OutputDirectory.GENFILES.getRoot(repositoryName, outputDirName, directories); + return getGenfilesDirectory(); } /** * Returns the directory where coverage-related artifacts and metadata files * should be stored. This includes for example uninstrumented class files * needed for Jacoco's coverage reporting tools. + * TODO(kchodorow): Use the repository name to derive the coverage directory. */ + @SuppressWarnings("unused") public Root getCoverageMetadataDirectory(RepositoryName repositoryName) { - return OutputDirectory.COVERAGE.getRoot(repositoryName, outputDirName, directories); + return outputRoots.coverageMetadataDirectory; } /** * Returns the testlogs directory for this build configuration. + * TODO(kchodorow): Use the repository name to derive the test directory. */ + @SuppressWarnings("unused") public Root getTestLogsDirectory(RepositoryName repositoryName) { - return OutputDirectory.TESTLOGS.getRoot(repositoryName, outputDirName, directories); + return outputRoots.testLogsDirectory; } /** @@ -1961,9 +2012,11 @@ public final class BuildConfiguration { /** * Returns the internal directory (used for middlemen) for this build configuration. + * TODO(kchodorow): Use the repository name to derive the middleman directory. */ + @SuppressWarnings("unused") public Root getMiddlemanDirectory(RepositoryName repositoryName) { - return OutputDirectory.MIDDLEMAN.getRoot(repositoryName, outputDirName, directories); + return outputRoots.middlemanDirectory; } public boolean getAllowRuntimeDepsOnNeverLink() { |