[Bug 855780] Review Request: apacheds-daemon - Reusable Daemon Framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=855780

--- Comment #10 from Mikolaj Izdebski <mizdebsk@xxxxxxxxxx> ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=h6SNYgMYrN&a=cc_unsubscribe
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]