Bug 458054 (arm4)

Hi, I'll do a full review, and can sponsor you once this is approved.  Also, I'd like to see some unofficial reviews of others' packages.  Post links here.

To begin with, rpmlint on SRPM:

arm4.i386: W: devel-file-in-non-devel-package /usr/lib/libarm4.so
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

arm4.i386: W: devel-file-in-non-devel-package /usr/lib/libarm4_null.so
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

Probably ok.

arm4.i386: W: non-standard-uid /var/arm4 arm4
A file in this package is owned by a non standard user. Standard users are:
root, bin, daemon, adm, lp, sync, shutdown, halt, mail, news, uucp, operator,
games, gopher, ftp, nobody.

arm4.i386: W: non-standard-gid /var/arm4 arm4
A file in this package is owned by a non standard group. Standard groups are:
root, bin, daemon, sys, adm, tty, disk, lp, mem, kmem, wheel, mail, news,
uucp, man, games, gopher, dip, ftp, lock, nobody, users.

Ok.  But might this not be better placed in /var/lib?  What is this dir for?

arm4.i386: W: no-reload-entry /etc/rc.d/init.d/arm4
In your init script (/etc/rc.d/init.d/your_file), you don't have a 'reload'
entry, which is necessary for good functionality.

Fix.  If the daemon doesn't support this, just duplicate the restart entry.

arm4-devel.i386: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

Fix.

arm4-java.i386: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

Fix.

Thanks for the review. I'm already in the midst of preparing an updated version, and most of your comments have already been incorporated. I need some advice from the selinux guys before the directory setup is complete, as some temp files may have to move out of temp, so it may be a couple of days before I get an updated version ready for review.

Some comments...

arm4.i386: W: devel-file-in-non-devel-package /usr/lib/libarm4.so
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

arm4.i386: W: devel-file-in-non-devel-package /usr/lib/libarm4_null.so
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

These are needed in the non-devel versions, as many ARM programs are designed to dynamically link against these libraries. An example is the apache mod_arm4 module, which I'll be packaging as soon as I get the selinux issues worked out :)

I've moved the offending directories from /var/arm4 to /var/lib/arm4.

There is documentation for the devel and java packages that will be included.

I'm updating the start up file as you requested.

Finally, if you're reviewing this, you may want to consider reviewing 458279 which is a python module related to this.

Thanks for your efforts!

Sounds good.  I'll watch this space for updates.

Update?

I'm encountering many small selinux issues that keep holding me up. I'm going to prepare a non-selinux release until these are fixed. I should have this ready within a day or two.

Excellent.

Updated version based on the just released upstream version:

Spec URL: http://arm4.org/Downloads/arm4.spec
SRPM URL: http://arm4.org/Downloads/arm4-0.8-0.1.fc9.beta2.src.rpm
Description: An open source implementation of the Open Group's Application
Response Measurement standard Version 4.0. This agent and daemon is capable of
measuring transaction response times across multiple tiers and correlating them
to determine the true source of application response problems. See
http://www.arm4.org for more information.


NOTE: This doesn't include any SE Linux support. That will be done as a follow on once this is in the system.

Whoops... a little too quick on the enter... the correct files are:

Spec URL: http://arm4.org/Downloads/0.8-0.4/arm4.spec
SRPM URL: http://arm4.org/Downloads/0.8-0.4/arm4-0.8-0.4.fc9.src.rpm

Practice reviewed new package python-foolscap bug number 462535

rpmlint clean, except for some things above that we've come to an understanding, and these:

arm4.i386: W: shared-lib-calls-exit /usr/lib/libarm4db_bdb.so.1.0.0 exit
This library package calls exit() or _exit(), probably in a non-fork()
context. Doing so from a library is strongly discouraged - when a library
function calls exit(), it prevents the calling program from handling the
error, reporting it to the user, closing files properly, and cleaning up any
state that the program has. It is preferred for the library to return an
actual error code and let the calling program decide how to handle the
situation.

arm4.i386: W: shared-lib-calls-exit /usr/lib/libarm4db_common.so.1.0.0 exit
This library package calls exit() or _exit(), probably in a non-fork()
context. Doing so from a library is strongly discouraged - when a library
function calls exit(), it prevents the calling program from handling the
error, reporting it to the user, closing files properly, and cleaning up any
state that the program has. It is preferred for the library to return an
actual error code and let the calling program decide how to handle the
situation.

arm4.i386: W: shared-lib-calls-exit /usr/lib/libarm4db.so.1.0.0 exit
This library package calls exit() or _exit(), probably in a non-fork()
context. Doing so from a library is strongly discouraged - when a library
function calls exit(), it prevents the calling program from handling the
error, reporting it to the user, closing files properly, and cleaning up any
state that the program has. It is preferred for the library to return an
actual error code and let the calling program decide how to handle the
situation.

How'd you get those warnings? rpmlint doesn't show them for me.

These calls are made when assertions fail. I can remove them.

(In reply to comment #11)
> How'd you get those warnings? rpmlint doesn't show them for me.

Not sure.  I just updated to the very latest F-9 rpmlint, rpmlint-0.85-2.fc9.noarch. 


> These calls are made when assertions fail. I can remove them.

Probably best to.

Updated versions:

Spec URL: http://arm4.org/Downloads/0.8-0.5/arm4.spec
SRPM URL: http://arm4.org/Downloads/0.8-0.5/arm4-0.8-0.5.fc9.src.rpm

I must say though, it's kind of annoying to hit an rpmlint update in the few short hour between submission and review :)

Some comments on these reported errors though. First of all, despite the assertion of rpmlint, there are many cases where this is a valid design choice, such as when a common error handler is put in a library. In this case, it was used in panic conditions only, and I still think that's valid. At this point, the fix is largely cosmetic, although I will address this more completely for a future release.

Which brings me to the larger issue. Fixing this will always require an architectural rethink. At the very least, it will require a change to the library version number, potentially putting it out of sync with the upstream version. Ultimately this isn't the responsibility of the packager, but of the original developer, and as pointed out already the developer may just say deal with it. Honestly, if I weren't also the upstream developer, I wouldn't have changed anything. As it is, I'm fixing internal libraries that provide no public API for problems that I don't think exist.

I guess my point is that this warning should largely be ignored during reviews. The dangers of not doing so are too great.

OK, rant done, the new code removes these warnings.

With regard to your comments on the practice review, I did use the checklist at http://fedoraproject.org/wiki/Packaging/ReviewGuidelines. I'll do a better job of noting the pass/fail states next time. I'll do a couple of additional reviews later this week when I get some of my work out of the way.

Thanks!
Dave

(In reply to comment #13)
> Updated versions:
> 
> Spec URL: http://arm4.org/Downloads/0.8-0.5/arm4.spec
> SRPM URL: http://arm4.org/Downloads/0.8-0.5/arm4-0.8-0.5.fc9.src.rpm
> 
> I must say though, it's kind of annoying to hit an rpmlint update in the few
> short hour between submission and review :)

Fortunately that isn't THAT common. :)

> Some comments on these reported errors though. First of all, despite the
> assertion of rpmlint, there are many cases where this is a valid design choice,
> such as when a common error handler is put in a library. In this case, it was
> used in panic conditions only, and I still think that's valid. At this point,
> the fix is largely cosmetic, although I will address this more completely for a
> future release.
> 
> Which brings me to the larger issue. Fixing this will always require an
> architectural rethink. At the very least, it will require a change to the
> library version number, potentially putting it out of sync with the upstream
> version. Ultimately this isn't the responsibility of the packager, but of the
> original developer, and as pointed out already the developer may just say deal
> with it. Honestly, if I weren't also the upstream developer, I wouldn't have
> changed anything. As it is, I'm fixing internal libraries that provide no
> public API for problems that I don't think exist.
> 
> I guess my point is that this warning should largely be ignored during reviews.
> The dangers of not doing so are too great.
> 
> OK, rant done, the new code removes these warnings.

If you really feel that strongly about it, why not make that case in a bug against rpmlint?  It's always being changed to adapt to different use cases.  And remember, rpmlint silence is not a prerequisite for approval, especially where warnings are concerned, it's more there to encourage discussion, documentation, and to make sure that both packager and reviewer have a better idea what's really going on.

> With regard to your comments on the practice review, I did use the checklist at
> http://fedoraproject.org/wiki/Packaging/ReviewGuidelines. I'll do a better job
> of noting the pass/fail states next time. I'll do a couple of additional
> reviews later this week when I get some of my work out of the way.

Sounds good.  One or two more, plus this package's approval, and we're in business.  I'll post a formal review of this package soon.
 
> Thanks!
> Dave

MUST: rpmlint must be run on every package. The output should be posted in the review.

Clean now. :)

- 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 on Package Naming Guidelines .

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 .
- MUST: The License field in the package spec file must match the actual license.
- 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.

Says EPL, which is ok, but what's in COPYING?

- MUST: The spec file must be written in American English.

OK.

- MUST: The spec file for the package MUST be legible. If the reviewer is unable to read the spec file, it will be impossible to perform a review. Fedora is not the place for entries into the Obfuscated Code Contest (http://www.ioccc.org/).

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.

OK.

- MUST: The package must successfully compile and build into binary rpms on at least one supported architecture.

OK on i386 F-9.

- 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 needs to have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number should then be placed in a comment, next to the corresponding ExcludeArch line. New packages will not have bugzilla entries during the review process, so they should put this description in the comment until the package is approved, then file the bugzilla entry, and replace the long explanation with the bug number. The bug should be marked as blocking one (or more) of the following bugs to simplify tracking such issues: FE-ExcludeArch-x86 , FE-ExcludeArch-x64 , FE-ExcludeArch-ppc , FE-ExcludeArch-ppc64

NA.

- 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.

- MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.

OK.

- MUST: Every binary RPM package which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. If the package has multiple subpackages with libraries, each subpackage should also have a %post/%postun section that calls /sbin/ldconfig. An example of the correct syntax for this is:

%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig


OK.

- 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. Refer to the Guidelines for examples.

OK.

- MUST: A package must not contain any duplicate files in the %files listing.

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, as described in the macros section of Packaging Guidelines .

OK.

- MUST: The package must contain code, or permissable content. This is described in detail in the code vs. content section of Packaging Guidelines .

OK.

- MUST: Large documentation files should 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.

OK.

- MUST: Header files must be in a -devel package.
- MUST: Static libraries must be in a -static package.

Do the static and shared libs need to be together for some reason, or could they be separated into -devel and -static?

- MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability).

OK.

- 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.

OK.

- 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 should be removed in the spec.

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. This is described in detail in the desktop files section of the Packaging Guidelines . 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.

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 ). See Prepping BuildRoot For %install for details.

OK.

- MUST: All filenames in rpm packages must be valid UTF-8.

OK.

Summary: licensing and static libs.

I'm been looking into the two issues you mentioned. Static libraries will be a quick fix.

The license stuff may take a bit more thought. I've been so involved in the project I forgot about the issue of the SDK code. It's certainly not a significant part of the project. My options are to rip out the offending code (used mostly in the tests), or to get it approved as is. It is potentially useful as part of the distributed package as well. I'll keep you posted.

- Dave

I stumbled over this Review Request and I'm happy to see that the arm4.org ARM is in process to be included into Red Hat distribution.

Just a comment about static libraries. I do not think a static library should be build. ARM is designed as a dynamic library which can be linked to an application implicitly by using -larm4 during link time. But the main usage is to dlopen() the libarm4.so. Just like the Apache mod_arm4 does.

See section "1.7 Linking to an ARM Implementation" of the standard documents for ARM 4.0 or ARM 4.1!

I don't know the Red Hat policies for libraries. But we should not encourage users of ARM to link statically!

Any comments?

Regards,
Stefan

RH/Fedora discourages, but does not outright forbid static libraries.

https://fedoraproject.org/wiki/Packaging/Guidelines#Exclusion_of_Static_Libraries

Good point Stefan. Truth is, I hadn't realized they were in there until review. Now that life's little annoyances are out of the way, I should be able to get an update in the next couple of days.

BTW Jon, Stefan is one of the stakeholders in that extra license file :)

The legal opinion is in. Looks like I'll have to some modification.

From Tom Callaway:
"Unfortunately, it isn't okay.

This is non-free, because there is no permission to redistribute
modified versions of code under this license, only to "Make copies of
the original file you download and distribute it". That, combined with:
"Except as expressly stated above, HP and tang-IT grant no other
licenses, express or implied, by estoppel or otherwise, to any
intellectual property rights." means that we cannot assume permission to
distribute modified/derived works is implied.

HP usually does a better job of this, perhaps they'd be willing to fix
this license, or re-license this work under an established free license?"

Stefan, any comments on the last paragraph? I know licensing held up the initial SDK release

I'm the author of some parts of the ARM4SDK but I'm not the copyright owner. Also it seems that the ARM4SDK development is dead since 2003 or 2004.

In the meanwhile I have founded my own company MyARM. And we at MyARM would also prefer to see the ARM4SDK license changed to a free license. I can write an email regarding ARM4SDK license change to the people how were involved in the development of the ARM4SDK at that time!

David, you are right release delay of the ARM4SDK was about licensing issues.

Updated versions:

Spec URL: http://arm4.org/Downloads/0.8.1-0.6/arm4.spec
SRPM URL: http://arm4.org/Downloads/0.8.1-0.6/arm4-0.8.1-0.6.fc9.src.rpm

Based on the 0.8.1 release which removed all code related to the SDK. Unfortunately this includes the test programs, so those packages have been removed as well.

Also, as per Stefan's suggestion, all static libraries have been removed.

Passed all the usual build tests with the same rpmlint warnings as before.

6th time's the charm? :)

Looks good.  And we thus have Mr. Callaway's legal blessing?

All that remains is Eclipse licensed, so we're good.

Then without further ado. . .

APPROVED.

New practice review completed (#466331). I'll do one or two more this week now that exams are over.

Also, the folks at HP are in discussions about relaxing the license restrictions on the code that got removed.

Sounds great.  I'll be AFK all next week, so I'll go over the whole bundle Monday the 28th.  Or so. :)

Sorry for the colossal delay, work and real life have been full as of late.  I commented on 466331.  Any others?

Yes, I re-reviewed 462535 after some changes. Also 479413 which has since been closed.

I haven't reviewed any others as my life has been hectic as well :)

I may have some new modules over the next couple of months though...

Those look solid, and demonstrate progress up the Packaging Guidlines learning curve. :)  So you still need a sponsor?

I'm willing to commit (figuratively and literally) if you're willing to sponsor :)

I am, just wanted to make sure you'd not found another in the interim.

APPROVED.

I've also marked your FAS account sponsored, so once that all propagates, do you CVS request, import, build, and close this bug.

Welcome aboard, and feel free to bug me with questions, etc.

New Package CVS Request
=======================
Package Name: arm4
Short Description: Application Response Measurement V4.0
Owners: dcarter
Branches: F-10 F-11
InitialCC: limb

New Package CVS Request
=======================
Package Name: arm4
Short Description: Application Response Measurement V4.0
Owners: grandcross
Branches: F-10 F-11
InitialCC: limb

cvs done.