Spec URL: http://salimma.fedorapeople.org/for_review/build_tools/waf.spec SRPM URL: http://salimma.fedorapeople.org/for_review/build_tools/waf-1.3.1-1.fc8.src.rpm Description: Waf is a Python-based framework for configuring, compiling and installing applications. It is a replacement for other tools such as Autotools, Scons, CMake or Ant.
rpmlint reported waf.src:34: E: hardcoded-library-path in %{_prefix}/lib/waf-%{version}-*/wafadmin/ waf.src:36: E: hardcoded-library-path in %{_prefix}/lib/waf-%{version}-*/wafadmin/pproc.py waf.src:47: E: hardcoded-library-path in %{_prefix}/lib/waf-%{version}-* waf.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 11) waf.noarch: E: only-non-binary-in-usr-lib
library paths: it's noarch, so /usr/lib should be fine spaces-and-tabs: woops, I mixed-and-matched Emacs and Vim again Let me know what you think about the first point (if you see the Installation page on the website, the /usr/lib/* files are not even strictly needed -- they are self-contained in the waf binary and will be unpacked to a temp directory if waf is not installed on the system)
Michel, Sorry that without commenting I left this review. The reason I left review is that tibbs already taken this for formal official review so let me work on some other fedora reviews :)
Parag, Thanks for clarifying. I'm not sure tibbs actually took over the review -- probably a misunderstanding. He probably set the review flag to '?' because you forgot to do that when assigning the bug. (Just re-checked and his name is not Cc:ed) Coming back?
Hmm? I didn't take this review; I went through and set the fedora-review flags on a bunch of review tickets which were assigned but didn't have the flag set.
By coincidence, I also made a waf package some days ago, see: Spec URL: http://thm.1erlei.de/waf.spec SRPM URL: http://thm.1erlei.de/waf-1.3.1-1.fc8.src.rpm Main difference is that I moved the wafadmin dir to /usr/share/waf-*, in order to keep rpmlint quiet. As far as I see, that's the way the yum package does it, for example. Maybe you want to merge that in? There are some other, minor differences. An additional remark wrt licenses: The complete package claims to be BSD(3c), but the wafadmin/pproc.py file seems to be similar to some earlier version of the subprocess module (by Peter Astrand, now part of Python 2.5), so would presumably be under the PSFv2 ?
Thomas, Thanks -- will do that. Do you want to be co-maintainer once the package lands in the repositories? I'll contact Peter Astrand and upstream, clarifying the license for pproc.py.
Thanks tibbs and michel for clarifying this. Will do review here.
(In reply to comment #7) > Thanks -- will do that. Do you want to be co-maintainer once the package > lands in the repositories? Yeah, why not. > I'll contact Peter Astrand and upstream, clarifying the license > for pproc.py. Maybe that's not a big issue at all, just wanted to bring that to attention.
Spec URL: http://salimma.fedorapeople.org/for_review/build_tools/waf.spec SRPM URL: http://salimma.fedorapeople.org/for_review/build_tools/waf-1.3.1-2.fc8.src.rpm Merged in changes from Thomas' package. The files are now moved to %{_datadir}, so Parag's /usr/lib concern is resolved. Licensing changed to BSD and Python (unless I hear otherwise from the author of pproc.py)
koji build failed http://koji.fedoraproject.org/koji/getfile?taskID=377239&name=build.log
Looks like some weird Koji failure. The package buildrequires python! I'll wait until tomorrow and check again, will update when I have a working Koji build.
The problem is the SRPM build step *before* the actual build step. There, the environment seems to be rather lean, and doesn't have python. So, the Requires: tag is evaluated to "python-abi =", and rpm doesn't like there being an equals sign without a version number following. So what about changing the Requires: tag to this: Requires: python-abi %(%{__python} -c "import sys ; print \"=\",sys.version[:3]")'
You know that the python(abi) thing is added automatically by rpmbuild and should not generally be manually included in the spec, right?
(In reply to comment #14) > You know that the python(abi) thing is added automatically by rpmbuild and > should not generally be manually included in the spec, right? Not in this case afaik, because nothing is installed to %python_sitelib.
Yup, as commented in the spec, rpmbuild appears to only pick up python-abi dependencies if %python_sitelib is populated (we should probably file a bug) http://koji.fedoraproject.org/koji/taskinfo?taskID=378548 Thomas, could you look at the GNOME demo? It fails on my machine: $ waf build [ 1/13] * glib_genmarshal : src/marshal.list -> build/default/src/marshal.h /bin/sh: ../src/marshal.list: Permission denied Compilation failed
Spec URL: http://salimma.fedorapeople.org/for_review/build_tools/waf.spec SRPM URL: http://salimma.fedorapeople.org/for_review/build_tools/waf-1.3.1-3.fc9.src.rpm
(In reply to comment #16) > Thomas, could you look at the GNOME demo? It fails on my machine: > > $ waf build > [ 1/13] * glib_genmarshal : src/marshal.list -> build/default/src/marshal.h > /bin/sh: ../src/marshal.list: Permission denied > Compilation failed That is a a waf issue, fixed on trunk. It seems that conf.check_tool('gnome') is not working, and thus the environment var 'GGM' (for 'glib_genmarshal') is not set, so it tries to execute the input file, which obviously fails. (In reply to comment #17) Not sure whether you really should 'rm demos/gnome/src/hello.h' just to keep rpmlint quiet. After all, it's a demo, and some other source files might try to include hello.h.
Hm, might put that back. On the other hand, it's currently not being referred to by any other files, so it's probably safe for now? Probably would not affect the review, but Thomas, if you could pull the trunk file that fixes check_tool, we should probably ship with that patched in?
(In reply to comment #19) > Hm, might put that back. On the other hand, it's currently not being > referred to by any other files, so it's probably safe for now? Yeah, but it's merely a matter of principle. Of course one has to look at rpmlint's complaints, but an empty file in the demo suite doesn't hurt. > Probably would not affect the review, but Thomas, if you could pull the > trunk file that fixes check_tool, we should probably ship with that patched > in? The attached patch (= trunk changeset of r2219) fixes the gnome demo. You could added that to the source rpm. However, I don't think we want to add more patches for the demo suite. Better let's bug upstream to fix that in the next release. The testsuite ('waf check') also fails for 1.3.1 (but maybe I didn't run it properly).
Created attachment 293390 [details] cherry-pick changeset 2219 from trunk, to fix the gnome demo
Spec URL: http://salimma.fedorapeople.org/for_review/build_tools/waf.spec SRPM URL: http://salimma.fedorapeople.org/for_review/build_tools/waf-1.3.1-4.fc9.src.rpm Applied fix for check_tools('gnome'), nothing else changed. Thanks, Thomas!
why directory name contains bug string attached to packages's nvr? /usr/share/waf-1.3.1-f157e378df1863aafcd40df84118156b
It's a hash of waf itself. If not installed system-wide, when you run waf it unpacks those files to a directory under the home directory of a similar name: ~/.waf-%{version}-%{hash}. It allows multiple copies of waf to not conflict with each other. Parag, Thomas, thoughts? If it's better to change it then I can do that.
We could of course patch waf to use /usr/share/waf, but to me this merely looks like a cosmetic issue, and I'm not sure if there is a real argument for doing so. The FHS only recommends that packages should use 'a subdirectory' under /usr/share, but doesn't state what the name of that subdirectory should be.
Spec URL: http://thm.1erlei.de/waf.spec SRPM URL: http://thm.1erlei.de/waf-1.3.2-5.fc8.src.rpm Changelog: - Update to 1.3.2. - Remove version and revision information from path to waf cache.
Looks good. Parag, review? (Thomas and I will be co-maintaining, so you can review his modifications)
Review: + package builds in mock (rawhide i386). koji build => http://koji.fedoraproject.org/koji/taskinfo?taskID=413925 + rpmlint is silent for SRPM and for RPM. + source files match upstream. 9caca69cb435911c9ed6ff0519ce19ae waf-1.3.2.tar.bz2 + package meets naming and packaging guidelines. + specfile is properly named, is cleanly written + Spec file is written in American English. + Spec file is legible. + dist tag is present. + build root is correct. + license is open source-compatible. + License text is included in package. + %doc files present. + BuildRequires are proper. + Compiler flags are honored correctly. + defattr usage is correct. + %clean is present. + package installed properly. + Macro use appears rather consistent. + Package contains code. + no static libraries. + no .pc file present. + no -devel subpackage exists. + no .la files. + no translations are available. + Does owns the directories it creates. + no duplicates in %files. + file permissions are appropriate. + no scriptlets are used. + Not a GUI app. APPROVED.
Thanks! Thomas, what's your Fedora username? You're not showing up on the Wiki.
(In reply to comment #29) > Thomas, what's your Fedora username? My username is 'thm'.
New Package CVS Request ======================= Package Name: waf Short Description: A Python-based build system Owners: salimma, thm Branches: F-7 F-8 EL-5 InitialCC: Cvsextras Commits: yes
cvs done.
waf-1.3.2-5.fc8 has been submitted as an update for Fedora 8
waf-1.3.2-5.fc7 has been submitted as an update for Fedora 7
waf-1.3.2-5.fc7 has been pushed to the Fedora 7 stable repository. If problems still persist, please make note of it in this bug report.
waf-1.3.2-5.fc8 has been pushed to the Fedora 8 stable repository. If problems still persist, please make note of it in this bug report.
Package Change Request ====================== Package Name: waf New Branches: epel7 Owners: thm salimma
Git done (by process-git-requests).