Bug 493236 - Review Request: xmlfy - Convert text/UTF-8 based output into XML format
Review Request: xmlfy - Convert text/UTF-8 based output into XML format
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Jan Klepek
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-31 22:47 EDT by arthurguru
Modified: 2009-07-05 16:15 EDT (History)
6 users (show)

See Also:
Fixed In Version: 1.4.3-1.fc9
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-06-04 17:15:10 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lkundrak: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description arthurguru 2009-03-31 22:47:43 EDT
Spec URL: http://xmlfy.sourceforge.net/submit/fedora/xmlfy.spec
SRPM URL: http://xmlfy.sourceforge.net/submit/fedora/xmlfy-1.4.2-1.src.rpm
Description: 

Hello, I've got a package called xmlfy for your consideration into the Fedora project. I've put together a formal submission page to make things a little easier:
http://xmlfy.sourceforge.net/submit/fedora/index.html

The xmlfy shell command is a powerful yet lightweight tool that primarily
caters for converting text or UTF-8 based output into XML format on
the fly and dealing with common issues associated with this kind of
transformation. 

It is predominantly written to alleviate the need for writing custom scripts for this purpose and to produce consistent outcomes in dealing with this kind of transformation problem.

Kind regards,
Arthur Gouros.
Comment 1 Jason Tibbitts 2009-04-01 15:22:31 EDT
Is this your first package for Fedora?  Have you read http://fedoraproject.org/wiki/PackageMaintainers/Join?

Your package does not build for me on my x86_64 machine.  You really should not use BuildArch unless you're specifying "BuildArch: noarch".  If your code really does not work on anything other than i386, you want "ExclusiveArch: i386", although in the modern world it's hard to imagine anyone writing C code that works only on i386.

It is rather odd to put the license file for the program directly into the spec file.  It's permissible for you to license the spec file if you want, as long as it's still sufficiently free (which yours is) but honestly it's legally questionable as to whether there's anything significant in a spec file which is eligible for copyright anyway.  Really what it does is clutter the spec with a bunch of junk you have to ignore before you get to the useful part.

You might consider using the dist tag to make maintaing the package across multiple Fedora versions easier.  http://fedoraproject.org/wiki/Packaging/DistTag
Comment 2 arthurguru 2009-04-01 22:20:55 EDT
Thanks Jason,

The spec file I used originally came from a Java application I used to maintain and had the line "BuildArch: noarch" which I changed in haste to "BuildArch: i386" which I now know is wrong.

Thanks also for the other helpful hints you made, they will be keenly applied.

Kind regards,
Arthur Gouros.
Comment 3 arthurguru 2009-04-03 00:55:16 EDT
Thanks for the feedback, changes have been applied and the package is ready for another review.

The same links apply (wasn't sure whether I should up the release number and break the links in the original description, so I left them as they are).

Changes I made:
Removed that superfluous license blurb inside the spec file.
Removed the BuildArch: directive.
Added the dist tag to the Release: directive.

Tested ok, rpmlint is still content.

Looking forward to your feedback.

Kind regards,
Arthur Gouros.
Comment 4 Jan Klepek 2009-04-08 12:16:56 EDT
I'm unable to build it on my system.

1] i would use in %install install -p instead install to preserve timestamps

2] in your Source you have reference to archive which contains package folder and make all in %build is trying to create package by itself. Not good, my build fails on this.

you could remove folder and update Makefile in %prep section

3] %install section needs rewrite to correct paths
Comment 5 arthurguru 2009-04-13 20:33:01 EDT
Thanks for the feedback Jan.

1] Timestamps are now preserved.

2] Which "Source" are you referring to, the SRPM or the upstream source file xmlfy-1.4.2-src.tar.gz? If you use the upstream source the build will fail at the packaging - I use another top level Makefile "Makefile_src_archive" to build the source archives including the SRPM. When I build from the SRPM it works for me, perhaps it could be the path issue that you refer to in point 3.

3] Path references in %install section have now been standardized.
FYI: In the SRPM source the "make all" in the %build section also runs a package "stage" target which is also the build's own workspace. These package dir references have now been taken out of the spec file and replaced with a DESTDIR option making the spec file look more standard.

Once again I’ve updated the files with the same names as to not break the links in the original post.

Tested ok, rpmlint is still content.

Looking forward to your feedback.

Kind regards,
Arthur Gouros.
Comment 6 Jan Klepek 2009-04-15 02:12:59 EDT
(In reply to comment #5)

> 2] Which "Source" are you referring to, the SRPM or the upstream source file
> xmlfy-1.4.2-src.tar.gz? If you use the upstream source the build will fail at
> the packaging - I use another top level Makefile "Makefile_src_archive" to
> build the source archives including the SRPM. When I build from the SRPM it
> works for me, perhaps it could be the path issue that you refer to in point 3.

Yes, I'm talking about upstream source. 
In upstream source which you are providing in your spec file, you have package subfolder which prevent successfull build.
you could:
1] remove this directory and modify Makefile, to prevent rpmbuild which is executed by make in %build section
2] create upstream source which would be modified as per 1]
3] modify Makefile to prevent rpmbuild which is executed by make in %build section

source tarball in your src.rpm differs from tarball which could be downloaded from Source0.
This breaks following rule:
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL.

md5sum of tarball from provided src.rpm:
7da9b3ee78500687cc4f357f01e92f3b  xmlfy-1.4.2-src.tar.gz
md5sum of tarball from Source0:
e02f4dfb6bd94676d3a72e315da2a10a  xmlfy-1.4.2-src.tar.gz
Comment 7 arthurguru 2009-04-16 21:46:53 EDT
Once again thanks for the very accurate feedback Jan.

Your analysis is very good. I have an intermediate process to build fedora source from the original source and then build the SRPM from that, but as this is causing another layer of concern I have logged a bug against my own code to get rid of that process.

To fix this properly I need to do another formal release which involves more than just Fedora, so in the meantime I have branched the source code for this Fedora project and all changes provided will now come from this branch and in the end will be merged into the next release of xmlfy which is some weeks away.

Now the upstream source specified in Source0 (from the branch) IS exactly the same file used in the SRPM.

Once again I’ve updated the files with the same names as to not break the links
in the original post.

Tested ok, rpmlint is still content.
Also note the new URL location of Source0: in the spec file.


Kind Regards,
Arthur Gouros.
Comment 8 Jan Klepek 2009-04-18 05:20:01 EDT
Source0: http://xmlfy.sourceforge.net/submit/fedora/xmlfy/xmlfy-1.4.2-src.tar.gz
404, file not found.

you doesn't need to branch source code...
you could do in %prep following actions:
%setup -q
rm -rf package
# remove package directory from Makefile
sed -i 's/.*package.*//g' Makefile

and build will be ok, tested with this source: 
Source0: http://downloads.sourceforge.net/%{name}/%{name}-%{version}-src.tar.gz

and in this tarball README and LICENSE are in dos encoding, so you have to convert it to unix encoding with help of sed and chmod it to more correct permissions (644 would be ok).
Comment 9 arthurguru 2009-04-20 00:29:12 EDT
So sorry about the link Jan, I missed an "xmlfy", anyhow I've moved the source file location to match the link path
http://xmlfy.sourceforge.net/submit/fedora/xmlfy/xmlfy-1.4.2-src.tar.gz

Thanks also for the pointers on updating the %prep section, however I feel that the changes I made on the branch are beneficial to the xmlfy project and will be incorporated in the next release in a few weeks. The pre-Fedora source of xmlfy had no "install" target and built a native package instead (blame my ITIL background), but the changes I've adopted through getting the xmlfy project up to the quality standards required by the fedora project now make the xmlfy project look like many other projects of similar category and scope. Thanks.

The file perms and encoding of the files README and LICENSE were corrected in the tarball of the link in this post.

Kind Regards,
Arthur Gouros.
Comment 10 Jan Klepek 2009-04-26 03:24:28 EDT
in your xmlfy/Makefile you have LDFLAGS = -O2 -s
could you remove -s and add into CFLAGS -g ?
or you could do in %prep add 
sed -i 's/CFLAGS=-O2/CFLAGS=-O2 -g/g' xmlfy/Makefile
sed -i 's/LDFLAGS=-O2 -s/LDFLAGS=-O2/g' xmlfy/Makefile

for reason, please see:
https://fedoraproject.org/wiki/Packaging/Debuginfo
otherwise rpmlint complains on empty debuginfo package.

so, my full package review
MUST: rpmlint must be run on every package. The output should be posted in the review.
- debuginfo package empty
- Not OK

MUST: The package must be named according to the Package Naming Guidelines .
- OK

MUST: The spec file name must match the base package %{name}
- OK

MUST: The package must meet the Packaging Guidelines
- debuginfo package empty
- NOT OK

MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
- OK

MUST: The License field in the package spec file must match the actual license.
- OK

MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc
- OK

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

MUST: The spec file for the package MUST be legible.
- OK

MUST: The sources used to build the package must match the upstream source
- OK

MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
- OK, koji build http://koji.fedoraproject.org/koji/taskinfo?taskID=1321066

MUST: If the package does not successfully compile, build or work on an architecture
- OK

MUST: All build dependencies must be listed in BuildRequires
- OK

MUST: The spec file MUST handle locales properly.
- OK, no locales available

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.
- OK, no shared library available

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, no relocatable package

MUST: A package must own all directories that it creates. 
- OK

MUST: A Fedora package must not list a file more than once in the spec file's %files listings.
- OK

MUST: Permissions on files must be set properly. 
- OK

MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
- OK

MUST: Each package must consistently use macros.
- OK

MUST: The package must contain code, or permissable content. 
- OK

MUST: Large documentation files must go in a -doc subpackage. 
- OK, no large documentation

MUST: If a package includes something as %doc, it must not affect the runtime of the application.
- OK

MUST: Header files must be in a -devel package. 
- OK, no header files

MUST: Static libraries must be in a -static package.
- OK, no static libraries

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

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, no library files 

MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
- OK, no devel package

MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
- OK, no .la files

MUST: Packages containing GUI applications must include a %{name}.desktop file
- OK, no gui available

MUST: Packages must not own files or directories already owned by other packages. 
- OK
MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
- OK
MUST: All filenames in rpm packages must be valid UTF-8. 
- OK

Conclusion, not properly created debuginfo package => does not meet http://fedoraproject.org/wiki/Packaging/Guidelines#Debuginfo_packages
Comment 11 Lubomir Rintel 2009-04-26 03:52:06 EDT
arthurguru: you need to find someone to sponsor you before Jan can approve your package.

I can sponsor you, but I'd be thankful if you could informal reviews of a package or two before. That's usually done to demonstrate that you are familiar with packaging guidelines.
Comment 12 arthurguru 2009-04-28 22:14:50 EDT
Jan,

Thanks for your feedback. The debuginfo rpm issue has now been resolved using RPM_OPT_FLAGS, thanks.

In the meantime I've made another point release of xmlfy that also incorporates all the good work identified in this review process (branch merge). This also means the links to the RPMs have changed from the original post.

Spec file
http://xmlfy.sourceforge.net/submit/fedora/xmlfy.spec

Source RPM
http://xmlfy.sourceforge.net/submit/fedora/xmlfy-1.4.3-1.src.rpm

Binary RPM derived from the above source RPM
http://xmlfy.sourceforge.net/submit/fedora/xmlfy-1.4.3-1.i386.rpm

Other files
http://xmlfy.sourceforge.net/submit/fedora/xmlfy-debuginfo-1.4.3-1.i386.rpm

Tested ok, rpmlint is content.


Lubomir,

Thanks for sponsoring the xmlfy project. I have enjoyed the review process and the quality it yields, and totally agree with strengthening ones knowledge of the package review guidelines rather than it being a "one-off" process.

I would be more than happy to participate in an informal package review or three. How are these informal reviews conducted?

Thanks to all
Arthur Gouros.
Comment 13 Jan Klepek 2009-04-29 02:36:59 EDT
Hi Arthur,

I will take a look at your latest package during today/tommorow.

For informal package review, you should follow
http://fedoraproject.org/wiki/Package_Review_Process

Just pickup any review request which is not yet assigned and perform review 
(for example, you could do review on this https://bugzilla.redhat.com/show_bug.cgi?id=497759, please be aware that there are specific checks for perl/python/php.... packages http://fedoraproject.org/wiki/Packaging/Guidelines#Application_Specific_Guidelines )
Comment 14 Lubomir Rintel 2009-05-03 04:44:24 EDT
ping?
Comment 15 arthurguru 2009-05-03 23:24:59 EDT
Packet delayed - I use the far less luxurious option of internet dial-up - though not by choice.  

Is there anything outstanding?
Comment 16 Jan Klepek 2009-05-04 17:06:04 EDT
last review of package looks ok from my point of view

builded in koji for f10/f11
F-11: http://koji.fedoraproject.org/koji/taskinfo?taskID=1335826
F-10: http://koji.fedoraproject.org/koji/taskinfo?taskID=1335835

Arthur,

you could probably put here links to review which you have done or on which you are working, or another packages which you have submitted.
I see that you work on https://bugzilla.redhat.com/show_bug.cgi?id=497759, 
you could do informal review for any other package.
Comment 17 arthurguru 2009-05-07 20:04:18 EDT
QA,

Just did an informal review on the following package:
https://bugzilla.redhat.com/show_bug.cgi?id=494986

Kind Regards,
Arthur Gouros.
Comment 18 Lubomir Rintel 2009-05-08 04:14:56 EDT
Arthur: The review seems mostly ok. Please ensure you understand which packages are considered -devel (feel free to ask if the guidelines are unclear to you). Please add yourself to to "packager" group in the account system, I'll sponsor your account.

It would also be awesome if you could do a couple more reviews, there's always something in the queue, and getting more familiar with RPM packaging won't hurt.
Comment 19 arthurguru 2009-05-11 20:42:41 EDT
QA,

Just did another informal review on the following package:
https://bugzilla.redhat.com/show_bug.cgi?id=499951

Lubomir, some delay (local server issues) in generating a ssh-key in order to join packager group.

Kind Regards,
Arthur Gouros.
Comment 20 Lubomir Rintel 2009-05-19 04:26:32 EDT
The package is APPROVED.

Since you're sponsored now, please proceed requesting CVS and import your package.
Thanks!
Comment 21 arthurguru 2009-05-19 21:24:34 EDT
New Package CVS Request
=======================
Package Name: xmlfy
Short Description: Convert text/UTF-8 based output into XML format
Owners: arthurguru
Branches: F-9 F-10 F-11
InitialCC: lkundrak hpejakle
Comment 22 Kevin Fenzi 2009-05-20 01:42:19 EDT
cvs done.
Comment 23 Fedora Update System 2009-06-02 22:30:55 EDT
xmlfy-1.4.3-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/xmlfy-1.4.3-1.fc9
Comment 24 Fedora Update System 2009-06-02 22:38:00 EDT
xmlfy-1.4.3-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/xmlfy-1.4.3-1.fc10
Comment 25 Fedora Update System 2009-06-02 23:02:45 EDT
xmlfy-1.4.3-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/xmlfy-1.4.3-1.fc11
Comment 26 Fedora Update System 2009-06-04 17:15:03 EDT
xmlfy-1.4.3-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 27 Fedora Update System 2009-06-04 17:17:28 EDT
xmlfy-1.4.3-1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 28 Fedora Update System 2009-06-04 17:22:07 EDT
xmlfy-1.4.3-1.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 29 Susi Lehtola 2009-07-05 16:15:23 EDT
Once you sponsor someone, please remember to remove the FE-NEEDSPONSOR tag to
avoid noise in the blocker bug.

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