Bug 855780 - Review Request: apacheds-daemon - Reusable Daemon Framework
Review Request: apacheds-daemon - Reusable Daemon Framework
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On: 977901 977904
Blocks: 823967
  Show dependency treegraph
 
Reported: 2012-09-10 05:26 EDT by gil cattaneo
Modified: 2015-03-31 12:23 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-03-31 12:23:23 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description gil cattaneo 2012-09-10 05:26:56 EDT
Spec URL: http://gil.fedorapeople.org/apacheds-daemon.spec
SRPM URL: http://gil.fedorapeople.org/apacheds-daemon-1.1.8-1.fc16.src.rpm
Description: Reusable framework for daemon applications based on Commons Daemon
Jsvc and Procrun. A small installation layout pattern combined with
some utility classes allows applications to be come UNIX daemons or
Windows NT services. Reusable bootstrappers along with an installer
plugin allow for the rapid creation of standalone daemon applications.
Fedora Account System Username: gil
ApacheDS/Studio build/requires
Comment 1 Mikolaj Izdebski 2012-09-18 09:25:45 EDT
I am taking this review.
Comment 2 Mikolaj Izdebski 2012-10-09 10:31:15 EDT
Fails to build in rawhide mock:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4574340
Comment 3 Mikolaj Izdebski 2012-10-09 10:47:18 EDT
This package depends on tanukisoft:wrapper, but currently nothing in fedora provides this artifact.
Comment 4 gil cattaneo 2012-10-09 12:14:35 EDT
build fine also with java-service-wrapper
but i dont know how fix the problem if use x86_64 arch in koji
Comment 5 Mikolaj Izdebski 2012-10-09 12:34:08 EDT
(In reply to comment #4)
> build fine also with java-service-wrapper
> but i dont know how fix the problem if use x86_64 arch in koji

This will definitely have to be resolved.
Comment 7 Mikolaj Izdebski 2012-10-26 06:40:44 EDT
As it's now the package won't work peoperly, the problem is libdir usage in noarch package.

If package is built on 64-bit system then <systemPath> in pom.xml file will contain reference to /usr/lib64 and this package will work only on 64-bit systems (and vice versa, if the package is compiled on 32-bit system it will only work on 32-bit systems).
Comment 8 Mikolaj Izdebski 2013-06-24 18:28:26 EDT
Somehow I missed this bug, sorry for the delay.
I'll try to review this package ASAP.
Comment 9 gil cattaneo 2013-06-25 04:58:08 EDT
Spec URL: http://gil.fedorapeople.org/apacheds-daemon.spec
SRPM URL: http://gil.fedorapeople.org/apacheds-daemon-1.1.8-2.fc18.src.rpm

- minor changes for the new guideline
Comment 10 Mikolaj Izdebski 2013-06-25 07:13:38 EDT
There are several problems with this package.

First of all, the package is noarch, but depends on arch-specific packages and hardcodes paths to libraries. This means that if this package is built on 32-bit system then it won't work properly on 64-bit ones and vice-versa.

You are using Maven scope "system". What's the reason behind that? Maven should be able to locate artifact in /usr/lib too. If not then either there is a bug in the package providing the artifact or in XMvn resolver. In either case this should be reported and fixed. Workaround is acceptable only if the real issue was reported.

You are injecting "<version>any</version>" to some POMs. What's the reason? Maven should be able to work without this. If it does not, please report a bug.

You are using old packaging guidelines. Please use the new guidelines for Fedora 19+. Fedora 19 is soon to be released. After Fedora 19 GA no new packages should be added to Fedora 18, so there is no reason to retain backwards-compatibility.

> %global bits   %(rpm --eval %{__isa_bits}  | sed 's/32//')
> #%%global libdir %%{_prefix}/lib%%{bits}

That's not needed (see above).

> # svn export http://svn.apache.org/repos/asf/directory/deceased/daemon/tags/daemon-parent-1.1.8/ apacheds-daemon-1.1.8
> # rm -rf apacheds-daemon-1.1.8/plugin/src/main/resources/org/apache/directory/daemon/installers/*
> # find apacheds-daemon-1.1.8 -name '*.bat' -delete
> # find apacheds-daemon-1.1.8 -name '*.class' -delete
> # find apacheds-daemon-1.1.8 -name '*.dll' -delete
> # find apacheds-daemon-1.1.8 -name '*.exe' -delete
> # find apacheds-daemon-1.1.8 -name '*.jar' -type f -delete
> # find apacheds-daemon-1.1.8 -name '*.jnilib' -delete
> # find apacheds-daemon-1.1.8 -name 'jsvc_*' -delete
> # find apacheds-daemon-1.1.8 -name '*.so' -delete
> # find apacheds-daemon-1.1.8 -name 'wrapper-*' -type f -delete
> # tar czf apacheds-daemon-1.1.8-clean-src-svn.tar.gz apacheds-daemon-1.1.8
> Source0:       %{name}-%{version}-clean-src-svn.tar.gz

To keep spec file simpler it would be better to create a separate shell script (cleanup-tarball.sh) to do the cleaning.

> Patch0:        %{name}-%{version}-disable-izpack.patch

I don't know what is the purpose of this patch. Every patch should have link to upstream bug or a comment explaining its purpose.

> BuildRequires: java-devel

Not needed.

> sed -i 's|${version}|${project.version}|' pom.xml

Is this necessary? Either explain why or provide link to upstream bug tracker.

> %pom_xpath_inject "pom:project/pom:reporting/pom:plugins/pom:plugin[pom:artifactId='maven-surefire-report-plugin']" "
> <version>any</version>"

I don't see why would you inject version "any" to the POM. Again, you need to explain why you are doing this.

> %pom_remove_dep tanukisoft:wrapper
> %pom_xpath_inject "pom:project/pom:dependencyManagement/pom:dependencies" '
>     <dependency>
>       <groupId>tanukisoft</groupId>
>       <artifactId>wrapper</artifactId>
>       <version>any</version>
>       <scope>system</scope>
>       <systemPath>%{_prefix}/lib%{bits}/java-service-wrapper/wrapper.jar</systemPath>
>     </dependency>'
> %pom_xpath_inject "pom:project/pom:dependencies" '
>     <dependency>
>       <groupId>tanukisoft</groupId>
>       <artifactId>wrapper</artifactId>
>       <version>any</version>
>       <scope>system</scope>
>       <systemPath>%{_prefix}/lib%{bits}/java-service-wrapper/wrapper.jar</systemPath>
>     </dependency>'

There is no need to use scope system with systemPath. Maven should be able to locate all artifacts, also under /usr/lib.

> %pom_remove_dep org.apache.maven:maven-project plugin
> %pom_add_dep org.apache.maven:maven-core plugin

I understand why this was done (Maven 2 -> Maven 3 migration), but that doesn't have to be obvious for everyone. Please add a comment.

> %pom_remove_dep plexus:plexus-utils plugin
> %pom_add_dep org.codehaus.plexus:plexus-utils plugin

plexus:plexus-utils is available in Fedora. There is no need to replace it with org.codehaus.plexus:plexus-utils.
Comment 11 gil cattaneo 2013-06-25 10:16:11 EDT
(In reply to Mikolaj Izdebski from comment #10)
> 
> > %global bits   %(rpm --eval %{__isa_bits}  | sed 's/32//')
> > #%%global libdir %%{_prefix}/lib%%{bits}
> 
> That's not needed (see above).
> 
> > # svn export http://svn.apache.org/repos/asf/directory/deceased/daemon/tags/daemon-parent-1.1.8/ apacheds-daemon-1.1.8
> > # rm -rf apacheds-daemon-1.1.8/plugin/src/main/resources/org/apache/directory/daemon/installers/*
> > # find apacheds-daemon-1.1.8 -name '*.bat' -delete
> > # find apacheds-daemon-1.1.8 -name '*.class' -delete
> > # find apacheds-daemon-1.1.8 -name '*.dll' -delete
> > # find apacheds-daemon-1.1.8 -name '*.exe' -delete
> > # find apacheds-daemon-1.1.8 -name '*.jar' -type f -delete
> > # find apacheds-daemon-1.1.8 -name '*.jnilib' -delete
> > # find apacheds-daemon-1.1.8 -name 'jsvc_*' -delete
> > # find apacheds-daemon-1.1.8 -name '*.so' -delete
> > # find apacheds-daemon-1.1.8 -name 'wrapper-*' -type f -delete
> > # tar czf apacheds-daemon-1.1.8-clean-src-svn.tar.gz apacheds-daemon-1.1.8
> > Source0:       %{name}-%{version}-clean-src-svn.tar.gz
> 
> To keep spec file simpler it would be better to create a separate shell
> script (cleanup-tarball.sh) to do the cleaning.
> 
> > Patch0:        %{name}-%{version}-disable-izpack.patch
> 
> I don't know what is the purpose of this patch. Every patch should have link
> to upstream bug or a comment explaining its purpose.
> 
> > BuildRequires: java-devel
> 
> Not needed.
ok removed
> > sed -i 's|${version}|${project.version}|' pom.xml
> 
> Is this necessary? Either explain why or provide link to upstream bug
> tracker.
> 
> > %pom_xpath_inject "pom:project/pom:reporting/pom:plugins/pom:plugin[pom:artifactId='maven-surefire-report-plugin']" "
> > <version>any</version>"
> 
the intent is to remove all warnings

> I don't see why would you inject version "any" to the POM. Again, you need
> to explain why you are doing this.
> 
> > %pom_remove_dep tanukisoft:wrapper
> > %pom_xpath_inject "pom:project/pom:dependencyManagement/pom:dependencies" '
> >     <dependency>
> >       <groupId>tanukisoft</groupId>
> >       <artifactId>wrapper</artifactId>
> >       <version>any</version>
> >       <scope>system</scope>
> >       <systemPath>%{_prefix}/lib%{bits}/java-service-wrapper/wrapper.jar</systemPath>
> >     </dependency>'
> > %pom_xpath_inject "pom:project/pom:dependencies" '
> >     <dependency>
> >       <groupId>tanukisoft</groupId>
> >       <artifactId>wrapper</artifactId>
> >       <version>any</version>
> >       <scope>system</scope>
> >       <systemPath>%{_prefix}/lib%{bits}/java-service-wrapper/wrapper.jar</systemPath>
> >     </dependency>'
> 
> There is no need to use scope system with systemPath. Maven should be able
> to locate all artifacts, also under /usr/lib.
> 

java-service-wrapper don provides maven depmap or pom ... maven or xmvn work also without these file?

> > %pom_remove_dep org.apache.maven:maven-project plugin
> > %pom_add_dep org.apache.maven:maven-core plugin
> 
> I understand why this was done (), but that
> doesn't have to be obvious for everyone. Please add a comment.
> 
ok done
> > %pom_remove_dep plexus:plexus-utils plugin
> > %pom_add_dep org.codehaus.plexus:plexus-utils plugin
> 
> plexus:plexus-utils is available in Fedora. There is no need to replace it
> with org.codehaus.plexus:plexus-utils.
ok removed

Spec URL: http://gil.fedorapeople.org/apacheds-daemon.spec
SRPM URL: http://gil.fedorapeople.org/apacheds-daemon-1.1.8-2.fc18.src.rpm
Comment 12 Mikolaj Izdebski 2013-06-25 10:29:36 EDT
(In reply to gil cattaneo from comment #11)
> > > %pom_xpath_inject "pom:project/pom:reporting/pom:plugins/pom:plugin[pom:artifactId='maven-surefire-report-plugin']" "
> > > <version>any</version>"
> > 
> the intent is to remove all warnings

There should be no warnings in Fedora 19+.  If there are please file a bug report against xmvn package.

> > There is no need to use scope system with systemPath. Maven should be able
> > to locate all artifacts, also under /usr/lib.
> > 
> 
> java-service-wrapper don provides maven depmap or pom ... maven or xmvn work
> also without these file?

Then these files should be added to java-service-wrapper package.

> Spec URL: http://gil.fedorapeople.org/apacheds-daemon.spec
> SRPM URL: http://gil.fedorapeople.org/apacheds-daemon-1.1.8-2.fc18.src.rpm

The most important issues are still open.
Comment 13 Mikolaj Izdebski 2013-06-25 10:54:18 EDT
I have filled 2 bugs for java-service-wrapper package.  That's what should be done instead of adding ugly hacks like scope "system" and systemPath...
Comment 14 Mikolaj Izdebski 2013-06-25 11:45:28 EDT
java-service-wrapper is fixed in rawhide now, you should remove your workarounds.
Comment 15 gil cattaneo 2013-06-25 12:21:03 EDT
Great! thanks

Spec URL: http://gil.fedorapeople.org/apacheds-daemon.spec
SRPM URL: http://gil.fedorapeople.org/apacheds-daemon-1.1.8-2.fc18.src.rpm

- removed java-service-wrapper workarounds
Comment 16 gil cattaneo 2013-06-25 12:30:24 EDT
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=5539859
Comment 17 Mikolaj Izdebski 2013-06-25 12:47:51 EDT
Please convert the package to current packaging guidelines.  This means using %mvn_build instead of mvn-rpmbuild.  mvn-rpmbuild is a legacy feature and it shouldn't be used for new packages.
Comment 18 gil cattaneo 2013-06-26 08:30:27 EDT
Spec URL: http://gil.fedorapeople.org/apacheds-daemon.spec
SRPM URL: http://gil.fedorapeople.org/apacheds-daemon-1.1.8-2.fc18.src.rpm

- replace mvn-rpmbuild with mvn_build
Comment 20 Mikolaj Izdebski 2013-07-02 00:10:21 EDT
POM modifications just to remove warnings are missing the point.  If there is a warning in the first place then the real reson should be fixed, not symptom.  You can keep these modifications if you provide a reference to a bug report (either upsteam of Fedora).
Comment 21 gil cattaneo 2013-07-02 06:19:42 EDT
Spec URL: http://gil.fedorapeople.org/apacheds-daemon.spec
SRPM URL: http://gil.fedorapeople.org/apacheds-daemon-1.1.8-2.fc19.src.rpm

- removed POM modifications
Comment 22 Christopher Meng 2013-09-05 09:51:32 EDT
Ping?
Comment 23 gil cattaneo 2013-09-05 13:12:34 EDT
pong?
Comment 24 Mikolaj Izdebski 2013-09-09 02:38:22 EDT
(In reply to Christopher Meng from comment #22)
> Ping?

What do you need to know?
If you want to take the review, feel free to do so.
Comment 25 gil cattaneo 2015-03-31 12:23:23 EDT
No more interested

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