From 82d43279f93d95e4c41b4bc598a3cc05ddd1ae1a Mon Sep 17 00:00:00 2001 From: Kristina Chodorow Date: Mon, 19 Sep 2016 18:08:59 +0000 Subject: 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 --- .../devtools/build/lib/rules/cpp/CcCommon.java | 8 ++++-- .../build/lib/rules/cpp/CcLibraryHelper.java | 2 +- .../build/lib/rules/cpp/CppCompileAction.java | 32 +++++++++++++++++----- .../devtools/build/lib/rules/cpp/CppHelper.java | 5 ++-- .../build/lib/rules/cpp/HeaderDiscovery.java | 21 ++++++++++---- 5 files changed, 50 insertions(+), 18 deletions(-) (limited to 'src/main/java/com/google/devtools/build/lib/rules/cpp') diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java index c7f11d8f29..35d5a99504 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java @@ -396,9 +396,13 @@ public final class CcCommon { continue; } PathFragment includesPath = packageFragment.getRelative(includesAttr).normalize(); - if (!includesPath.isNormalized()) { + // It's okay for the includes path to start with ../workspace-name for external repos. + if ((packageIdentifier.getRepository().isMain() && !includesPath.isNormalized()) + || (!packageIdentifier.getRepository().isMain() + && !includesPath.startsWith( + packageIdentifier.getRepository().getPathUnderExecRoot()))) { ruleContext.attributeError("includes", - "Path references a path above the execution root."); + includesAttr + " references a path above the execution root (" + includesPath + ")."); } if (includesPath.segmentCount() == 0) { ruleContext.attributeError( diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java index de7373a8ba..2e64a02e03 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java @@ -1106,7 +1106,7 @@ public final class CcLibraryHelper { ruleContext.getLabel().getPackageIdentifier().getRepository().getPathUnderExecRoot(); contextBuilder.addQuoteIncludeDir(repositoryPath); contextBuilder.addQuoteIncludeDir( - ruleContext.getConfiguration().getGenfilesFragment().getRelative(repositoryPath)); + repositoryPath.getRelative(ruleContext.getConfiguration().getGenfilesFragment())); for (PathFragment systemIncludeDir : systemIncludeDirs) { contextBuilder.addSystemIncludeDir(systemIncludeDir); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 21d0d307c5..d878e90f63 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -43,6 +43,7 @@ import com.google.devtools.build.lib.analysis.actions.ExecutionInfoSpecifier; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.PerLabelOptions; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.collect.CollectionUtils; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -328,10 +329,15 @@ public class CppCompileAction extends AbstractAction continue; } - if (include.isAbsolute() - || !PathFragment.EMPTY_FRAGMENT.getRelative(include).normalize().isNormalized()) { - ruleContext.ruleError( - "The include path '" + include + "' references a path outside of the execution root."); + // One starting ../ is okay for getting to a sibling repository. + PathFragment originalInclude = include; + if (include.startsWith(new PathFragment(Label.EXTERNAL_PATH_PREFIX))) { + include = include.relativeTo(Label.EXTERNAL_PATH_PREFIX); + } + + if (include.isAbsolute() || !include.normalize().isNormalized()) { + ruleContext.ruleError("The include path '" + originalInclude + + "' references a path outside of the execution root."); } } } @@ -971,6 +977,7 @@ public class CppCompileAction extends AbstractAction if (execPath.getBaseName().endsWith(".pcm")) { continue; } + RepositoryName repositoryName = RepositoryName.MAIN; PathFragment execPathFragment = execPath.asFragment(); if (execPathFragment.isAbsolute()) { // Absolute includes from system paths are ignored. @@ -983,14 +990,25 @@ public class CppCompileAction extends AbstractAction // the build with an error. if (execPath.startsWith(execRoot)) { execPathFragment = execPath.relativeTo(execRoot); // funky but tolerable path + } else if (execPath.startsWith(execRoot.getParentDirectory())) { + // External repository. + execPathFragment = execPath.relativeTo(execRoot.getParentDirectory()); + String workspace = execPathFragment.getSegment(0); + execPathFragment = execPathFragment.relativeTo(workspace); + try { + repositoryName = RepositoryName.create("@" + workspace); + } catch (LabelSyntaxException e) { + throw new IllegalStateException(workspace + " is not a valid repository name"); + } } else { problems.add(execPathFragment.getPathString()); continue; } } - Artifact artifact = allowedDerivedInputsMap.get(execPathFragment); + Artifact artifact = allowedDerivedInputsMap.get( + repositoryName.getPathUnderExecRoot().getRelative(execPathFragment)); if (artifact == null) { - artifact = artifactResolver.resolveSourceArtifact(execPathFragment, RepositoryName.MAIN); + artifact = artifactResolver.resolveSourceArtifact(execPathFragment, repositoryName); } if (artifact != null) { inputs.add(artifact); @@ -1003,7 +1021,7 @@ public class CppCompileAction extends AbstractAction problems.add(execPathFragment.getPathString()); } } - //TODO(b/22551695): Remove in favor of seperate implementations. + //TODO(b/22551695): Remove in favor of separate implementations. if (semantics == null || semantics.needsIncludeValidation()) { problems.assertProblemFree(this, getSourceFile()); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java index f5e0ab5f39..7e7e83ae21 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java @@ -485,7 +485,7 @@ public class CppHelper { Preconditions.checkState(Link.SHARED_LIBRARY_FILETYPES.matches(artifact.getFilename())); symlinkedArtifacts.add(isCppRuntime ? SolibSymlinkAction.getCppRuntimeSymlink( - ruleContext, artifact, solibDirOverride, configuration) + ruleContext, artifact, solibDirOverride, configuration) : SolibSymlinkAction.getDynamicLibrarySymlink( ruleContext, artifact, false, true, configuration)); } @@ -494,7 +494,8 @@ public class CppHelper { } return ImmutableList.of( factory.createMiddlemanAllowMultiple(ruleContext.getAnalysisEnvironment(), actionOwner, - ruleContext.getPackageDirectory(), purpose, artifacts, + ruleContext.getLabel().getPackageIdentifier().getPathUnderExecRoot(), purpose, + artifacts, configuration.getMiddlemanDirectory(ruleContext.getRule().getRepository()))); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java index 512281ad93..e77d02a71d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionExecutionException; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactResolver; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; @@ -93,7 +94,7 @@ public class HeaderDiscovery { return inputs.build(); } List systemIncludePrefixes = permittedSystemIncludePrefixes; - + RepositoryName repositoryName = RepositoryName.MAIN; // Check inclusions. IncludeProblems problems = new IncludeProblems(); for (Path execPath : depSet.getDependencies()) { @@ -111,16 +112,24 @@ public class HeaderDiscovery { // non-system include paths here should never be absolute. If they // are, it's probably due to a non-hermetic #include, & we should stop // the build with an error. + // funky but tolerable path if (execPath.startsWith(execRoot)) { - execPathFragment = execPath.relativeTo(execRoot); // funky but tolerable path - } else { - problems.add(execPathFragment.getPathString()); - continue; + execPathFragment = execPath.relativeTo(execRoot); + } else if (execPath.startsWith(execRoot.getParentDirectory())) { + // External repository. + execPathFragment = execPath.relativeTo(execRoot.getParentDirectory()); + String workspace = execPathFragment.getSegment(0); + execPathFragment = execPathFragment.relativeTo(workspace); + try { + repositoryName = RepositoryName.create("@" + workspace); + } catch (LabelSyntaxException e) { + throw new IllegalStateException(workspace + " is not a valid repository name"); + } } } Artifact artifact = allowedDerivedInputsMap.get(execPathFragment); if (artifact == null) { - artifact = artifactResolver.resolveSourceArtifact(execPathFragment, RepositoryName.MAIN); + artifact = artifactResolver.resolveSourceArtifact(execPathFragment, repositoryName); } if (artifact != null) { inputs.add(artifact); -- cgit v1.2.3