diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java index f5ec733c271967..76f13a4e8e86b4 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java @@ -164,6 +164,8 @@ public PathFragment getPackageFragment() { * Returns a path to the source code for this package relative to the corresponding source root. * Returns pkgName for all repositories. */ + // TODO(bazel-team): This name is misleading, since the returned object is not a source root. + // Maybe "getSourceRootRelative()"? public PathFragment getSourceRoot() { return pkgName; } diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index e9d68f06a5757d..542e3d748d5596 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java @@ -159,6 +159,8 @@ public enum ConfigSettingVisibilityPolicy { private final Metadata metadata; + private final Optional sourceRoot; + // For BUILD files, this is initialized immediately. For WORKSPACE files, it is known only after // Starlark evaluation of the WORKSPACE file has finished. private String workspaceName; @@ -172,11 +174,6 @@ public enum ConfigSettingVisibilityPolicy { // Mutated during BUILD file evaluation (but not by symbolic macro evaluation). private ImmutableMap makeEnv; - // Computed in finishInit. - // TODO(bazel-team): There's no real reason this can't be computed sooner, reducing the set of - // things determined during/after BUILD file evaluation. - private Optional sourceRoot; - // These two fields are mutually exclusive. Which one is set depends on // PackageSettings#precomputeTransitiveLoads. See Package.Builder#setLoads. @Nullable private ImmutableList directLoads; @@ -282,6 +279,7 @@ public enum ConfigSettingVisibilityPolicy { // BUILD evaluation. private Package(Metadata metadata) { this.metadata = metadata; + this.sourceRoot = computeSourceRoot(metadata); } // ==== General package metadata accessors ==== @@ -704,45 +702,60 @@ public MacroInstance getDeclaringMacroForTarget(String target) { // ==== Initialization ==== + private static Optional computeSourceRoot(Metadata metadata) { + if (metadata.isRepoRulePackage()) { + return Optional.empty(); + } + + RootedPath buildFileRootedPath = metadata.buildFilename(); + Root buildFileRoot = buildFileRootedPath.getRoot(); + PathFragment pkgIdFragment = metadata.packageIdentifier().getSourceRoot(); + PathFragment pkgDirFragment = buildFileRootedPath.getRootRelativePath().getParentDirectory(); + + Root sourceRoot; + if (pkgIdFragment.equals(pkgDirFragment)) { + // Fast path: BUILD file path and package name are the same, don't create an extra root. + sourceRoot = buildFileRoot; + } else { + // TODO(bazel-team): Can this expr be simplified to just pkgDirFragment? + PathFragment current = buildFileRootedPath.asPath().asFragment().getParentDirectory(); + for (int i = 0, len = pkgIdFragment.segmentCount(); i < len && current != null; i++) { + current = current.getParentDirectory(); + } + if (current == null || current.isEmpty()) { + // This is never really expected to work. The below check should fail. + sourceRoot = buildFileRoot; + } else { + // Note that current is an absolute path. + sourceRoot = Root.fromPath(buildFileRoot.getRelative(current)); + } + } + + Preconditions.checkArgument( + sourceRoot.asPath() != null + && sourceRoot.getRelative(pkgIdFragment).equals(metadata.getPackageDirectory()), + "Invalid BUILD file name for package '%s': %s (in source %s with packageDirectory %s and" + + " package identifier source root %s)", + metadata.packageIdentifier(), + metadata.buildFilename(), + sourceRoot, + metadata.getPackageDirectory(), + metadata.packageIdentifier().getSourceRoot()); + + return Optional.of(sourceRoot); + } + /** * Completes the initialization of this package. Only after this method may a package by shared * publicly. */ private void finishInit(Builder builder) { - String baseName = metadata.buildFilename().getRootRelativePath().getBaseName(); - this.containsErrors |= builder.containsErrors; if (directLoads == null && transitiveLoads == null) { Preconditions.checkState(containsErrors, "Loads not set for error-free package"); builder.setLoads(ImmutableList.of()); } - if (isWorkspaceFile(baseName) || isModuleDotBazelFile(baseName)) { - Preconditions.checkState(isRepoRulePackage()); - this.sourceRoot = Optional.empty(); - } else { - Root sourceRoot = - computeSourceRoot(metadata.buildFilename(), metadata.packageIdentifier().getSourceRoot()); - if (sourceRoot.asPath() == null - || !sourceRoot - .getRelative(metadata.packageIdentifier().getSourceRoot()) - .equals(metadata.getPackageDirectory())) { - throw new IllegalArgumentException( - "Invalid BUILD file name for package '" - + metadata.packageIdentifier() - + "': " - + metadata.buildFilename() - + " (in source " - + sourceRoot - + " with packageDirectory " - + metadata.getPackageDirectory() - + " and package identifier source root " - + metadata.packageIdentifier().getSourceRoot() - + ")"); - } - this.sourceRoot = Optional.of(sourceRoot); - } - this.workspaceName = builder.workspaceName; this.makeEnv = ImmutableMap.copyOf(builder.makeEnv); @@ -774,34 +787,6 @@ private void finishInit(Builder builder) { this.packageOverhead = overheadEstimate.orElse(PACKAGE_OVERHEAD_UNSET); } - private static boolean isWorkspaceFile(String baseFileName) { - return baseFileName.equals(LabelConstants.WORKSPACE_DOT_BAZEL_FILE_NAME.getPathString()) - || baseFileName.equals(LabelConstants.WORKSPACE_FILE_NAME.getPathString()); - } - - private static boolean isModuleDotBazelFile(String baseFileName) { - return baseFileName.equals(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME.getPathString()); - } - - private static Root computeSourceRoot( - RootedPath buildFileRootedPath, PathFragment packageFragment) { - PathFragment packageDirectory = buildFileRootedPath.getRootRelativePath().getParentDirectory(); - if (packageFragment.equals(packageDirectory)) { - // Fast path: BUILD file path and package name are the same, don't create an extra root. - return buildFileRootedPath.getRoot(); - } - PathFragment current = buildFileRootedPath.asPath().asFragment().getParentDirectory(); - for (int i = 0, len = packageFragment.segmentCount(); i < len && current != null; i++) { - current = current.getParentDirectory(); - } - if (current == null || current.isEmpty()) { - // This is never really expected to work. The check below in #finishInit should fail. - return buildFileRootedPath.getRoot(); - } - // Note that current is an absolute path. - return Root.fromPath(buildFileRootedPath.getRoot().getRelative(current)); - } - // TODO(bazel-team): This is a mutation, but Package is supposed to be immutable. In practice it // seems like this is only called during Package construction (including deserialization), // principally via Rule#reportError. I would bet that all of the callers have access to a @@ -2770,6 +2755,16 @@ public record Metadata( Preconditions.checkNotNull(repositoryMapping); Preconditions.checkNotNull(associatedModuleName); Preconditions.checkNotNull(associatedModuleVersion); + + // Check for consistency between isRepoRulePackage and whether the buildFilename is a + // WORKSPACE / MODULE.bazel file. + String baseName = buildFilename.asPath().getBaseName(); + boolean isWorkspaceFile = + baseName.equals(LabelConstants.WORKSPACE_DOT_BAZEL_FILE_NAME.getPathString()) + || baseName.equals(LabelConstants.WORKSPACE_FILE_NAME.getPathString()); + boolean isModuleDotBazelFile = + baseName.equals(LabelConstants.MODULE_DOT_BAZEL_FILE_NAME.getPathString()); + Preconditions.checkArgument(isRepoRulePackage == (isWorkspaceFile || isModuleDotBazelFile)); } /** diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageTest.java index 8b56fdc44621c9..6f34cf7fdbf911 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/PackageTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/PackageTest.java @@ -162,7 +162,8 @@ private Package.Builder pkgBuilder(String name) { PackageSettings.DEFAULTS, PackageIdentifier.createInMainRepo(name), /* filename= */ RootedPath.toRootedPath( - Root.fromPath(fileSystem.getPath("/irrelevantRoot")), PathFragment.create(name)), + Root.fromPath(fileSystem.getPath("/irrelevantRoot")), + PathFragment.create(name + "/BUILD")), "workspace", Optional.empty(), Optional.empty(), diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java index 73f13601f2eb8d..8bbc73d6828036 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java @@ -219,7 +219,7 @@ private static void assertAttr(RuleClass ruleClass, String attrName, Type typ @Test public void testOutputFileNotEqualDot() { - Path myPkgPath = scratch.resolve("/workspace/mypkg"); + Path myPkgPath = scratch.resolve("/workspace/mypkg/BUILD"); Package.Builder pkgBuilder = newBuilder(PackageIdentifier.createInMainRepo("mypkg"), myPkgPath); Map attributeValues = new HashMap<>();