Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,19 @@ public abstract class BuildLanguageOptions extends OptionsBase {
+ " attributes of symbolic macros or attribute default values.")
public abstract boolean getIncompatibleSimplifyUnconditionalSelectsInRuleAttrs();

@Option(
name = "incompatible_check_external_repo_source_dir_package_boundary",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
help =
"If true, a source directory in an external repository fails the build if it crosses a"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a quick comment: is "source directory" the well-known name for this concept? This sentence is very confusing to me without reading the integration set up in SourceDirectoryIntegrationTest.java ("how can a directory cross package boundaries?").

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the rollout flag was named "track source directories" and this has been the term used for them (unfortunately we say tree artifact instead of generated directory...)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source directory / package interaction is legitimately confusing. Happy to reword it you think that this should be clarified, I'm just not sure how.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks for the context. I don't have a much better idea either to be honest, so let's keep it as is for now.

+ " package boundary into a sub-package, matching the behavior that already applies"
+ " to source directories in the main repository. If false, such package boundary"
+ " crossings inside external repositories are not detected.")
public abstract boolean getIncompatibleCheckExternalRepoSourceDirPackageBoundary();

@Option(
name = "experimental_enable_starlark_set",
defaultValue = "true",
Expand Down Expand Up @@ -859,6 +872,9 @@ EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT, getExperimentalSiblingRepositoryLayout()
.setBool(
INCOMPATIBLE_ALWAYS_CHECK_DEPSET_ELEMENTS,
getIncompatibleAlwaysCheckDepsetElements())
.setBool(
INCOMPATIBLE_CHECK_EXTERNAL_REPO_SOURCE_DIR_PACKAGE_BOUNDARY,
getIncompatibleCheckExternalRepoSourceDirPackageBoundary())
.setBool(INCOMPATIBLE_DISALLOW_EMPTY_GLOB, getIncompatibleDisallowEmptyGlob())
.setBool(
INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX,
Expand Down Expand Up @@ -1050,6 +1066,8 @@ public FlagConsumer setBool(String key, boolean ignored) {
"-experimental_sibling_repository_layout";
public static final String INCOMPATIBLE_ALWAYS_CHECK_DEPSET_ELEMENTS =
"+incompatible_always_check_depset_elements";
public static final String INCOMPATIBLE_CHECK_EXTERNAL_REPO_SOURCE_DIR_PACKAGE_BOUNDARY =
"-incompatible_check_external_repo_source_dir_package_boundary";

// Note that INCOMPATIBLE_DISALLOW_EMPTY_GLOB differs in Google and in OSS Bazel.
public static final String INCOMPATIBLE_DISALLOW_EMPTY_GLOB = "+incompatible_disallow_empty_glob";
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -2292,6 +2292,7 @@ java_library(
":dirents",
":filesystem_keys",
":package_lookup_value",
":precomputed_value",
":sky_functions",
":tree_artifact_value",
"//src/main/java/com/google/devtools/build/lib/actions",
Expand All @@ -2304,6 +2305,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/io:file_symlink_infinite_expansion_uniqueness_function",
"//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
Expand All @@ -2313,6 +2315,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//src/main/java/net/starlark/java/eval",
"//src/main/protobuf:failure_details_java_proto",
"//third_party:auto_value",
"//third_party:error_prone_annotations",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import com.google.devtools.build.lib.io.FileSymlinkInfiniteExpansionUniquenessFunction;
import com.google.devtools.build.lib.io.InconsistentFilesystemException;
import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.profiler.SilentCloseable;
Expand Down Expand Up @@ -72,6 +73,7 @@
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
import net.starlark.java.eval.StarlarkSemantics;

/** A {@link SkyFunction} to build {@link RecursiveFilesystemTraversalValue}s. */
public final class RecursiveFilesystemTraversalFunction implements SkyFunction {
Expand Down Expand Up @@ -557,6 +559,14 @@ private PkgLookupResult checkIfPackage(
// those are symlinks to other locations (e.g. in the case of a local_repository), so all other
// roots are part of the main repository.
if (rootPath.startsWith(externalDirectory)) {
StarlarkSemantics semantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
if (semantics == null) {
return null;
}
if (!semantics.getBool(
BuildLanguageOptions.INCOMPATIBLE_CHECK_EXTERNAL_REPO_SOURCE_DIR_PACKAGE_BOUNDARY)) {
return PkgLookupResult.directory(traversal, rootInfo);
}
checkState(
rootPath.relativeTo(externalDirectory).isSingleSegment(),
"%s is expected to be directly under %s",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep
"--experimental_dormant_deps=" + rand.nextBoolean(),
"--force_starlark_stack_trace=" + rand.nextBoolean(),
"--incompatible_always_check_depset_elements=" + rand.nextBoolean(),
"--incompatible_check_external_repo_source_dir_package_boundary=" + rand.nextBoolean(),
"--incompatible_disallow_empty_glob=" + rand.nextBoolean(),
"--incompatible_do_not_split_linking_cmdline=" + rand.nextBoolean(),
"--incompatible_enable_deprecated_label_apis=" + rand.nextBoolean(),
Expand Down Expand Up @@ -185,6 +186,9 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
.setBool(BuildLanguageOptions.EXPERIMENTAL_DORMANT_DEPS, rand.nextBoolean())
.setBool(StarlarkSemantics.FORCE_STARLARK_STACK_TRACE, rand.nextBoolean())
.setBool(BuildLanguageOptions.INCOMPATIBLE_ALWAYS_CHECK_DEPSET_ELEMENTS, rand.nextBoolean())
.setBool(
BuildLanguageOptions.INCOMPATIBLE_CHECK_EXTERNAL_REPO_SOURCE_DIR_PACKAGE_BOUNDARY,
rand.nextBoolean())
.setBool(BuildLanguageOptions.INCOMPATIBLE_DISALLOW_EMPTY_GLOB, rand.nextBoolean())
.setBool(BuildLanguageOptions.INCOMPATIBLE_DO_NOT_SPLIT_LINKING_CMDLINE, rand.nextBoolean())
.setBool(BuildLanguageOptions.INCOMPATIBLE_ENABLE_DEPRECATED_LABEL_APIS, rand.nextBoolean())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,74 @@ public void crossingRepoPackageCollision_doesNotFail() throws Exception {
buildTarget("//:consume");
}

/**
* With --incompatible_check_external_repo_source_dir_package_boundary, a source directory in an
* external repository that crosses a package boundary into a sub-package of that same repository
* fails the build, just like in the main repository.
*/
@Test
public void crossingExternalRepoPackageBoundary_withFlag_fails() throws Exception {
if (!AnalysisMock.get().isThisBazel()) {
return;
}
setUpCrossingExternalRepoPackageBoundary();
addOptions("--incompatible_check_external_repo_source_dir_package_boundary");

assertThrows(BuildFailedException.class, () -> buildTarget("//:consume"));
assertContainsEvent("crosses package boundary into package rooted at data/sub");
}

/**
* Without --incompatible_check_external_repo_source_dir_package_boundary (the default), the same
* package boundary crossing inside an external repository is not detected and the build succeeds.
*/
@Test
public void crossingExternalRepoPackageBoundary_withoutFlag_doesNotFail() throws Exception {
if (!AnalysisMock.get().isThisBazel()) {
return;
}
setUpCrossingExternalRepoPackageBoundary();

buildTarget("//:consume");
}

private void setUpCrossingExternalRepoPackageBoundary() throws Exception {
write(
"MODULE.bazel",
"""
bazel_dep(name = "ext")
local_path_override(
module_name = "ext",
path = "ext",
)
""");
write("ext/MODULE.bazel", "module(name = 'ext')");
write(
"ext/BUILD",
"""
filegroup(
name = "dir",
srcs = ["data"],
visibility = ["//visibility:public"],
)
""");
// The presence of this BUILD file makes ext's `data/sub` a package, so the `data` source directory
// crosses a package boundary.
write("ext/data/sub/BUILD");
writeIsoLatin1(getWorkspace().getRelative("ext/data/top"), "content");

write(
"BUILD",
"""
genrule(
name = "consume",
srcs = ["@ext//:dir"],
outs = ["consume.out"],
cmd = "touch $@",
)
""");
}

private static final String GENRULE_EVENT = "Executing genrule //foo:foo";

private void assertInvalidatedByBuild() throws Exception {
Expand Down
Loading