Spec URL: http://mohammed.morsi.org/blog/files/anyterm.spec SRPM URL: http://mohammed.morsi.org/blog/files/anyterm-1.1.29-0.1.src_.rpm Description: Main project site: http://anyterm.org/ Anyterm is a web based terminal emulator that allows terminal (or any other non-graphical command) access to a javascript enabled web browser, such as Firefox. Anyterm (written in c++ and uses boost) provides a HTTP/1.1 complaint web server (may easily be proxied via apache) which can be configured to run an arbitrary command on the backend. The anyterm daemon redirects stdin / stdout to and from the program being executed and the client's web browser, which relies on javascript to do the same. To use anyterm, build the rpm and install it, configure the daemon via /etc/sysconfig/anyterm to run any command ('bash' by default) as any user and listen on any port, and start the service via service / init, eg 'service anyterm start'. You then can use Firefox, navigating to http://server:8080/, to access bash or whatever other command you specified.
1 - replace test "x$RPM_BUILD_ROOT" != "x" && rm -rf $RPM_BUILD_ROOT mkdir %{buildroot} with rm -rf $RPM_BUILD_ROOT 2 - look about source0 https://fedoraproject.org/wiki/Packaging/SourceURL 3 - missing dist-tag Release: 1%{?dist} 4 - you can make install section smaller, merging commands, there are no need of install -d if install -Dp create directory's
Thanks for the feedback. I incorporated all your suggestions as well as a few from the upstream anyterm community and uploaded new versions of the srpm and spec files (original url's still apply). Looking forward to any more feedback, moving along in the process. Thanks alot!
More stuff to check: * The License: field must be filled with the appropriate license Short License identifier(s) from https://fedoraproject.org/wiki/Licensing#Good_Licenses * They already provide a package suitable for Source0: http://anyterm.org/download/anyterm-1.1.29.tbz2 so I don't see the need to pull it from SVN * the stuff you added can go in Source1: Source2: etc * you can remove the gcc-c++ BR https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2 * chances are the boost requirement will be picked up automatically, we can check this out with a mock build * build with make %{?_smp_mflags}, if for some reason you can't use parallel build, please add a comment See: http://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make * remember to bump the release number at each iteration (and update changelog where appropriate)
users in fedora are not removed when package is removed, please look https://fedoraproject.org/wiki/Packaging:UsersAndGroups please use getent according guidelines. why you're not using the tarball from website ? http://anyterm.org/download/anyterm-1.1.29.tbz2
Uploaded new versions of the spec and srpm. Changes included: * license is now GPLv2+ * gcc-c++ removed * %{?_smp_mflags} added * correct user/group management, using getent and not deleting them on uninstall Changes ommitted: * boost still included, not sure whether you were referring to the boost req or boost-devel buildreq (or both) and if they actually will be pulled in (preliminary googling it reveals many specs w/ that req included, are they erroneous?) * source0 location, essentially I checked out the code from the anyterm svn repo trunk, added the spec, init and sysconfig scripts, made a few changes to the code base, and then generated / submitted the srpm and spec here. I'm not sure how exactly my changes, specifically the new files I added and the changes to the code will be available and make my way into the codebase when listing the hosted anyterm release tarball Appreciate the help
Can you include a koji build in dist-f11 ?
according with http://anyterm.org/1.1/install.html Anyterm uses some of the Boost C++ libraries, but it currently only needs their header files at compile time; it doesn't actually link with any of the libraries at run-time. you can remove the Requires: boost
(In reply to comment #5) > Changes ommitted: > * boost still included, not sure whether you were referring to the boost req or > boost-devel buildreq (or both) and if they actually will be pulled in I was referring to the Require: line. Usually, runtime dependencies are auto-detected during rpmbuild, but that is not bullet-proof. That's why I suggested we double check with a mock build. > * source0 location, essentially I checked out the code from the anyterm svn > repo trunk, added the spec, init and sysconfig scripts, made a few changes to > the code base, and then generated / submitted the srpm and spec here. I'm not > sure how exactly my changes, specifically the new files I added and the changes > to the code will be available and make my way into the codebase when listing > the hosted anyterm release tarball One review item is to check if the source tarball matches upstream sources. That's why you should, when possibile, use an unmodified upstream tarball; I usually download it with "spectool -g name.spec" and this also checks Source0: is correct Additional stuff you need to use for packaging should go either in additional "SourceX:" or "PatchX:" lines; are you saying you are not sure how to use these?
Thanks for submitting this! I packaged anyterm locally a while back, but I've been too lazy to submit it. Feel free to copy anything you like from it: http://ayo.sys.kth.se/kth/linux/5/extras/SRPMS/anyterm-1.1.29-0.kth.5.src.rpm Some notes about it: Installs the static content in /var/www for httpd to pick up. Listens on localhost, installs a suitable httpd conf file to proxy via SSL. Starts anytermd at port 81 where regular users can't bind(). Asks for a username and runs ssh@localhost.
Hey sorry for the delay, I got pulled onto something else last week and have been busy since. Some updates (new spec / srpm has been uploaded) - boost requirement has been removed - dist-f11 koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1276085 - been working w/ the anyterm upstream community to get some changes committed, including some necessary bugfixes, new features, and integration w/ httpd. After this no patches / additional changes should be needed for this - because of the previous I don't think anyterm 1.1.29 tarball from the site will cut it, so we'll need to either source this from the version control checkout tarball, or (less preferably) wait for the next release Thanks for the feedback
(In reply to comment #10) > Some updates (new spec / srpm has been uploaded) Ok, please remember to bump the release number when you upload a new spec/srpm so I can be sure I'm looking at an updated spec file. > - been working w/ the anyterm upstream community to get some changes committed, > including some necessary bugfixes, new features, and integration w/ httpd. > After this no patches / additional changes should be needed for this Great > > - because of the previous I don't think anyterm 1.1.29 tarball from the site > will cut it, so we'll need to either source this from the version control > checkout tarball, or (less preferably) wait for the next release Packaging a SVN checkout is OK; what is (still) not good is that I can't verify your tarball matches upstream. If you want to stick to a svn snapshot until the next release, please pick a suitable revision from upstream, then package it with "svn export -r" and be sure to update the package version field according to: https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages
Looks pretty good! My comments: * The typical use case for this is a multiuser machine where you'd usually not completely trust all users. Since Apache is configured to proxy to port 8080 then if anytermd is not running for some reason any user will be able to listen to that port and have other users' passwords sent there. So I still think using a port <1024 by default is the way to go. * In anyterm-cmd: read U ssh $U@localhost Here the user could enter any ssh client option into $U, and I'm pretty sure it's possible to do evil that way, for example by causing ~anytermd/.ssh/config and ~anytermd/.ssh/known_hosts to be replaced. I suggest something like this: while :; do echo -n "Username: " read U # Make sure it does not start with a "-" and only contains valid # username characters. if [[ "$U" =~ "^[A-Za-z0-9_]" && ! ( "$U" =~ "[^A-Za-z0-9_-]" ) ]]; then exec ssh "$U@localhost" fi echo "Bad username." done * I'd package the static content in /var/www/anyterm to make it easy for admins to customize it and cut down on proxy traffic.
Reworked the rpm based on feedback. Since I bumped the dist / changelog the SRPM can now be accessed at http://mohammed.morsi.org/blog/files/anyterm-1.1.29-2.fc10.src_.rpm (location of spec still the same) Changes include: - Use official anyterm-1.1.29 tbz2 that can be d/l from anyterm site. Additional sources are added for init, sysconfig, anyterm-cmd, httpd conf. Additional patch has been included to url-parameterize various variables currently statically set (patch submitted to anyterm upstream community as well) - port is set to 81, less than 1024 as recommended - anyterm-cmd changes incorporated, and addition to make it loop back on failed login or logout - static content copied over and served by apache Enjoy!
Argh, I'm sorry, I mislead you. The web content should be in /usr/share/anyterm rather than in /var/www/anyterm, as per the packaging guidelines. Btw, how come it defaults to ASCII? Double wide characters, Arabic script and Cyrillic only works to some degree, but UTF-8 seems to work fine for "extended latin" characters, so I see no reason not to enable it anyway. Now let's see if I can write an informal review. (I'm not sponsored.)
NOT A FORMAL REVIEW. I'll do this the verbose way. * MUST: rpmlint must be run on every package. The output should be posted in the review.[1] anyterm.src: W: summary-ended-with-dot Anyterm is a web-based terminal emulator. anyterm.src: W: name-repeated-in-summary Anyterm anyterm.x86_64: W: summary-ended-with-dot Anyterm is a web-based terminal emulator. anyterm.x86_64: W: name-repeated-in-summary Anyterm "A web-based terminal emulator" should be ok I guess. anyterm.x86_64: W: executable-stack /usr/sbin/anytermd Maybe 'export CFLAGS="$RPM_OPT_FLAGS"' or similar would fix this? anyterm.x86_64: W: spurious-executable-perm /usr/share/man/man1/anytermd.1.gz anyterm.x86_64: E: executable-marked-as-config-file /etc/sysconfig/anyterm anyterm.x86_64: E: executable-marked-as-config-file /etc/httpd/conf.d/anyterm.conf Copypaste errors in %install - easy fix. anyterm.x86_64: E: wrong-script-end-of-line-encoding /usr/share/man/man1/anytermd.1.gz Might need to run dos2unix on it? anyterm.x86_64: E: script-without-shebang /etc/sysconfig/anyterm anyterm.x86_64: E: script-without-shebang /etc/httpd/conf.d/anyterm.conf Not a problem. anyterm.x86_64: E: init-script-without-chkconfig-postin /etc/rc.d/init.d/anyterm anyterm.x86_64: E: init-script-without-chkconfig-preun /etc/rc.d/init.d/anyterm anyterm.x86_64: W: no-reload-entry /etc/rc.d/init.d/anyterm anyterm.x86_64: E: subsys-not-used /etc/rc.d/init.d/anyterm Merge in stuff from http://fedoraproject.org/wiki/Packaging/SysVInitScript to hopefully fix this. * 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. [2] . Ok. * MUST: The package must meet the Packaging Guidelines . Some notes: 1.6 Writing a package from scratch BuildRoot is wrong. Easy fix. Whitespace mismatch in tags and %description has a newline, as compared to the sample .spec. (Don't know if that matters.) 1.8.1 Architecture Build Failures Doesn't build on rawhide i386. 1.20 Compiler flags - Need fixing. 1.42 Web Applications - Change /var/www to /usr/share. 1.47 All patches should have an upstream bug link or comment 1.47.1 If upstream doesn't have a bug tracker Add a comment: # http://anyterm.org/1.1/install.html#secid2252601 # http://anyterm.org/forums/viewtopic.php?id=577 Patch0: url-parameterize-vars.patch Also, the patch should really be accepted upstream first. (The proxy URL part is ok since it's from upstream docs.) I'm slightly worried about the security implications of it so I think it's fairly important even though it's just a small patch. * 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. [3] 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.[4] Add: %doc LICENSE * MUST: The spec file must be written in American English. [5] Ok, AFAICT. * MUST: The spec file for the package MUST be legible. [6] It's fine, but I think the definiton of "pbuild" can be removed and the use of "%{pbuild}/" removed since it will be equal to the current working directory anyway. * 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. Ok, matches. * MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [7] Not ok. (Rawhide i386 fails.) * 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. [8] Ok, build problem is probably not a arch-related. * 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. Ok, I believe. * MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.[9] Ok, I guess. No translation. * 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. [10] Ok. No shared libraries. * 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. [11] 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. [12] Not ok. It should own /usr/share/anyterm. * MUST: A Fedora package must not list a file more than once in the spec file's %files listings. [13] 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. [14] Not ok. See rpmlint. * MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [15] Ok. * MUST: Each package must consistently use macros. [16] Ok, unless __install should not be used. * MUST: The package must contain code, or permissable content. [17] Ok. * 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). [18] 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. [18] Ok. * MUST: Header files must be in a -devel package. [19] * MUST: Static libraries must be in a -static package. [20] * MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). [21] * 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. [19] Ok. * MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} [22] Ok, no devel package. * MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[20] 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. [23] 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. [24] Ok. * MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [25] Ok. * MUST: All filenames in rpm packages must be valid UTF-8. [26] 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. [27] Ok. * SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. [28] Ok. * SHOULD: The reviewer should test that the package builds in mock. [29] Not ok. * SHOULD: The package should compile and build into binary rpms on all supported architectures. [30] * SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example. Not tested. * SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. [31] Ok. * SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [22] Ok. * 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. [21] 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. [32] Ok.
Updated based on feedback, current srpm can now be accessed here http://mohammed.morsi.org/blog/files/anyterm-1.1.29-3.fc10.src_.rpm changed /var/www/anyterm to /usr/share/anyterm, default to UTF8, took care of rpmlint output, hopefully catching everything.
Apologize for the long delay, I have been very busy with other tasks. I made changed to address all of the remaining issues raised above. The new spec / srpm can be found here: http://mohammed.morsi.org/blog/files/anyterm.spec http://mohammed.morsi.org/blog/files/anyterm-1.1.29-4.fc10.src_.rpm Please note the following: - as far as the executable stack in anytermd issue 'export CFLAGS="$RPM_OPT_FLAGS"' did not fix the issue adding "execstack -c anytermd" after the make in the build section fixes this but I'm not sure if thats the ideal solution - removed the small patch I had added to parameterize the url vars per your concerns that it isn't upstream - Doesn't build on rawhide i386 issue: I was unable to get a rawhide system working (installed F11, enabled rawhide repo, updated, restarted, boot sequence crashed w/ a stack trace) I could not reproduce this, on a regular non-rawhide F11 i386 vm Here is the output: http://ovirt.pastebin.com/m570a2b2 Is this a major issue, it is rawhide after all - Latest Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1460119 - Current rpmlint output: $ rpmlint RPMS/x86_64/anyterm-1.1.29-4.fc10.x86_64.rpm anyterm.x86_64: W: unstripped-binary-or-object /usr/sbin/anytermd 1 packages and 0 specfiles checked; 0 errors, 1 warnings. - rpmlint reports "anyterm.x86_64: W: unstripped-binary-or-object /usr/sbin/anytermd", couldn't find much info on this and adding "-i" to rpmlint didn't help - had to introduce DEFAULT_LOCKFILE to init script as rpmlint reported (without it) "anyterm.x86_64: E: incoherent-subsys /etc/rc.d/init.d/anyterm anyterm}" (note trailing brace) - Clarification needed for your comments: "BuildRoot is wrong. Easy fix." "Whitespace mismatch in tags" (are these still issues?)
Suggested Buildroot tags are listed in: https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag yours is a variant of the third one, it's fine according to the guidelines. The main problem I still see if that compilation dos not pick up our RPM_OPT_FLAGS. try setting both CFLAGS and CXXFLAGS with it, then it should work as expected and it will probably fix the debuginfo problem.
Thanks for the feedback, I tried changing the spec to incorporate cflags / cxx flags as you suggested like so: %build export CFLAGS="$RPM_OPT_FLAGS" export CXXFLAGS="$RPM_OPT_FLAGS" make %{?_smp_mflags} CFLAGS="$CFLAGS" CXXFLAGS="$CXXFLAGS" %{__gzip} anytermd.1 #execstack -c anytermd Unfortunately after building / running rpm lint on it both the unstripped-binary-or-object and executable stack (note I commented out execstack) issues persisted. Looking at the 1.1.29 makefile http://svn.anyterm.org/anyterm/tags/releases/1.1/1.1.29/common.mk the issue might be that the author explicitly sets CPP_FLAGS, overwriting it if previously set, and doesn't use CFLAGS or CXXFLAGS at all. Then again, trying the rpmbuild process w/ ${RPM_OPT_FLAGS} explicitly set and passed to each invocation of g++ doesn't seem to resolve either of the issues (but does add "-02 -g" to the g++ command) Still not sure what I need do to resolve this.
Sorry I spoke too soon. Adding the CFLAGS / CXXFLAGS as suggested above is successful in adding "-02 -g" to the g++ invocation, the same result as when I added ${RPM_OPT_FLAGS} directly to g++. The two rpmlint issues persist regardless though (will leave execstack in to take care the executable stack issue) but at least RPM_OPT_FLAGS is picked up. Here is the updated spec and srpm (version bumped as well): http://mohammed.morsi.org/blog/files/anyterm.spec http://mohammed.morsi.org/blog/files/anyterm-1.1.29-5.fc10.src_.rpm
I rebuilt it on Fedora 11 and rpmlint was silent for me.
Still not a formal review. == FAIL == * MUST: The package must meet the Packaging Guidelines . See below. * MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines . * MUST: The License field in the package spec file must match the actual license. [3] No license is provided for http://svn.anyterm.org/anyterm/trunk/src/sun_forkpty.h * 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.[4] %doc must be in the %files section. * 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. [12] Needs to req. httpd. * MUST: Each package must consistently use macros. [16] Mixed use of %{buildroot} and $RPM_BUILD_ROOT and mixed %{__foo} vs. foo. == Would be nice to fix == * SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. [28] * SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. [31] Shouldn't it use >/dev/null instead of &>/dev/null for useradd/groupadd? They're basically sane, though. * Additionally: anyterm-cmd should perhaps be moved to %{_libexecdir}/%{name}/anyterm-cmd == Not checked == * SHOULD: The package should compile and build into binary rpms on all supported architectures. [30] I haven't seen any failures though. == PASS == * MUST: rpmlint must be run on every package. The output should be posted in the review.[1] $ rpmlint -i SRPMS/anyterm-1.1.29-5.fc11.src.rpm RPMS/x86_64/anyterm-1.1.29-5.fc11.x86_64.rpm RPMS/x86_64/anyterm-debuginfo-1.1.29-5.fc11.x86_64.rpm 3 packages and 0 specfiles checked; 0 errors, 0 warnings. * MUST: The package must be named according to the Package Naming Guidelines . * MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. [2] . * MUST: The spec file must be written in American English. [5] * MUST: The spec file for the package MUST be legible. [6] * 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. * MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [7] * 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. [8] * 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. * MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.[9] * 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. [10] * 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. [11] * MUST: A Fedora package must not list a file more than once in the spec file's %files listings. [13] * 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. [14] * MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [15] * MUST: The package must contain code, or permissable content. [17] * 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). [18] * 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. [18] * MUST: Header files must be in a -devel package. [19] * MUST: Static libraries must be in a -static package. [20] * MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). [21] * 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. [19] * MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} [22] * MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[20] * 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. [23] * 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. [24] * MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [25] * MUST: All filenames in rpm packages must be valid UTF-8. [26] * 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. [27] * SHOULD: The reviewer should test that the package builds in mock. [29] * SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [22] * 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. [21] * 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. [32] * SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
(In reply to comment #22) > Still not a formal review. > > == FAIL == > > * MUST: The package must meet the Packaging Guidelines . > > See below. > > * MUST: The package must be licensed with a Fedora approved license and meet > the Licensing Guidelines . Is this a separate issue or does this relate to the next one? The current spec file includes "License: GPLv2+" which should satisfy this requirement. > * MUST: The License field in the package spec file must match the actual > license. [3] > > No license is provided for > http://svn.anyterm.org/anyterm/trunk/src/sun_forkpty.h Since I'm building from the latest release, 1.1.29, and that file doesn't exist in there (only in trunk) this is not an issue: http://svn.anyterm.org/anyterm/tags/releases/1.1/1.1.29/src/ (just out of curiosity if it was an issue, what would I need to do, eg would this something we couldn't proceed with until it was resolved upstream? once again, not an issue but I am interested) > > * 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.[4] > > %doc must be in the %files section. Done, moved %doc to %files > > * 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. [12] > > Needs to req. httpd. I'm wondering if we could make this optional. Yes we do install anyterm.conf to %{_sysconfdir}/httpd/conf.d/ but nothing in anyterm explicitly depends on httpd and it can be run 100% fine as is without it. I think it would be really worthwhile to make this optional so as not to impose the httpd requirement on sysadmins who want to run anyterm standalone (for example in a scenario where httpd / anytermd are running on seperate machines). Is there anyways to do this (what are your thoughts about a seperate anyterm-httpd package?) > > * MUST: Each package must consistently use macros. [16] > > Mixed use of %{buildroot} and $RPM_BUILD_ROOT and mixed %{__foo} vs. foo. > substituted $RPM_BUILD_ROOT w/ %{buildroot}, but not sure which 'foo' you are referring too, I tried to use all the predefined %{} macros that I could where appropriate > > > == Would be nice to fix == > > * SHOULD: The description and summary sections in the package spec file should > contain translations for supported Non-English languages, if available. [28] I'm not sure how to approach doing these translations / which are needed > * SHOULD: If scriptlets are used, those scriptlets must be sane. This is > vague, and left up to the reviewers judgement to determine sanity. [31] > > Shouldn't it use >/dev/null instead of &>/dev/null for useradd/groupadd? > They're basically sane, though. Which way is standard? Googling for this, I find most specs redirect both stdout / stderr (or even just stderr) http://www.google.com/#hl=en&q=groupadd+%2Fdev%2Fnull+filetype%3Aspec+&aq=f&oq=groupadd+filetype%3Aspec&aqi=&aq=&oq=&aqi=&aq=f&oq=&aqi=&fp=Xmf0jJ9P_V0 > > * Additionally: > > anyterm-cmd should perhaps be moved to %{_libexecdir}/%{name}/anyterm-cmd Done, moved it from bindir to libexecdir New SPEC and SRPM (bumped release): http://mohammed.morsi.org/blog/files/anyterm.spec http://mohammed.morsi.org/blog/files/anyterm-1.1.29-6.fc10.src_.rpm
(In reply to comment #23) > (In reply to comment #22) > > * MUST: The package must be licensed with a Fedora approved license and meet > > the Licensing Guidelines . > > Is this a separate issue or does this relate to the next one? The current spec > file includes "License: GPLv2+" which should satisfy this requirement. Yeah, I think that bullet refers to the package as a whole. GPLv2+ is fine, course! Also, the build reqs. have GPL compatible licenses. > in there (only in trunk) this is not an issue: > > http://svn.anyterm.org/anyterm/tags/releases/1.1/1.1.29/src/ Eh, sorry, rather sloppy of me to not check that. I'm glad it's not really an issue, though. > (just out of curiosity if it was an issue, what would I need to do, eg would > this something we couldn't proceed with until it was resolved upstream? In the general case I think upstream needs to have its licensing in order for their code to go into Fedora. But in this case, looking further at the code, I see that worst case we'd just need a new tarball with the file removed, because it seems it's actually only used on Solaris. > nothing in anyterm explicitly depends on httpd > and it can be run 100% fine as is without it. > (what are your thoughts about a seperate anyterm-httpd package?) Hmm, you're right. A subpackage would be fine. Another option is to just put anyterm.conf in %doc since it asks to be edited anyway. > I tried to use all the predefined %{} macros that I could where > appropriate There's __make, __rm, __mkdir. > I'm not sure how to approach doing these translations / which are needed Me neither, but I just mentioned it for completeness. Not a blocker! > Which way is standard? Googling for this, I find most specs redirect both > stdout / stderr > (or even just stderr) I'd say only redirect stderr if there's a known, harmless error to hide. If it's normally silent on stderr then don't redirect it. The same goes for stdout, only redirect it if there's anything unsightly to hide. getent might send uninteresting output to stdout but is always silent on stderr even when the entry is missing. useradd/groupadd is always silent on both stdout and stderr. So only redirect stdout from getent and don't redirect anything from useradd/groupadd. http://fedoraproject.org/wiki/Packaging/UsersAndGroups does it that way.
(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > nothing in anyterm explicitly depends on httpd > > and it can be run 100% fine as is without it. > > > (what are your thoughts about a seperate anyterm-httpd package?) > > Hmm, you're right. A subpackage would be fine. Another option is to just put > anyterm.conf in %doc since it asks to be edited anyway. > I created a anyterm-httpd subpackage, that depends on anyterm and httpd, and just contains the anyterm / httpd proxy config. > > I tried to use all the predefined %{} macros that I could where > > appropriate > > There's __make, __rm, __mkdir. Gah, sorry I missed these, updated the spec to use the macro versions. > > Which way is standard? Googling for this, I find most specs redirect both > > stdout / stderr > > (or even just stderr) > > I'd say only redirect stderr if there's a known, harmless error to hide. If > it's normally silent on stderr then don't redirect it. The same goes for > stdout, only redirect it if there's anything unsightly to hide. > > getent might send uninteresting output to stdout but is always silent on stderr > even when the entry is missing. useradd/groupadd is always silent on both > stdout and stderr. So only redirect stdout from getent and don't redirect > anything from useradd/groupadd. > > http://fedoraproject.org/wiki/Packaging/UsersAndGroups does it that way. Done, useradd and groupadd no longer redirected stderr. Again thanks for the feedback, new versions uploaded: http://mohammed.morsi.org/blog/files/anyterm.spec http://mohammed.morsi.org/blog/files/anyterm-1.1.29-7.fc10.src_.rpm
(In reply to comment #25) > (In reply to comment #24) > > Hmm, you're right. A subpackage would be fine. Another option is to just put > > anyterm.conf in %doc since it asks to be edited anyway. > > I created a anyterm-httpd subpackage, that depends on anyterm and httpd, and > just contains the anyterm / httpd proxy config. Cool, though there's * SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. It's only a SHOULD. I don't know if it matters. > Done, useradd and groupadd no longer redirected stderr. Good enough, though the redirect of stdout for those seems superfluous. Also, some new rpmlints: anyterm.src:111: E: files-attr-not-set I think you need a defattr for each subpackage. anyterm.src:120: W: macro-in-%changelog doc Edit the changelog to say %%doc. anyterm-httpd.i586: W: summary-not-capitalized httpd proxy configuration for anyterm It seem wrong to capitalize a package name. anyterm-httpd.i586: W: no-documentation There's not much documentation to include. It's all on the web site.
> Cool, though there's > > * SHOULD: Usually, subpackages other than devel should require the base > package using a fully versioned dependency. > > It's only a SHOULD. I don't know if it matters. Changed the anyterm-httpd dependency to "Requires: %{name} = %{version}-%{release}" > > > Done, useradd and groupadd no longer redirected stderr. > > Good enough, though the redirect of stdout for those seems superfluous. > Ah srry, thought the example had that, removed. > > Also, some new rpmlints: > > anyterm.src:111: E: files-attr-not-set > I think you need a defattr for each subpackage. Done. > > anyterm.src:120: W: macro-in-%changelog doc > Edit the changelog to say %%doc. Done. > > anyterm-httpd.i586: W: summary-not-capitalized httpd proxy configuration for > anyterm > It seem wrong to capitalize a package name. Just had to capitalize the first word in the anyterm-httpd documentation > > anyterm-httpd.i586: W: no-documentation > There's not much documentation to include. It's all on the web site. Ya don't think any docs is appropriate here (the package summary / description pretty much says it all). I slightly reworded the description to be a little more clear that the package provides a httpd conf New versions uploaded: http://mohammed.morsi.org/blog/files/anyterm.spec http://mohammed.morsi.org/blog/files/anyterm-1.1.29-8.fc10.src_.rpm
So now that this is pretty much complete, could anyone sponsor this. Want to get it in before the Fedora 12 feature freeze deadline next week. The rpms and spec are all good to go and the functionality should be 100% working. Install anyterm to get the anyterm server, install anytetrm-httpd to get the anyterm proxy config for httpd.
Can you also make a scratch build in koji and post the URL here ? Is there any rpmlint output left ? If so, please post here
http://koji.fedoraproject.org/koji/taskinfo?taskID=1486707 rpmlint ~/rpmbuild/RPMS/x86_64/anyterm-1.1.29-8.fc10.x86_64.rpm anyterm.x86_64: W: unstripped-binary-or-object /usr/sbin/anytermd 1 packages and 0 specfiles checked; 0 errors, 1 warnings. (couldn't get this fixed, see above, adding -i to rpmlint doesn't help, but it doesn't seem to affect anything) rpmlint ~/rpmbuild/RPMS/x86_64/anyterm-httpd-1.1.29-8.fc10.x86_64.rpm anyterm-httpd.x86_64: W: no-documentation 1 packages and 0 specfiles checked; 0 errors, 1 warnings. (as discussed above, this package is self-documenting, no documentation needed / appropriate for this) rpmlint ~/rpmbuild/SRPMS/anyterm-1.1.29-8.fc10.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings.
Once again shouting out to look for a sponsor as there has been no activity on this review request for a few days and the F12 deadline is coming up in less than a week (also adding the FE-NEEDSPONSOR flag) I've received several requests from people interested in seeing this go in. Any help doing so would be greatly appreciated.
This looks good now, the only issue is that there is a type in the BuildRoot: it should use %{name}, not ${name}
Updated, changed "${name}" to "%{name}", new SRPM / SPEC uploaded here (though that was the only change): http://mohammed.morsi.org/blog/files/anyterm.spec http://mohammed.morsi.org/blog/files/anyterm-1.1.29-8.fc10.src_.rpm
OK - Package name OK - License info is accurate OK - License tag is correct and licenses are approved OK - License files are installed as %doc OK - Specfile name OK - Specfile is legible OK - No prebuilt binaries included OK - BuildRoot value (one of the recommended values) OK - PreReq not used OK - Source md5sum matches upstream OK - No hardcoded pathnames OK - Package owns all the files it installs OK - 'Requires' create needed unowned directories OK - Package builds successfully on i386 and x86_64 (mock) OK - BuildRequires sufficient OK - File permissions set properly OK - Macro usage is consistent OK - rpmlint is silent OK - Proper debuginfo packages APPROVED (thanks to everybody doing inofficial reviews, made the official review a lot easier) Please follow http://fedoraproject.org/wiki/CVSAdminProcedure and import the package. Close this bug as RAWHIDE once it's been successfully imported and built.
New Package CVS Request ======================= Package Name: anyterm Short Description: A web-based terminal emulator Owners: mmorsi Branches: F-11 F-12 InitialCC:
(In reply to comment #35) what you think about maintaining the EL-5 branch too ?
cvs done with the exception of F-12, we are not doing F-12 branches yet.
I'd offer to co-maintain for EL-5 if I was sponsored.
(In reply to comment #38) > I'd offer to co-maintain for EL-5 if I was sponsored. to get sponsored, you need to send something for review, after a package approved you can apply to co-maintain this package :-)
anyterm-1.1.29-8.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/anyterm-1.1.29-8.fc11
anyterm-1.1.29-8.fc11 has been pushed to the Fedora 11 stable repository. If problems still persist, please make note of it in this bug report.