Try to prevent sideeffects during macro expansion #499
Conversation
Summary of ChangesHello @nforro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where RPM macro expansion could inadvertently introduce side effects by modifying other macros, particularly problematic when processing spec files line by line where even unexecuted conditional branches are expanded. To mitigate this, a new "safe" expansion mode has been implemented in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'safe' mode for macro expansion to prevent side effects, which is a good approach to solve the described issue. The implementation correctly overrides and restores macros that could modify other macros. The accompanying test case effectively validates the new functionality. I've provided a couple of suggestions in specfile/macros.py to improve code readability and efficiency, mainly by simplifying the try-except-finally structure and using a more idiomatic loop for cleanup operations.
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 55s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 03s |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a 'safe' mode for macro expansion to prevent side effects, which is a solid approach to address the problem of unintended macro modifications during spec file processing. The implementation correctly uses a try...finally block to ensure the state of macros is restored after expansion. The accompanying test case effectively verifies the new functionality. I have one suggestion to make the cleanup logic more idiomatic and efficient.
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 00s |
Macro expansion can have sideeffects and modify other macros, which becomes a problem when the spec file is processed line by line to determine line validity. Even lines in false condition branches are expanded, and if the expansion has sideeffects it can affect further processing and condition evaluation. Try to prevent that by temporarily overriding macros that can affect other macros, making them noop during expansion. Signed-off-by: Nikola Forró <[email protected]>
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 59s |
|
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 1m 58s |
Macro expansion can have sideeffects and modify other macros, which becomes a problem when the spec file is processed line by line to determine line validity. Even lines in false condition branches are expanded, and if the expansion has sideeffects it can affect further processing and condition evaluation.
Try to prevent that by temporarily overriding macros that can affect other macros, making them noop during expansion.
Unfortunately overriding doesn't work on EL9 and EL8 where RPM behaves differently.
Fixes #498.