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
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}
> 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
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 :)
I made those changes and updated the .spec and source rpm at the links mentioned in comment 2.
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?).
I think you need to request a permission: http://fedoraproject.org/wiki/Packaging:Guidelines#Statically_Linking_Executables
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.
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
Submitted the request with FESCo ( https://fedorahosted.org/fesco/ticket/1370 ) but was then directed to FPC ( https://fedorahosted.org/fpc/ticket/477 ).
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?
The static linking was approved by FESCo ( https://fedorahosted.org/fesco/ticket/1370#comment:6 ).
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:
WARNING: fedora-review flag not set
I will post a full review soon, then you can push it.
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
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
Any update on the review?
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.
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.
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?
(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.
> 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})
> 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
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.
(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.
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.
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.
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:
Git done (by process-git-requests). Added f22 since we've branched.
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
iwyu-0.3-1.fc21 has been submitted as an update for Fedora 21. https://admin.fedoraproject.org/updates/iwyu-0.3-1.fc21
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
iwyu-0.2-1.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/iwyu-0.2-1.fc20
iwyu-0.2-1.fc20 has been pushed to the Fedora 20 testing repository.
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".
Ok, I made it ExclusiveArch. Thanks for the feedback.
iwyu-0.2-1.fc20 has been pushed to the Fedora 20 stable repository.
iwyu-0.3-1.fc21 has been pushed to the Fedora 21 stable repository.
iwyu-0.2-1.el6 has been pushed to the Fedora EPEL 6 stable repository.
iwyu-0.2-1.el7 has been pushed to the Fedora EPEL 7 stable repository.