Hide Forgot
SPEC: http://pbrobinson.fedorapeople.org/network-manager-netbook-1.1-1.fc11.src.rpm SRPM: http://pbrobinson.fedorapeople.org/network-manager-netbook.spec network-manager-netbook is a connection management panel for Moblin
aha. So you got finally good connection manager. I am actually looking for information like if I want to review this, then do I need to test it on moblin? Do I need to actually build and run moblin packages on actual moblin system?
(In reply to comment #1) > aha. So you got finally good connection manager. I am actually looking for > information like if I want to review this, then do I need to test it on moblin? > Do I need to actually build and run moblin packages on actual moblin system? It won't actually build on rawhide at the moment as it depends on other moblin packages. I'm not sure what your asking here though.
I saw that there are custom builds from fedora packages for moblin. So, It looks I can't directly build and use moblin packages on Fedora releases. Is there any page on Fedora wiki that describes how reviewers should review moblin packages? But from your reply it seems I need actual moblin system to test/review moblin packages.
(In reply to comment #3) > I saw that there are custom builds from fedora packages for moblin. So, It > looks I can't directly build and use moblin packages on Fedora releases. Is > there any page on Fedora wiki that describes how reviewers should review moblin > packages? > > But from your reply it seems I need actual moblin system to test/review moblin > packages. No. I'm slowly creating the moblin packages for Fedora. They will only work on rawhide. So you need to wait for the dependencies to hit rawhide before you can test them. The package review is to ensure that the packaging meets the Fedora requirements. Hopefully later in the F12 rawhide release cycle once I have everything built in rawhide there will be test images to test the entire stack. Its too early for that though.
New upstream release SPEC: http://pbrobinson.fedorapeople.org/network-manager-netbook-1.2-1.fc11.src.rpm
I have a question regarding the sources for this package. The comment at the top of the spec file indicates that the sources come from git://git.gnome.org/mutter, but the URL: filed shows http://projects.gnome.org/NetworkManager. The Version: is 1.2, but the latest version of NetworkManager (at the provided URL) is 0.7. Will you please provide some clarification as the to the sources and versions needed for this package?
The network-manager-netbook component uses a different versioning sheme to the main NetworkManager component. You can see the 1.2 version tag in the git repo here http://git.gnome.org/cgit/network-manager-netbook/
I've updated the spec file SPEC: As above SRPM: http://pbrobinson.fedorapeople.org/network-manager-netbook-1.2-2.fc11.src.rpm
This package does not yet build due to missing dependencies: -------- checking for NMN... configure: error: Package requirements (dbus-glib-1 >= 0.75 gtk+-2.0 gconf-2.0 gnome-keyring-1 libnm-util libnm_glib mobile-broadband-provider-info) were not met: No package 'mobile-broadband-provider-info' found -------- Perhaps mobile-broadband-provider-info should be added to the "BuildRequires" list. In the meantime, I did a preliminary review of the spec file and found a few issues/questions: 1) * MUST: The License field in the package spec file must match the actual license. License: GPLv2+ but the COPYING file in the latest source RPM explicitly states "Version 3": GNU GENERAL PUBLIC LICENSE Version 3, 29 June 2007 2) * 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 package, please see the Source URL Guidelines for how to deal with this. I think the comment and URL field in the spec should be: # git clone git://git.gnome.org/network-manager-netbook URL: http://git.gnome.org/cgit/network-manager-netbook/ 3) * MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. There are library files with a suffix in the files list, but no -devel package. Should one be added? 4) * MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. I believe this needs to be added to the spec file.
> Perhaps mobile-broadband-provider-info should be added to the "BuildRequires" > list. It does include the package as BuildRequires. The problem is that the build of that package removes the pkgconfig during the build process and that is what its looking for. There is a bug filed which blocks this package in bugzilla. > In the meantime, I did a preliminary review of the spec file and found a few > issues/questions: > > > 1) * MUST: The License field in the package spec file must match the actual > license. > > License: GPLv2+ > > but the COPYING file in the latest source RPM explicitly states "Version 3": > > GNU GENERAL PUBLIC LICENSE > Version 3, 29 June 2007 Actually no its not. The COPYING file in the source package is actually a symbolic link to /usr/share/automake-1.10/COPYING so what your seeing is likely that file in your local file system. I've sent an email to the mailing list for a license clarification. See here http://mail.gnome.org/archives/networkmanager-list/2009-August/msg00084.html > 2) * 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 package, please see the Source URL > Guidelines for how to deal with this. > > I think the comment and URL field in the spec should be: > > # git clone git://git.gnome.org/network-manager-netbook > > URL: http://git.gnome.org/cgit/network-manager-netbook/ Copy and paste error. Will be fixed in the next srpm. > 3) * MUST: If a package contains library files with a suffix (e.g. > libfoo.so.1.1), then library files that end in .so (without suffix) must go in > a -devel package. > > There are library files with a suffix in the files list, but no -devel package. > Should one be added? I'll review this and update as appropriate. > 4) * MUST: Packages containing GUI applications must include a %{name}.desktop > file, and that file must be properly installed with desktop-file-install in the > %install section. If you feel that your packaged GUI application does not need > a .desktop file, you must put a comment in the spec file with your explanation. > > I believe this needs to be added to the spec file. The gui is a applet so I don't think it needs one. I'll look further as there's possibly a configuration gui too.
SPEC: http://pbrobinson.fedorapeople.org/network-manager-netbook.spec SRPM: http://pbrobinson.fedorapeople.org/network-manager-netbook-1.2-3.fc11.src.rpm koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1597620 Build updated, test build complete in koji. Points addressed above: 1) no response yet, license currently "GPLv2+ and LGPLv2+" as that's the two licenses explicitly mentioned in the source. 2) fixed 3) no longer an issue. Not sure where I got the .so from. 4) gui application is an applet. desktop file is in autostart directory. desktop-file-utils borks because it has different values in it that are valid. See bug 514351 for another example of this issue. I've also done the following: - extended the description a little - cleaned up the BuildRequires somewhat - patched the desktop file so it will only start in the Moblin desktop environment and not gnome (will be submitting the patch upstream).
As far as I can tell, the only pending issues on this are: - The COPYING file in the source package is actually a symbolic link to /usr/share/automake-1.10/COPYING - the desktop file is not being properly installed (desktop-file-install) It also appears that the 'sed' command does not prevent configure from running in autogen.sh. Everything else looks good to me. Once these are resolved I can approve this.
New package with fixes. SRPM: http://pbrobinson.fedorapeople.org/network-manager-netbook-1.2-4.fc11.src.rpm
Review of package network-manager-netbook-1.2-4 OK * MUST: rpmlint must be run on every package. The output should be posted in the review. $ rpmlint SPECS/network-manager-netbook.spec network-manager-netbook-1.2-4.fc12.* network-manager-netbook.i686: W: non-conffile-in-etc /etc/xdg/autostart/network-manager-netbook.desktop network-manager-netbook.ppc64: W: non-conffile-in-etc /etc/xdg/autostart/network-manager-netbook.desktop network-manager-netbook.ppc: W: non-conffile-in-etc /etc/xdg/autostart/network-manager-netbook.desktop network-manager-netbook.x86_64: W: non-conffile-in-etc /etc/xdg/autostart/network-manager-netbook.desktop 5 packages and 1 specfiles checked; 0 errors, 4 warnings. network-manager-netbook.desktop is not a "config" file - can ignore warning. OK * MUST: The package must be named according to the Package Naming Guidelines. OK * MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. OK * MUST: The package must meet the Packaging Guidelines. OK * MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines. OK * MUST: The License field in the package spec file must match the actual license. OK * MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc. OK * MUST: The spec file must be written in American English. OK * MUST: The spec file for the package MUST be legible. OK * 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 package, please see the Source URL Guidelines for how to deal with this. e45589dde04728029f0afa616af6fed3 SOURCES/network-manager-netbook-1.2.tar.bz2 OK * MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. N/A * MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. OK * MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense. N/A * MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden. N/A * MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. N/A * MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. OK * MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. OK * MUST: A Fedora package must not list a file more than once in the spec file's %files listings. OK * MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. OK * MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). OK * MUST: Each package must consistently use macros. OK * MUST: The package must contain code, or permissible content. N/A * MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). OK * MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. N/A * MUST: Header files must be in a -devel package. N/A * MUST: Static libraries must be in a -static package. N/A * MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). N/A * MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. N/A * MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} OK * MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built. OK * MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. # Issues with this due to MOBLIN autostart value not recognized by Fedora's d-f-u network-manager-netbook.desktop fails validation only due to unregistered value "MOBLIN" $ desktop-file-validate network-manager-netbook.desktop network-manager-netbook.desktop: error: value "MOBLIN;" for key "OnlyShowIn" in group "Desktop Entry" contains an unregistered value "MOBLIN"; values extending the format should start with "X-" This has been submitted for registration, and is required for proper operation. OK * MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. OK * MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). OK * MUST: All filenames in rpm packages must be valid UTF-8. SHOULD Items: OK * SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. #Copy license files until upstream fixes them in git N/A * SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. OK * SHOULD: The reviewer should test that the package builds in mock. OK * SHOULD: The package should compile and build into binary rpms on all supported architectures. * SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example. OK * SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. N/A * SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. N/A * SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. OK * SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. It does appear that .configure is still being run twice, once in autogen.sh and once during the %build stage. autogen.sh does not appear to recognize the NOCONFIGURE=true added to the spec file. This is not a blocker, but something that should probably be addressed at some point. Approved.
Built and on its way to rawhide. Thanks!
(In reply to comment #15) > Built and on its way to rawhide. Thanks! Not built. wrong bug.
Thanks for the review! New Package CVS Request ======================= Package Name: network-manager-netbook Short Description: Moblin Netbook GUI for NetworkManger Owners: pbrobinson Branches: F-11 InitialCC:
cvs done.
In rawhide. Thanks for the review.