@coderabbitai[bot] commented on this pull request.
**Actionable comments posted: 2**
<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CMakeLists.txt`:
- Around line 322-325: Update the FIND_DEBUGINFO_VERSION condition to use
CMake’s VERSION_GREATER_EQUAL comparison so dotted versions such as 5.10 are
ordered correctly, while preserving the existing FIND_DEBUGINFO_OPTS
assignments.
In `@docs/man/rpm-spec.5.scd`:
- Around line 577-584: Update the `_PATH_` argument glossary for the `*%files*`
`*-f*` option to allow relative pathnames as well as absolute pathnames,
matching the syntax permitted by this section and removing the conflicting
definition.
```
</details>
<details>
<summary>🪄 Autofix (Beta)</summary>
Fix all unresolved CodeRabbit comments on this PR:
- [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a
commit to this branch (recommended)
- [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a
new PR with the fixes
</details>
---
<details>
<summary>ℹ️ Review info</summary>
<details>
<summary>⚙️ Run configuration</summary>
**Configuration used**: defaults
**Review profile**: CHILL
**Plan**: Pro Plus
**Run ID**: `7e23a579-b955-4e9e-b3a4-30d489243feb`
</details>
<details>
<summary>📥 Commits</summary>
Reviewing files that changed from the base of the PR and between
c1fe256483b4802af27c2fe67a31443ac4045bd4 and
208e7d008f07d9d369c49715ec159de1c6530254.
</details>
<details>
<summary>📒 Files selected for processing (12)</summary>
* `CMakeLists.txt`
* `build/build.cc`
* `build/files.cc`
* `build/parsePreamble.cc`
* `build/rpmbuild_internal.hh`
* `build/spec.cc`
* `docs/man/rpm-scriptlets.7.scd`
* `docs/man/rpm-spec.5.scd`
* `macros.in`
* `tests/Dockerfile.fedora`
* `tests/data/SPECS/readin.spec`
* `tests/rpmbuild.at`
</details>
</details>
<!-- This is an auto-generated comment by CodeRabbit for review status -->
> +if (FIND_DEBUGINFO_VERSION GREATER_EQUAL 5.3)
+ set(FIND_DEBUGINFO_OPTS "--output-dir \"%{_debuginfo_manifests_dir}\"")
+else()
+ set(FIND_DEBUGINFO_OPTS "")
_🎯 Functional Correctness_ | _🟠 Major_ | _⚡ Quick win_
<details>
<summary>🧩 Analysis chain</summary>
🌐 Web query:
`CMake documentation: compare the behavior of `GREATER_EQUAL` and
`VERSION_GREATER_EQUAL` for the values `5.10` and `5.3`.`
💡 Result:
In CMake, `GREATER_EQUAL` and `VERSION_GREATER_EQUAL` use fundamentally
different comparison logic, which leads to different results for the values
`5.10` and `5.3` [1]. GREATER_EQUAL This operator treats its arguments as real
numbers (like C doubles) [1]. It evaluates whether the value on the left is
numerically greater than or equal to the value on the right. For `5.10` and
`5.3`, CMake treats these as the numbers 5.10 and 5.3. Since 5.10 is less than
5.3, the expression `5.10 GREATER_EQUAL 5.3` evaluates to false.
VERSION_GREATER_EQUAL This operator performs a component-wise integer
comparison [1]. It parses the strings into version components
(major[.minor[.patch[.tweak]]]) and compares them sequentially as integers [1].
For `5.10` and `5.3`: 1. It compares the first components: 5 equals 5. 2. It
compares the second components: 10 is greater than 3. Because the second
component (10) is greater than the second component (3), the expression `5.10
VERSION_GREATER_EQUAL 5.3` evaluates to true. Recommendation Always use
`VERSION_GREATER_EQUAL` (or related `VERSION_*` operators) when comparing
version numbers [2]. Comparing versions as floating-point numbers can lead to
incorrect results, particularly when minor version numbers are 10 or higher [2].
Citations:
- 1: https://cmake.org/cmake/help/latest/command/if.html
- 2: https://cmake.org/cmake/help/latest/variable/CMAKE_VERSION.html
---
🏁 Script executed:
```shell
#!/bin/bash
set -euo pipefail
printf 'CMakeLists.txt sections mentioning FIND_DEBUGINFO_VERSION:\n'
rg -n
"FIND_DEBUGINFO_VERSION|find-debuginfo|VERSION_GREATER|GREATER_EQUAL|output-dir"
CMakeLists.txt || true
printf '\nContext around CMakeLists.txt lines 308-330:\n'
sed -n '308,330p' CMakeLists.txt | nl -ba -v308
```
Repository: rpm-software-management/rpm
Length of output: 579
---
🌐 Web query:
`rpm-software-management rpm CMakeLists.txt find-debuginfo DEBUGINFO_OPTIONAL`
💡 Result:
Based on the analysis of the RPM software management ecosystem,
"DEBUGINFO_OPTIONAL" is not a standard configuration variable, macro, or flag
recognized by the upstream RPM build process or the find-debuginfo utility
[1][2][3][4][5]. The find-debuginfo script is the primary tool used by RPM for
generating debug information packages, and it is controlled by various
macro-based options (such as _find_debuginfo_opts) [2][6][5]. The
CMakeLists.txt file in the RPM repository uses find_program to locate the
find-debuginfo utility on the system [7][8][9]. If you are encountering a
reference to DEBUGINFO_OPTIONAL, it is likely specific to a custom build
system, a third-party project wrapper (like a specific CPack or CMake setup),
or a misinterpretation of a macro name. In the context of RPM packaging,
debuginfo generation is typically controlled by macros such as:
%_enable_debug_packages (to enable or disable debuginfo generation) [3][4][5]
%_find_debuginfo_opts (to pass additional arguments to the find-debuginfo
script) [2][5] If you intended to make debuginfo generation optional in a
specific build, the standard practice is to use %global debug_package %{nil} or
undefine _enable_debug_packages in the spec file to disable the process for
that specific package [6][5][10].
Citations:
- 1:
https://github.com/rpm-software-management/rpm/blob/7cc9eb84a3b2baa0109be599572d78870e0dd3fe/scripts/find-debuginfo.sh
- 2:
https://github.com/rpm-software-management/rpm/blob/b2e8a2bed582a9ccc70fbbf3c8586bdd712ea4d1/macros.in
- 3: https://rpm.org/docs/latest/man/rpmbuild-config.5
- 4: https://rpm.org/docs/4.20.x/manual/buildprocess.html
- 5:
https://git.almalinux.org/rpms/redhat-rpm-config/commit/ff647eda5385d5c41be7c6329e65885b47f2852e
- 6: https://github.com/rpm-software-management/rpm/issues/2204
- 7:
https://github.com/rpm-software-management/rpm/blob/rpm-6.0.1-release/CMakeLists.txt
- 8:
https://github.com/rpm-software-management/rpm/blob/rpm-4.19.0-release/CMakeLists.txt
- 9:
https://github.com/rpm-software-management/rpm/commit/5ac27313a5ecd601e01393cda10e6f16728a434a
- 10: https://github.com/rpm-software-management/rpm/issues/3057
---
</details>
**Use a CMake version comparison.**
`GREATER_EQUAL` treats version values as numbers, so `5.10` compares as smaller
than `5.3` and skips `--output-dir` for newer supported `debugedit` versions.
Use `VERSION_GREATER_EQUAL` for this comparison.
<details>
<summary>Proposed fix</summary>
```diff
-if (FIND_DEBUGINFO_VERSION GREATER_EQUAL 5.3)
+if (FIND_DEBUGINFO_VERSION VERSION_GREATER_EQUAL 5.3)
```
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately
> replaces the highlighted code, contains no missing lines, and has no issues
> with indentation. Thoroughly test & benchmark the code to ensure it meets the
> requirements.
```suggestion
if (FIND_DEBUGINFO_VERSION VERSION_GREATER_EQUAL 5.3)
set(FIND_DEBUGINFO_OPTS "--output-dir \"%{_debuginfo_manifests_dir}\"")
else()
set(FIND_DEBUGINFO_OPTS "")
```
</details>
<!-- suggestion_end -->
<details>
<summary>🤖 Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CMakeLists.txt` around lines 322 - 325, Update the FIND_DEBUGINFO_VERSION
condition to use CMake’s VERSION_GREATER_EQUAL comparison so dotted versions
such as 5.10 are ordered correctly, while preserving the existing
FIND_DEBUGINFO_OPTS assignments.
```
</details>
<!-- fingerprinting:phantom:poseidon:terra -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:5b9261cdb74be1b270646b74 -->
<!-- This is an auto-generated comment by CodeRabbit -->
> +*%files* [*-n*] [_SUBNAME_] [*-f* _PATH_ [*-f* _PATH_ ...]]
A manifest describing the files in the (sub-)package.
- *-f* _FILE_
+ *-f* _PATH_
Additional files manifest to be read from disk after
the build scripts have completed.
May be supplied multiple times.
+ See *READ-IN FILES* for details.
_📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_
**Correct the `_PATH_` definition.**
The new `-f` syntax accepts relative paths, but the argument glossary defines
`_PATH_` as an absolute pathname. The rendered manual gives conflicting input
rules.
Change the glossary text to allow a relative pathname where the section permits
it.
<details>
<summary>🤖 Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/man/rpm-spec.5.scd` around lines 577 - 584, Update the `_PATH_`
argument
glossary for the `*%files*` `*-f*` option to allow relative pathnames as well as
absolute pathnames, matching the syntax permitted by this section and removing
the conflicting definition.
```
</details>
<!-- fingerprinting:phantom:poseidon:terra -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:3dd0810db32c422e1ab86da5 -->
<!-- This is an auto-generated comment by CodeRabbit -->
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/4293#pullrequestreview-4829951885
You are receiving this because you are subscribed to this thread.
Message ID: <rpm-software-management/rpm/pull/4293/review/[email protected]>_______________________________________________
Rpm-maint mailing list
[email protected]
https://lists.rpm.org/mailman/listinfo/rpm-maint
