Bug 491694 - Review Request: anyterm - Web based terminal emulator
Review Request: anyterm - Web based terminal emulator
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: David Lutterkort
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-23 12:42 EDT by Mo Morsi
Modified: 2013-04-30 19:40 EDT (History)
8 users (show)

See Also:
Fixed In Version: 1.1.29-8.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-28 12:33:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lutter: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Mo Morsi 2009-03-23 12:42:46 EDT
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.
Comment 1 Itamar Reis Peixoto 2009-03-23 12:57:35 EDT
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
Comment 2 Mo Morsi 2009-03-23 17:27:20 EDT
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!
Comment 3 Gianluca Sforna 2009-03-23 17:41:31 EDT
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)
Comment 4 Itamar Reis Peixoto 2009-03-23 18:50:04 EDT
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
Comment 5 Mo Morsi 2009-03-23 20:07:04 EDT
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
Comment 6 Itamar Reis Peixoto 2009-03-23 20:14:16 EDT
Can you include a koji build in dist-f11 ?
Comment 7 Itamar Reis Peixoto 2009-03-23 20:21:45 EDT
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
Comment 8 Gianluca Sforna 2009-03-24 05:39:06 EDT
(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?
Comment 9 Alexander Boström 2009-03-24 08:10:25 EDT
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.
Comment 10 Mo Morsi 2009-04-03 16:19:41 EDT
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
Comment 11 Gianluca Sforna 2009-04-07 02:58:55 EDT
(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
Comment 12 Alexander Boström 2009-04-07 05:33:46 EDT
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.
Comment 13 Mo Morsi 2009-04-08 20:54:31 EDT
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!
Comment 14 Alexander Boström 2009-04-09 05:05:06 EDT
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.)
Comment 15 Alexander Boström 2009-04-09 08:09:05 EDT
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.
Comment 16 Mo Morsi 2009-04-09 17:11:19 EDT
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.
Comment 17 Mo Morsi 2009-07-08 15:06:22 EDT
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?)
Comment 18 Gianluca Sforna 2009-07-09 03:50:54 EDT
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.
Comment 19 Mo Morsi 2009-07-09 11:00:06 EDT
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.
Comment 20 Mo Morsi 2009-07-09 11:14:23 EDT
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
Comment 21 Alexander Boström 2009-07-13 11:31:16 EDT
I rebuilt it on Fedora 11 and rpmlint was silent for me.
Comment 22 Alexander Boström 2009-07-13 12:54:39 EDT
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.
Comment 23 Mo Morsi 2009-07-13 18:18:57 EDT
(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
Comment 24 Alexander Boström 2009-07-14 04:51:52 EDT
(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.
Comment 25 Mo Morsi 2009-07-14 15:19:05 EDT
(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
Comment 26 Alexander Boström 2009-07-15 09:19:18 EDT
(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.
Comment 27 Mo Morsi 2009-07-15 10:45:49 EDT
> 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
Comment 28 Mo Morsi 2009-07-17 15:41:31 EDT
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.
Comment 29 David Lutterkort 2009-07-17 17:57:22 EDT
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
Comment 30 Mo Morsi 2009-07-20 10:06:20 EDT
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.
Comment 31 Mo Morsi 2009-07-23 17:17:20 EDT
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.
Comment 32 David Lutterkort 2009-07-27 11:55:01 EDT
This looks good now, the only issue is that there is a type in the BuildRoot: it should use %{name}, not ${name}
Comment 33 Mo Morsi 2009-07-27 12:06:21 EDT
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
Comment 34 David Lutterkort 2009-07-27 12:10:45 EDT
  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.
Comment 35 Mo Morsi 2009-07-27 13:55:24 EDT
New Package CVS Request
=======================
Package Name: anyterm
Short Description:  A web-based terminal emulator
Owners: mmorsi
Branches: F-11 F-12
InitialCC:
Comment 36 Itamar Reis Peixoto 2009-07-27 14:05:03 EDT
(In reply to comment #35)
what you think about maintaining the EL-5 branch too ?
Comment 37 Kevin Fenzi 2009-07-28 00:32:24 EDT
cvs done with the exception of F-12, we are not doing F-12 branches yet.
Comment 38 Alexander Boström 2009-07-28 05:03:51 EDT
I'd offer to co-maintain for EL-5 if I was sponsored.
Comment 39 Itamar Reis Peixoto 2009-07-28 06:53:39 EDT
(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 

:-)
Comment 40 Fedora Update System 2009-07-28 12:32:24 EDT
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
Comment 41 Fedora Update System 2009-07-29 17:27:21 EDT
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.

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