Bug 1091659 - Review Request: iwyu - #include analysis tool
Summary: Review Request: iwyu - #include analysis tool
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Christopher Meng
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-04-27 03:54 UTC by Dave Johansen
Modified: 2015-03-08 22:44 UTC (History)
7 users (show)

Fixed In Version: iwyu-0.2-1.el7
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-02-27 09:23:16 UTC
Type: ---
i: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Dave Johansen 2014-04-27 03:54:34 UTC
Spec URL: http://daveisfera.fedorapeople.org/iwyu_3.4/include-what-you-use.spec
SRPM URL: http://daveisfera.fedorapeople.org/iwyu_3.4/include-what-you-use-3.4-1.el6.src.rpm
Description: A tool for use with clang to analyze #includes in C and C++ source files
Fedora Account System Username: daveisfera

Comment 1 Christopher Meng 2014-04-27 04:25:06 UTC
1. iwyu or this loooooooooong name?

2. Why cmake28?

3. %cmake28 -DLLVM_LIB_PATH=/usr/lib/llvm ..

Wwhat about 64 bits arch?

3. Drop rm -rf %{buildroot}, %clean.

4. %{buildroot}/ --> %{buildroot}

Comment 2 Dave Johansen 2014-04-27 05:26:22 UTC
> 1. iwyu or this loooooooooong name?
I was just trying to match the upstream name, but I agree that it's annoyingly long so I changed it to iwyu.

> 2. Why cmake28?
The upstream source said it was required, but I just tried it with 2.6 and it worked just fine, so I added a patch to use cmake 2.6.

> 3. %cmake28 -DLLVM_LIB_PATH=/usr/lib/llvm ..
> What about 64 bits arch?
Fixed to use %{_libdir} and I noticed some missing BuildRequires when testing this change, so I added those as well.

> 3. Drop rm -rf %{buildroot}, %clean.
Sorry, I'm developing on EL6 and the rpmlint from there still warns about that sort of stuff. However, since I'm not intending on supporting EL5 (just Fedora and EL6), I removed those lines since they're not required and just clutter things up.

> 4. %{buildroot}/ --> %{buildroot}
I believe that I made the requested change (i.e. removed the / between %{buildroot} and %{_bindir}), but please let me know if there's something else.

The updated .spec and source RPM can be found at:
http://daveisfera.fedorapeople.org/iwyu_3.4/iwyu.spec
http://daveisfera.fedorapeople.org/iwyu_3.4/iwyu-3.4-1.el6.src.rpm

Comment 3 Christopher Meng 2014-05-19 06:04:23 UTC
1. Summary suggestion:

C/C++ source files #include analyzer based on clang

2. %{_bindir}/fix_includes.py

Maybe you can rename it to %{_bindir}/fix_includes? I think this name is shorter :)

Comment 4 Dave Johansen 2014-05-20 03:39:37 UTC
I made those changes and updated the .spec and source rpm at the links mentioned in comment 2.

Comment 5 Dave Johansen 2014-06-03 03:05:54 UTC
I added running of the tests to the check section and fixed the description because the 3rd line was incorrectly being viewed as a comment (is there a way to escape the # other than putting it later on the line like I did?).

Comment 6 Christopher Meng 2014-06-18 05:09:12 UTC
I think you need to request a permission:

http://fedoraproject.org/wiki/Packaging:Guidelines#Statically_Linking_Executables

Comment 7 Dave Johansen 2014-09-02 16:36:38 UTC
Sorry for the delayed action on this. Rather than getting approval to use the static libraries from clang, I've been working on trying to get the shared libraries built so those could be used by iwyu and other applications.

Comment 8 Dave Johansen 2014-11-28 22:11:15 UTC
It is currently not possible to build libclang as dynamic libraries ( http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-November/040088.html ). What do I need to do to submit a request for an exception?
Thanks,
Dave

Comment 9 Dave Johansen 2014-12-12 02:34:56 UTC
Submitted the request with FESCo ( https://fedorahosted.org/fesco/ticket/1370 ) but was then directed to FPC ( https://fedorahosted.org/fpc/ticket/477 ).

Comment 10 Rex Dieter 2014-12-13 16:45:55 UTC
I dont think you need permission here.  It is only required if you *choose* to statically link when dynamic linking is possible... which isn't the case here, right?

Comment 11 Dave Johansen 2014-12-18 15:06:52 UTC
The static linking was approved by FESCo ( https://fedorahosted.org/fesco/ticket/1370#comment:6 ).

Comment 12 Dave Johansen 2014-12-18 15:09:11 UTC
New Package SCM Request
=======================
Package Name: iwyu
Short Description: A tool for use with clang to analyze #includes in C and C++ source files
Owners: daveisfera
Branches: f21 el6 epel7
InitialCC:

Comment 13 Gwyn Ciesla 2014-12-18 15:20:03 UTC
WARNING: fedora-review flag not set

Comment 14 Christopher Meng 2014-12-20 03:08:41 UTC
I will post a full review soon, then you can push it.

Comment 15 Dave Johansen 2015-01-28 05:16:24 UTC
Since F21 updated to clang 3.5, here are the files for that:
https://daveisfera.fedorapeople.org/iwyu_3.5/iwyu.spec
https://daveisfera.fedorapeople.org/iwyu_3.5/iwyu-3.5-1.fc21.src.rpm

Comment 16 Dave Johansen 2015-01-31 03:11:31 UTC
I wanted to make sure everything still worked with clang 3.4.2 on EL 6/7 and I found an issue with the check section, so the updated .spec and source .rpm can be found at:
https://daveisfera.fedorapeople.org/iwyu_3.4.2/iwyu.spec
https://daveisfera.fedorapeople.org/iwyu_3.4.2/iwyu-3.4-1.el7.centos.src.rpm

Comment 17 Dave Johansen 2015-02-05 03:49:16 UTC
Any update on the review?

Comment 18 Christopher Meng 2015-02-09 06:02:06 UTC
Based on my previous review and static linking approval, this package should be approved as well. However I still give you some suggestions for this and hope you can fix these:

1.

-  cd build
-  make install DESTDIR=%{buildroot}
-  cp ../fix_includes.py %{buildroot}%{_bindir}/fix_includes
+  %make_install DESTDIR=%{buildroot} -C build
+  install -pDm755 fix_includes.py %{buildroot}%{_bindir}/fix_includes

Use install -m will make sure its permission is correct.

2. Processing files: iwyu-3.5-1.fc22.i686
Provides: iwyu = 3.5-1.fc22 iwyu(x86-32) = 3.5-1.fc22
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
Requires: /usr/bin/python libc.so.6 libc.so.6(GLIBC_2.0) libc.so.6(GLIBC_2.1) libc.so.6(GLIBC_2.1.3) libc.so.6(GLIBC_2.11) libc.so.6(GLIBC_2.15) libc.so.6(GLIBC_2.2) libc.so.6(GLIBC_2.2.3) libc.so.6(GLIBC_2.3) libc.so.6(GLIBC_2.3.4) libc.so.6(GLIBC_2.4) libc.so.6(GLIBC_2.6) libc.so.6(GLIBC_2.7) libdl.so.2 libdl.so.2(GLIBC_2.0) libform.so.5 libgcc_s.so.1 libgcc_s.so.1(GCC_3.0) libgcc_s.so.1(GCC_3.4) libgcc_s.so.1(GLIBC_2.0) libm.so.6 libm.so.6(GLIBC_2.0) libncurses.so.5 libpthread.so.0 libpthread.so.0(GLIBC_2.0) libpthread.so.0(GLIBC_2.1) libpthread.so.0(GLIBC_2.2) libstdc++.so.6 libstdc++.so.6(CXXABI_1.3) libstdc++.so.6(CXXABI_1.3.8) libstdc++.so.6(GLIBCXX_3.4) libstdc++.so.6(GLIBCXX_3.4.11) libstdc++.so.6(GLIBCXX_3.4.15) libstdc++.so.6(GLIBCXX_3.4.20) libstdc++.so.6(GLIBCXX_3.4.9) libtinfo.so.5 libz.so.1 libz.so.1(ZLIB_1.2.0) rtld(GNU_HASH)

Thus insert one line based on opinion 1:

   %make_install DESTDIR=%{buildroot} -C build
+  sed -i '1{\@^#!/usr/bin/python@s/^.*$/%{__python2}/}' fix_includes.py

You may need BR: python2-devel because of that python2 macro, if you want it easier, just use /usr/bin/python2, but I don't think BR python2-devel is bad, though. This script doesn't support Python 3 certainly.

3. Drop Group tag.

4. ./run_iwyu_tests.py
./fix_includes_test.py

You'd better use %{__python2} instead of directly execute it since I don't know the affection from Python 3 as default change in f22.

5. Consider virtual provides of that obtuse name:

Provides:  include-what-you-use = %{version}-%{release}
Provides:  include-what-you-use%{?_isa} = %{version}-%{release}

6. manpage can be ignored since help has been written into the python executables.

Comment 19 Pierre-YvesChibon 2015-02-09 07:43:15 UTC
the use of the %{_ptyhon2} macro is strongly encouraged if not mandatory in the guidelines now (especially with the python3 as default around the corner).

For the manpage, if there is one I am in favor of including it, there is no point in excluding some documentation if upstream provides it.

Comment 20 Dave Johansen 2015-02-10 04:39:05 UTC
In reply to comment #18:

> 1. Install cleanup.

Fixed.

> 2a. Specify Python 2 in script

The sed gave the following error:
sed: -e expression #1, char 32: unknown option to `s'

So I used this instead:
sed -i '1 s|^#!/usr/bin/python\b|#!%{__python2}|' fix_includes.py

> 2b. BR python2-devel.

Fixed.

> 3. Drop Group tag.

Fixed.

> 4. use %{__python2} instead of directly executing.

Fixed

> 5. Consider virtual provides of that obtuse name:

Fixed

> 6. manpage can be ignored since help has been written into the python executables.

Agreed, because there's no manpage in the upstream source.


The updated .spec and source .rpm can be found at the URLs listed in comment 15 with the fixes above.


Additional Question:
Currently, the version is set to 3.5 because that's what the tarball is called, but when you run "iwyu --version" it says 0.3 and I believe that's probably the more correct value. But when I change it, %setup chokes on the filename of the tar. Is there a way that I can get %setup to untar the file properly when I set the Version tag to 0.3?

Comment 21 Christopher Meng 2015-02-10 05:20:52 UTC
(In reply to Dave Johansen from comment #20)
> Additional Question:
> Currently, the version is set to 3.5 because that's what the tarball is
> called, but when you run "iwyu --version" it says 0.3 and I believe that's
> probably the more correct value. But when I change it, %setup chokes on the
> filename of the tar. Is there a way that I can get %setup to untar the file
> properly when I set the Version tag to 0.3?

"%setup -q" expanded as "%setup -q -n %{name}-%{version}", if you want to specify the version in the top-level dir of the tarball, just change it to

%global real_version 0.3
%setup -q -n %{name}-%{real_version}

I checked the upstream yesterday, while they said the version is 0.3, their tarball still contains 3.5. It's a bit confusing, but they might use 3.5 to ease the pain of choosing the right version to use with different LLVM.

I think you should consult with upstream first. We should avoid using epoch tag in the SPEC if it's packaged with wrong version.

Comment 22 Pierre-YvesChibon 2015-02-10 11:20:02 UTC
> sed -i '1 s|^#!/usr/bin/python\b|#!%{__python2}|' fix_includes.py

Did you check that the macro is correctly expanded by sed?

(ie: that the file has #!/usr/bin/python2 and no directly #!%{__python2})

Comment 23 Dave Johansen 2015-02-12 03:34:57 UTC
> I think you should consult with upstream first. We should avoid using epoch tag in
> the SPEC if it's packaged with wrong version.

Upstream said that the version is 0.3 ( see https://groups.google.com/forum/#!topic/include-what-you-use/dpVkAtWDyPM ) and that's what "iywu --version" outputs, so I'll run with that.

> Did you check that the macro is correctly expanded by sed?

Yes, the macro is correctly expanded.

I believe that's all of the issues that have been brought up so far. The updated .spec and source .rpm can be found at:
https://daveisfera.fedorapeople.org/iwyu_0.3/iwyu.spec
https://daveisfera.fedorapeople.org/iwyu_0.3/iwyu-0.3-1.fc21.src.rpm

Comment 24 Michael Schwendt 2015-02-12 12:59:26 UTC
FWIW, note that sed substitutions would fail without causing an error.

I would not worry much about inserting an unexpanded '#!%{__python2})' into the script, because that would break the shebang and result in an unresolvable auto-dependency in the package. It would only be bad, if nobody would notice that in the built packages. And if %__python2 were undefined, the %check section would fail as it also uses %__python.

However:

Generally, when using sed substitutions, it is a good recommendation to add guards to the spec file, which protect against sed failing to match. Either before or after running sed. While applying a patch file would error out, if it doesn't apply anymore, a failing sed match would cause the substitution not to be applied. In this particular case, if the script contained anything different from '#!/usr/bin/python\b' in line 1, e.g. /usr/bin/env something, it would not be substituted. A guard would run "grep" on the file and ensure that the sed match will be successful, e.g. an explicit "grep -m 1 '^#!/usr/bin/python\b' || exit -1" would add safety for this particular case.

Comment 25 Christopher Meng 2015-02-12 13:29:13 UTC
(In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #24)
> However:
> 
> Generally, when using sed substitutions, it is a good recommendation to add
> guards to the spec file, which protect against sed failing to match. Either
> before or after running sed. While applying a patch file would error out, if
> it doesn't apply anymore, a failing sed match would cause the substitution
> not to be applied. In this particular case, if the script contained anything
> different from '#!/usr/bin/python\b' in line 1, e.g. /usr/bin/env something,
> it would not be substituted. A guard would run "grep" on the file and ensure
> that the sed match will be successful, e.g. an explicit "grep -m 1
> '^#!/usr/bin/python\b' || exit -1" would add safety for this particular case.

I personally don't force people doing this since I believe people check the package in every update. 

But as you've said, it's worse to see the stillness when the hack doesn't work. So Dave here is your choice.

This package is approved now. Feel free to request SCM.

Comment 26 Michael Schwendt 2015-02-12 13:44:16 UTC
This is _not_ about forcing people.

It is about trying to give hints and general guidance beyond following existing guidelines. "People" are free to ignore such advice.

It is my experience that patch is safer than sed without guards, and it would not be the first time a packager is bitten by a failing sed match. A worst-case example is a lazy sed subst that fails to replace something that enters the built executables "silently" without making the build fail (e.g. paths, config changes). So, guards are a good thing. Add them where you see the benefit.

Comment 27 Dave Johansen 2015-02-13 03:42:40 UTC
In response to comment 24, I appreciate the feedback and I think automated checks are great, so I will add the "grep check" to make sure the sed doesn't silently fail.

Comment 28 Dave Johansen 2015-02-13 03:46:51 UTC
New Package SCM Request
=======================
Package Name: iwyu
Short Description: C/C++ source files #include analyzer based on clang
Upstream URL: https://code.google.com/p/include-what-you-use/
Owners: daveisfera
Branches: f20 f21 el6 epel7
InitialCC:

Comment 29 Gwyn Ciesla 2015-02-13 13:34:45 UTC
Git done (by process-git-requests).

Added f22 since we've branched.

Comment 30 Fedora Update System 2015-02-14 20:33:26 UTC
iwyu-0.2-1.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/iwyu-0.2-1.el7

Comment 31 Fedora Update System 2015-02-14 20:33:32 UTC
iwyu-0.3-1.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/iwyu-0.3-1.fc21

Comment 32 Fedora Update System 2015-02-14 20:33:38 UTC
iwyu-0.2-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/iwyu-0.2-1.el6

Comment 33 Fedora Update System 2015-02-14 20:33:44 UTC
iwyu-0.2-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/iwyu-0.2-1.fc20

Comment 34 Fedora Update System 2015-02-15 13:56:46 UTC
iwyu-0.2-1.fc20 has been pushed to the Fedora 20 testing repository.

Comment 35 Jakub Čajka 2015-02-19 11:01:22 UTC
Hello, package seems to fail to pass multiple tests on all non Intel architectures(for example due to use of x86 in-line assembly in one test).

s390(x):
http://s390.koji.fedoraproject.org/koji/buildinfo?buildID=301751
ppc64(le):
http://ppc.koji.fedoraproject.org/koji/buildinfo?buildID=294450
aarch64:
http://arm.koji.fedoraproject.org/koji/buildinfo?buildID=260770

It seems that upstream don't support any other platform than Intel. It would be best to make this package "ExclusiveArch: %{ix86} x86_64".

Comment 36 Dave Johansen 2015-02-26 03:38:42 UTC
Ok, I made it ExclusiveArch. Thanks for the feedback.

Comment 37 Fedora Update System 2015-02-27 09:23:16 UTC
iwyu-0.2-1.fc20 has been pushed to the Fedora 20 stable repository.

Comment 38 Fedora Update System 2015-02-27 09:24:34 UTC
iwyu-0.3-1.fc21 has been pushed to the Fedora 21 stable repository.

Comment 39 Fedora Update System 2015-03-08 22:41:19 UTC
iwyu-0.2-1.el6 has been pushed to the Fedora EPEL 6 stable repository.

Comment 40 Fedora Update System 2015-03-08 22:44:47 UTC
iwyu-0.2-1.el7 has been pushed to the Fedora EPEL 7 stable repository.


Note You need to log in before you can comment on or make changes to this bug.