Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=634911 --- Comment #5 from Lubomir Rintel <lkundrak@xxxxx> 2010-10-27 14:36:55 EDT --- (In reply to comment #4) > - The %{_prefix}/lib should be replaced with %{_libdir} > > NOT OK Not really. This is /usr/share/lib even on 64-bit architectures, since it contains the architecture independent-code. This is consistent with Perl and possibly other scripting languages. Note that dependent packages that ship compiled *.node files should not go there, this is not the case for libxmljs for which another review request is filed. Node currently looks into ~/.node_libraries and /usr/lib/node; an arch-specific directory should be added there; I'll try to work that out with upstream. > - According to the guildeline[1] you can safely remove the BuildRoot from the > spec - if I'm not mistaken you're targeting only: >=F-14, >=el6. Not sure really. I run this on el5 primarily, but use a custom-made build. I would certainly prefer this from EPEL but don't know if it would be possible yet. Please let me keep it for now; I'll eventually bulk-remove if from all my packages once el6 is out and el5 will get reasonably obsolete. > - The package duplicates the waf[2] package which is already available in the > Fedora. > > NOT OK > - MUST: The License field in the package spec file must match the actual > license. > > NOT OK. IMHO the license of the package is MIT only (assuming that the > duplicated "waf" files will be removed). Fixed > - MUST: The spec file must be written in American English. > > - An American English is not my native language, but maybe it's a good idea to > change the "Evented" to e.g. "Event based". This is what upstream uses... Changed to event-driven. > - The description doesn't provide a useful information about the program. The > second sentence seems to contain irrelevant copy-paste content. > > NOT OK Uh, good catch. Fixed. > - MUST: The sources used to build the package must match the upstream source, > as provided in the spec URL. Reviewers should use md5sum for this task. If no > upstream URL can be specified for this pa... > > Please consider to update to the latest available version[3]. > > NOT OK Done. > > > Additional things worth considering: > > - The provided /usr/bin/node will generate a clash with the /usr/sbin/node[4] > especially when the user will have in the PATH both /usr/bin/ and /usr/sbin/ > and node[4] package installed. Upstream probably could not have chosen a more generic name. I'll leave it as it is now and will attempt to settle this with upstream cooperation. > - I'm not sure what is the purpose of the node-waf or node-repl? node-waf is useless when waf is packaged and is now gone. > - There is the following shebang in the /usr/bin/node-repl: > > #!/usr/bin/env node > > which can be resolved to the /usr/sbin/node from the node[4] package. Fixed. May change if upstream decides to rename the binary. > - The -devel package contains the node.h file which includes the following > additional headers: > > #include <ev.h> > #include <eio.h> > #include <v8.h> > > but there is no explicit "Requires" for: v8-devel, libev-devel, libeio-devel. > It might be difficult for user to guess from which packages the aforementioned > headers comes from. Fixed. SPEC: http://v3.sk/~lkundrak/SPECS/nodejs.spec SRPM: http://v3.sk/~lkundrak/SRPMS/nodejs-0.2.3-1.fc13.src.rpm -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review