Skip to content

buildozer: Respect .bazelignore#1343

Merged
vladmos merged 4 commits intobazelbuild:mainfrom
amartani:martani/bazelignore
May 22, 2025
Merged

buildozer: Respect .bazelignore#1343
vladmos merged 4 commits intobazelbuild:mainfrom
amartani:martani/bazelignore

Conversation

@amartani
Copy link
Contributor

@amartani amartani commented Mar 7, 2025

Currently, when expanding /... targets, buildozer does not respect .bazelignore files, which can be surprising. This implements reading .bazelignore if it exists and ignoring any paths from it to match bazel's behavior.

Note that we only do prefix comparison, with no support for globs, as that is bazel's behavior. We also ignore any absolute paths, which, in bazel itself, results in an error. Reference implementation:

https://github.com/bazelbuild/bazel/blob/8dbfcfae924b014366a0b47cd26632c5076b51a2/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredSubdirectoriesFunction.java#L76-L103

Part of #623 (this only applies for buildozer)

@amartani amartani force-pushed the martani/bazelignore branch 2 times, most recently from 121595d to ef60cb0 Compare March 7, 2025 00:35
@amartani amartani force-pushed the martani/bazelignore branch from ef60cb0 to 8d38f91 Compare March 7, 2025 00:40
@amartani amartani marked this pull request as ready for review March 7, 2025 00:46
@amartani
Copy link
Contributor Author

Hi @vladmos, are you available to review this PR? Thank you!

rel = filepath.ToSlash(rel)

for _, prefix := range ignoredPrefixes {
if strings.HasPrefix(rel, prefix) {
Copy link
Member

Choose a reason for hiding this comment

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

Will foobar be ignored if foo is among ignored prefixes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, good call. I changed the comparison to only accept exact match or match with prefix+"/", and changed getIgnoredPaths to Clean the paths so that / suffixes in .bazelignore are stripped from it. 111bc6e

@amartani amartani requested a review from vladmos April 2, 2025 18:37
@vladmos
Copy link
Member

vladmos commented Apr 3, 2025

Could you please add a boolean option to Opts (e.g. UseBazelignore), with a default value of true, and propagate it from appendCommands to targetExpressionToBuildFiles? And, if it's set to false, use an empty slice instead of calling getIgnoredPrefixes.

This is a useful feature for the open-source version of buildozer, however, not required by the internal one and may add some unnecessary latency for file operations there, it'd be nice if it can be configurable.

@amartani
Copy link
Contributor Author

amartani commented Apr 4, 2025

Could you please add a boolean option to Opts (e.g. UseBazelignore), with a default value of true, and propagate it from appendCommands to targetExpressionToBuildFiles? And, if it's set to false, use an empty slice instead of calling getIgnoredPrefixes.

This is a useful feature for the open-source version of buildozer, however, not required by the internal one and may add some unnecessary latency for file operations there, it'd be nice if it can be configurable.

@vladmos PTAL; I named RespectBazelignore since it feels clearer to me on how it is used, and it aligns with what I see in other tools (eg. respect-gitignore on ruff) but let me know if you prefer to UseBazelignore. Also exposed it as a command-line flag and added a shortcut conditional on shouldIgnorePath to minimize any impact on the common case of not using .bazelignore.

Copy link
Member

@vladmos vladmos left a comment

Choose a reason for hiding this comment

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

Thanks! I'll merge it within a couple of days, there's currently another chunk of changes being imported at the moment, don't want multiple imports to interfere with each other.

@amartani
Copy link
Contributor Author

@vladmos no particular hurry, but is this unblock to be merged? Thank you!

@vladmos
Copy link
Member

vladmos commented May 22, 2025

Sorry for the delay, merging it.

@vladmos vladmos merged commit 9a75f39 into bazelbuild:main May 22, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants