Bug 2175012

Summary:	Review request: iwyu - A tool for use with clang to analyze #includes in C and C++ source files
Product:	[Fedora] Fedora	Reporter:	Benson Muite <benson_muite>
Component:	Package Review	Assignee:	Ian McInerney <ian.s.mcinerney>
Status:	CLOSED DUPLICATE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	unspecified	Docs Contact:
Priority:	unspecified
Version:	rawhide	CC:	ian.s.mcinerney, package-review, topazus
Target Milestone:	---	Flags:	ppisar: fedora-review?
Target Release:	---
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:		Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2023-04-01 02:18:20 UTC	Type:	Bug
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

Description Benson Muite 2023-03-02 20:44:41 UTC

spec: https://download.copr.fedorainfracloud.org/results/fed500/include-what-you-use/fedora-rawhide-i386/05587380-iwyu/iwyu.spec
srpm: https://download.copr.fedorainfracloud.org/results/fed500/include-what-you-use/fedora-rawhide-i386/05587380-iwyu/iwyu-0.19-1.fc39.src.rpm

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.

Comment 1 Ian McInerney 2023-03-02 23:42:03 UTC

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_wide_guidelines)

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.

Comment 2 Tom Stellard 2023-03-02 23:47:53 UTC

> 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.

Comment 3 Felix Wang 2023-03-03 00:41:34 UTC

Maybe using %autorelease and %autochangelog macro for packaging has some benefits.

Comment 4 Benson Muite 2023-03-03 07:54:05 UTC

Thanks for the feedback.
> 1) 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.

> 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.
Done

> 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_wide_guidelines)
Done

> 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.
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-use/fedora-rawhide-x86_64/05588697-iwyu/iwyu.spec
srpm: https://download.copr.fedorainfracloud.org/results/fed500/include-what-you-use/fedora-rawhide-x86_64/05588697-iwyu/iwyu-0.19-2.fc39.src.rpm

Comment 5 Benson Muite 2023-04-01 02:18:20 UTC

https://bugzilla.redhat.com/show_bug.cgi?id=2179648

*** This bug has been marked as a duplicate of bug 2179648 ***