Request to Merge #4
Conversation
Previously we weren't creating input files for labels used in the attribute of a top-level symbolic macro. This meant that if you wrote
```
my_macro(
name = "foo",
srcs = ["input.txt"],
)
```
you'd get an error message claiming that input.txt is not declared and requires an `exports_files()`. (The usages of input.txt by targets declared inside of foo don't count, because implicit input file creation only works for usages of labels outside of symbolic macro bodies.)
This CL fixes this behavior, refactors the relevant logic in Package.java, and adds more test coverage.
Package.java:
- Factor the big for loop from `beforeBuild()` to `createAssumedInputFiles()`. Leave the original for loop in place for test suite accumulation, and pull the `if (discover...)` out of the new call.
- Factor the meat of the logic into a helper, `maybeCreateAssumedInputFile()`, so we can reuse it for both rules and macros (previously there was no loop over references in macros).
- Add tedious javadoc to this tedious logic.
SymbolicMacroTest.java
- Add explanatory comment to `macroCanReferToInputFile()`.
- Expand `macroCannotForceCreationOfImplicitInputFileOnItsOwn()` to check that the target is still not created when the inner usage is in an attr of a MacroInstance rather than a Rule.
PackageFactoryTest.java
- Expand comment in `testSymbolicMacro_macroPreventsImplicitCreationOfInputFilesUnderItsNamespace()`, include coverage for when the reference comes from within a target or submacro inside the symbolic macro itself.
- New test case for the behavior of this CL, `..._macroInstantiationCanForceImplicitCreationOfInputFile()`.
- Drive-by: Add a test case for when a badly named target in a symbolic macro clashes with an implicitly created input file. This behavior may be affected by lazy macro evaluation in the future.
Confirmed that this CL works for the case I originally discovered it on while writing examples documentation.
Fixes bazelbuild#24007.
PiperOrigin-RevId: 686919555
Change-Id: I6313070f76456c0b0b5d5458ca35c89d1d6da33b
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Code Suggestions ✨Explore these optional code suggestions:
|
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues... but I did find this squirrel.
_.-"""-,
.' ..::. `\
/ .::' `'` /
/ .::' .--.=;
| ::' / C ..\
| :: | \ _.)
\ ':| / \
'-, \./ \)\)
`-| );/
'--'-'
Files scanned
| File Path | Reviewed |
|---|---|
| src/main/java/com/google/devtools/build/lib/packages/Package.java | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ✅ Issue Categories
Category Enabled Naming ✅ Database Operations ✅ Documentation ✅ Logging ✅ Error Handling ✅ Systems and Environment ✅ Objects and Data Structures ✅ Readability and Maintainability ✅ Asynchronous Processing ✅ Design Patterns ✅ Third-Party Libraries ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
User description
Note
I'm currently writing a description for your pull request. I should be done shortly (<1 minute). Please don't edit the description field until I'm finished, or we may overwrite each other. If I find nothing to write about, I'll delete this message.
PR Type
Bug fix, Tests
Description
Package.javato correctly handle the creation of implicit input files when using symbolic macros.createAssumedInputFilesandmaybeCreateAssumedInputFileto streamline and document the process.SymbolicMacroTest.javaandPackageFactoryTest.javato verify the new behavior and ensure no implicit input files are created within symbolic macros.Changes walkthrough 📝
Package.java
Refactor and fix implicit input file creation logicsrc/main/java/com/google/devtools/build/lib/packages/Package.java
createAssumedInputFilesandmaybeCreateAssumedInputFilemethods.
SymbolicMacroTest.java
Enhance tests for symbolic macro input file behaviorsrc/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java
PackageFactoryTest.java
Add comprehensive tests for input file creation in macrossrc/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java