Bug 429445 - Review Request: waf - A Python-based build system
Review Request: waf - A Python-based build system
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-20 01:15 EST by Michel Alexandre Salim
Modified: 2014-12-02 15:47 EST (History)
4 users (show)

See Also:
Fixed In Version: 1.3.2-5.fc7
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-02-20 21:54:08 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
panemade: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
cherry-pick changeset 2219 from trunk, to fix the gnome demo (306 bytes, patch)
2008-01-30 06:04 EST, Thomas Moschny
no flags Details | Diff

  None (edit)
Description Michel Alexandre Salim 2008-01-20 01:15:16 EST
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.
Comment 1 Parag AN(पराग) 2008-01-20 06:09:05 EST
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
Comment 2 Michel Alexandre Salim 2008-01-20 10:34:16 EST
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)
Comment 3 Parag AN(पराग) 2008-01-20 21:04:58 EST
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 :)
Comment 4 Michel Alexandre Salim 2008-01-20 21:24:36 EST
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?
Comment 5 Jason Tibbitts 2008-01-20 21:58:23 EST
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.
Comment 6 Thomas Moschny 2008-01-22 14:53:16 EST
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 ?
Comment 7 Michel Alexandre Salim 2008-01-22 15:25:49 EST
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.
Comment 8 Parag AN(पराग) 2008-01-23 02:05:32 EST
Thanks tibbs and michel for clarifying this. Will do review here.
Comment 9 Thomas Moschny 2008-01-23 13:54:31 EST
(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.
Comment 10 Michel Alexandre Salim 2008-01-25 17:57:49 EST
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)

Comment 11 Parag AN(पराग) 2008-01-27 23:12:35 EST
koji build failed
http://koji.fedoraproject.org/koji/getfile?taskID=377239&name=build.log
Comment 12 Michel Alexandre Salim 2008-01-27 23:46:05 EST
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.
Comment 13 Thomas Moschny 2008-01-28 04:47:21 EST
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]")'
Comment 14 Jason Tibbitts 2008-01-28 11:40:37 EST
You know that the python(abi) thing is added automatically by rpmbuild and
should not generally be manually included in the spec, right?
Comment 15 Thomas Moschny 2008-01-28 11:50:15 EST
(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.
Comment 16 Michel Alexandre Salim 2008-01-28 13:03:15 EST
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
Comment 18 Thomas Moschny 2008-01-28 14:06:11 EST
(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.
Comment 19 Michel Alexandre Salim 2008-01-28 15:47:24 EST
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?
Comment 20 Thomas Moschny 2008-01-30 06:02:29 EST
(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).
Comment 21 Thomas Moschny 2008-01-30 06:04:06 EST
Created attachment 293390 [details]
cherry-pick changeset 2219 from trunk, to fix the gnome demo
Comment 22 Michel Alexandre Salim 2008-02-01 11:08:23 EST
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!
Comment 23 Parag AN(पराग) 2008-02-07 05:53:09 EST
why directory name contains bug string attached to packages's nvr?
/usr/share/waf-1.3.1-f157e378df1863aafcd40df84118156b
Comment 24 Michel Alexandre Salim 2008-02-07 11:04:23 EST
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.
Comment 25 Thomas Moschny 2008-02-08 03:50:35 EST
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.
Comment 26 Thomas Moschny 2008-02-10 15:00:53 EST
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.
Comment 27 Michel Alexandre Salim 2008-02-10 19:55:40 EST
Looks good. Parag, review? (Thomas and I will be co-maintaining, so you can
review his modifications)
Comment 28 Parag AN(पराग) 2008-02-11 01:09:39 EST
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.
Comment 29 Michel Alexandre Salim 2008-02-11 16:19:33 EST
Thanks! Thomas, what's your Fedora username? You're not showing up on the Wiki.
Comment 30 Thomas Moschny 2008-02-11 17:02:15 EST
(In reply to comment #29)
> Thomas, what's your Fedora username?

My username is 'thm'.

Comment 31 Michel Alexandre Salim 2008-02-11 17:55:53 EST
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
Comment 32 Kevin Fenzi 2008-02-11 21:43:59 EST
cvs done.
Comment 33 Fedora Update System 2008-02-12 04:27:57 EST
waf-1.3.2-5.fc8 has been submitted as an update for Fedora 8
Comment 34 Fedora Update System 2008-02-12 04:29:28 EST
waf-1.3.2-5.fc7 has been submitted as an update for Fedora 7
Comment 35 Fedora Update System 2008-02-20 21:54:06 EST
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.
Comment 36 Fedora Update System 2008-02-20 21:56:25 EST
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.
Comment 37 Thomas Moschny 2014-12-02 13:19:10 EST
Package Change Request
======================
Package Name: waf
New Branches: epel7
Owners: thm salimma
Comment 38 Jon Ciesla 2014-12-02 15:47:50 EST
Git done (by process-git-requests).

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