Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement ignoring directories based on wildcards. #24032

Closed
wants to merge 7 commits into from
Closed

Implement ignoring directories based on wildcards. #24032

wants to merge 7 commits into from

Conversation

lberki
Copy link
Contributor

@lberki lberki commented Oct 18, 2024

This is accomplished by a new directive in REPO.bazel, "ignore_directories()". It takes a single argument, a list of directories to ignore and it allows the same wildcards as glob().

This is done separately from .bazelignore to provide a migration path off of that weird single-purpose configuration file.

Implementing this requires splitting RepoFileFunction into two: a part that parses the repository file and one that creates a PackageArgs instance. This was necessary to avoid a Skyframe dependency cycle: when a WORKSPACE file is present and it loads a .bzl file from a repository with a REPO.bazel file, the repo mapping for the main repository depends on the WORKSPACE file, which depends on the .bzl file, which depends on the IgnoredPackagePrefixesValue of its repository, which then depends on the repo mapping of the main repository and the one the .bzl file is in, which then depend on the WORKSPACE file.

Fixes #7093.

RELNOTES[NEW]: REPO.bazel now allows another directive, "ignore_directories()". It takes a list of directories to ignore just like .bazelignore does, but with glob semantics.

@lberki lberki requested a review from fmeum October 18, 2024 06:05
@github-actions github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Oct 18, 2024
This was referenced Oct 18, 2024
Open
Closed
@meteorcloudy
Copy link
Member

Thanks! Waiting for @Wyverald to take a look!

Wyverald
Wyverald reviewed Oct 25, 2024
View reviewed changes
Copy link
Member

@Wyverald Wyverald left a comment

Choose a reason for hiding this comment

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

so I reviewed some more superficial parts of this change before happening on to the whole "splitting RepoFileFunction into two parts" thing. After reading the context in #7093 (comment), my initial gut reaction is that this is a lot of extra complexity that's really not justified by its gains.

Besides Fabian's suggestion to cut edge 8 (which you had a counterargument for), I was also thinking if we could just cut edge 1 -- that is, WORKSPACE is going away anyway, so let's not worry about that. Essentially, we can try to make the assertion that computing the main repo mapping does not transitively depend on IgnoredPackagePrefixesValue.

Unfortunately, that's not true... If we have a non-registry override for a bazel_dep, and the override refers to a patch file (which is addressed by a label), then we'll need to look up the package for that label. (Not to mention we actually need to load @bazel_tools//tools/build_defs/repo:http.bzl anyway, but I was hoping those could be avoided using exceptions -- which we already have quite a few of, regarding stuff in @bazel_tools.)

So all that was a long way to say I haven't finished the review yet, and will get to the meaty parts now.

@lberki lberki requested a review from a team as a code owner October 28, 2024 07:47
@lberki lberki requested review from aranguyen and removed request for a team October 28, 2024 07:47
@lberki
Copy link
Contributor Author

lberki commented Oct 28, 2024

I addressed the review comments and although the presubmits haven't quite passed yet, they look good so far so I'll context switch away from this. PTAL!

@lberki lberki removed the request for review from aranguyen October 28, 2024 09:13
Wyverald
Wyverald reviewed Oct 30, 2024
View reviewed changes
Copy link
Member

@Wyverald Wyverald left a comment

Choose a reason for hiding this comment

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

LGTM overall

src/main/java/com/google/devtools/build/lib/skyframe/PackageArgsFunction.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/PackageArgsFunction.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/PackageArgsFunction.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/PackageArgsFunction.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/PackageArgsFunction.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesFunction.java Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/packages/RepoFileGlobals.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java Outdated Show resolved Hide resolved
@lberki
Copy link
Contributor Author

lberki commented Oct 31, 2024

Comments addressed and tests are green. PTAL!

@lberki
Copy link
Contributor Author

lberki commented Oct 31, 2024

Done and done. PTYAL!

Wyverald
Wyverald approved these changes Oct 31, 2024
View reviewed changes
Copy link
Member

@Wyverald Wyverald left a comment

Choose a reason for hiding this comment

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

few more nits, but overall LGTM! Please import this yourself, as I'd imagine there are a few Blaze-specific changes to be made. Or if you'd rather I do the import, just let me know!

src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java Outdated
@@ -140,7 +136,7 @@ private static String getDisplayNameForRepo(
return displayName;
}

private PackageArgs evalRepoFile(
private RepoFileValue evalRepoFile(
StarlarkFile starlarkFile,
RepositoryName repoName,
RepositoryMapping repoMapping,
Copy link
Member

Choose a reason for hiding this comment

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

ping -- this arg should be removed

src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesFunction.java Show resolved Hide resolved
@lberki
Implement ignoring directories based on wildcards.
3f1c287 
This is accomplished by a new directive in REPO.bazel,
"ignore_directories()". It takes a single argument, a list of
directories to ignore and it allows the same wildcards as glob().

This is done separately from .bazelignore to provide a migration path
off of that weird single-purpose configuration file.

Implementing this requires splitting RepoFileFunction into two: a part
that parses the repository file and one that creates a PackageArgs
instance. This was necessary to avoid a Skyframe dependency cycle: when
a WORKSPACE file is present and it loads a .bzl file from a repository
with a REPO.bazel file, the repo mapping for the main repository depends
on the WORKSPACE file, which depends on the .bzl file, which depends on
the IgnoredPackagePrefixesValue of its repository, which then depends on
the repo mapping of the main repository and the one the .bzl file is in,
which then depend on the WORKSPACE file.

Fixes #7093.

RELNOTES[NEW]: REPO.bazel now allows another directive,
"ignore_directories()". It takes a list of directories to ignore just
like .bazelignore does, but with glob semantics.
@lberki
add missing file
e93e711
@lberki
address review comments
9c09e32
@lberki
Address code review comments
407a171
@lberki
remove useless ctor arguments of RepoThreadContext
2a0d045
@lberki lberki force-pushed the lberki-ignore-wildcard-good branch from aff0fba to f7fa158 Compare November 4, 2024 07:14
@copybara-service copybara-service bot closed this in 9c6d24f Nov 5, 2024
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Nov 5, 2024
lberki added a commit that referenced this pull request Nov 5, 2024
@lberki
Implement ignoring directories based on wildcards.
277edb7 
This is accomplished by a new directive in REPO.bazel, "ignore_directories()". It takes a single argument, a list of directories to ignore and it allows the same wildcards as glob().

This is done separately from .bazelignore to provide a migration path off of that weird single-purpose configuration file.

Implementing this requires splitting RepoFileFunction into two: a part that parses the repository file and one that creates a PackageArgs instance. This was necessary to avoid a Skyframe dependency cycle: when a WORKSPACE file is present and it loads a .bzl file from a repository with a REPO.bazel file, the repo mapping for the main repository depends on the WORKSPACE file, which depends on the .bzl file, which depends on the IgnoredPackagePrefixesValue of its repository, which then depends on the repo mapping of the main repository and the one the .bzl file is in, which then depend on the WORKSPACE file.

Fixes #7093.

RELNOTES[NEW]: REPO.bazel now allows another directive, "ignore_directories()". It takes a list of directories to ignore just like .bazelignore does, but with glob semantics.

Closes #24032.

PiperOrigin-RevId: 693227896
Change-Id: Ia3e02a2bfe9caf999fc641f75261b528b19c1d03
@lberki lberki mentioned this pull request Nov 5, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 5, 2024
@lberki
Implement ignoring directories based on wildcards. (#24203)
00da366 
This is accomplished by a new directive in REPO.bazel,
"ignore_directories()". It takes a single argument, a list of
directories to ignore and it allows the same wildcards as glob().

This is done separately from .bazelignore to provide a migration path
off of that weird single-purpose configuration file.

Implementing this requires splitting RepoFileFunction into two: a part
that parses the repository file and one that creates a PackageArgs
instance. This was necessary to avoid a Skyframe dependency cycle: when
a WORKSPACE file is present and it loads a .bzl file from a repository
with a REPO.bazel file, the repo mapping for the main repository depends
on the WORKSPACE file, which depends on the .bzl file, which depends on
the IgnoredPackagePrefixesValue of its repository, which then depends on
the repo mapping of the main repository and the one the .bzl file is in,
which then depend on the WORKSPACE file.

Fixes #7093.

RELNOTES[NEW]: REPO.bazel now allows another directive,
"ignore_directories()". It takes a list of directories to ignore just
like .bazelignore does, but with glob semantics.

Closes #24032.

PiperOrigin-RevId: 693227896
Change-Id: Ia3e02a2bfe9caf999fc641f75261b528b19c1d03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmeum fmeum fmeum left review comments

@Wyverald Wyverald Wyverald approved these changes

@meteorcloudy meteorcloudy Awaiting requested review from meteorcloudy meteorcloudy is a code owner

Assignees
No one assigned
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FR] .bazelignore should support wildcard prefix
4 participants
@lberki @meteorcloudy @Wyverald @fmeum