[Bug 2175012] New: Review request: iwyu - A tool for use with clang to analyze #includes in C and C++ source files
https://bugzilla.redhat.com/show_bug.cgi?id=2175012
Bug ID: 2175012 Summary: Review request: iwyu - A tool for use with clang to analyze #includes in C and C++ source files Product: Fedora Version: rawhide Status: NEW Component: Package Review Assignee: nobody@fedoraproject.org Reporter: benson_muite@emailplus.org QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
spec: https://download.copr.fedorainfracloud.org/results/fed500/include-what-you-u... srpm: https://download.copr.fedorainfracloud.org/results/fed500/include-what-you-u...
Description: Include what you use" means this: for every symbol (type, function, variable, or macro) that you use in foo.cc (or foo.cpp), either foo.cc or foo.h should include a .h file that exports the declaration of that symbol. (Similarly, for foo_test.cc, either foo_test.cc or foo.h should do the including.) Obviously symbols defined in foo.cc itself are excluded from this requirement.
This puts us in a state where every file includes the headers it needs to declare the symbols that it uses. When every file includes what it uses, then it is possible to edit any file and remove unused headers, without fear of accidentally breaking the upwards dependencies of that file. It also becomes easy to automatically track and update dependencies in the source code.
Fedora Account System Username: fed500
Would like to unretire this package.
https://bugzilla.redhat.com/show_bug.cgi?id=2175012
Ian McInerney ian.s.mcinerney@ieee.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value Status|NEW |ASSIGNED CC| |ian.s.mcinerney@ieee.org Assignee|nobody@fedoraproject.org |ian.s.mcinerney@ieee.org
https://bugzilla.redhat.com/show_bug.cgi?id=2175012
--- Comment #1 from Ian McInerney ian.s.mcinerney@ieee.org --- Some initial comments reading through the spec file (not the formal review yet, I'll do that after these are addressed).
1) Why do you need to specify the LDFLAGS and the prefix path? Both those should be specified by the Fedora build flags properly.
2) I'd suggest modifying the way you exclude the assembly test to be by deleting the file in the prep section instead of modifying the configured CMake files. Additionally, it is more readable if you use a %ifnarch and just have one branch for the conditional.
3) `BuildRequires: python-devel` should be versioned, so it should be `BuildRequires: python3-devel` (according to the guidelines https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_distro_wi...)
4) For F36 and newer, I don't think you need to specify the compilers to CMake directly, it should find clang using the CC and CXX environment variables set by RPM.
https://bugzilla.redhat.com/show_bug.cgi?id=2175012
--- Comment #2 from Tom Stellard tstellar@redhat.com ---
clang_major_ver=$(clang --version | grep version | cut -f3 -d" " | cut -f1 -d".")
Use the %clang_major_version macro instead. This is defined in macros.clang which is part of clang-devel.
for test in ${failed_tests_add_include_argument[@]};do sed -i "/===///a// IWYU_ARGS: -I %{_libdir}/clang/${clang_major_ver}/include/" tests/cxx/$test done for test in ${failed_tests_additional_include_argument[@]};do sed -i "s|-I .|-I . -I %{_libdir}/clang/${clang_major_ver}/include|g" tests/cxx/$test done
There is also a %clang_resource_dir amcro you can use here instead.
https://bugzilla.redhat.com/show_bug.cgi?id=2175012
Felix Wang topazus@outlook.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |topazus@outlook.com
--- Comment #3 from Felix Wang topazus@outlook.com --- Maybe using %autorelease and %autochangelog macro for packaging has some benefits.
https://bugzilla.redhat.com/show_bug.cgi?id=2175012
--- Comment #4 from Benson Muite benson_muite@emailplus.org --- Thanks for the feedback.
- Why do you need to specify the LDFLAGS and the prefix path? Both those should be specified
by the Fedora build flags properly.
Without LDFLAG do not get a position independent executable. Prefix path is suggested in install docs.
- I'd suggest modifying the way you exclude the assembly test to be by deleting the file in the
prep section instead of modifying the configured CMake files. Additionally, it is more readable if you use a %ifnarch and just have one branch for the conditional.
Done
- `BuildRequires: python-devel` should be versioned, so it should be
`BuildRequires: python3-devel` (according to the guidelines https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_distro_wi...)
Done
- For F36 and newer, I don't think you need to specify the compilers to CMake directly,
it should find clang using the CC and CXX environment variables set by RPM.
Done
There is also a %clang_resource_dir macro you can use here instead.
Done
Maybe using %autorelease and %autochangelog macro for packaging has some benefits.
Prefer to manage manually for the time being, commits do not all correspond to changelog entries.
spec: https://download.copr.fedorainfracloud.org/results/fed500/include-what-you-u... srpm: https://download.copr.fedorainfracloud.org/results/fed500/include-what-you-u...
https://bugzilla.redhat.com/show_bug.cgi?id=2175012
Benson Muite benson_muite@emailplus.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |DUPLICATE Last Closed| |2023-04-01 02:18:20
--- Comment #5 from Benson Muite benson_muite@emailplus.org --- https://bugzilla.redhat.com/show_bug.cgi?id=2179648
*** This bug has been marked as a duplicate of bug 2179648 ***
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com