This review is not yet ready.

Ladies and gentleman, we're ready!

Changes:

- Added provides and obsoletes for jboss-as
- Fixed requires
- Fixed symlinks
- Cleaned up the spec file

Spec URL: http://goldmann.fedorapeople.org/package_review/wildfly/2/wildfly.spec
SRPM URL: http://goldmann.fedorapeople.org/package_review/wildfly/2/wildfly-8.0.0-0.2.Alpha3.fc19.src.rpm

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5797986

Please note that this is a re-review after package name change (from jboss-as).

Spec URL: http://goldmann.fedorapeople.org/package_review/wildfly/3/wildfly.spec
SRPM URL: http://goldmann.fedorapeople.org/package_review/wildfly/3/wildfly-8.0.0-0.3.Alpha3.fc19.src.rpm

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5810923

Changes:

- Using xmvn-subst for 90% of symlinks

Patches look OK (sanity check only).

Problem 1:
wildfly uses jboss-as user (as explained in a comment in the spec),
but that user points to jboss-as home, not wildfly.
You should file a bug against setup package to update
/usr/share/doc/setup/uidgid (or contact ovasik directly).

Problem 2:
It is a good idea to check if all JARs were replaced by symlinks
and faild the build if not. Not doing so may have a secutity
implications. For example if some JAR is not replaced with symlink
and it has a security bug, updating the dependency wouldn't be enough
-- wildfly would be left with old, vulnerable version of dependency.

Problem 3:
%preun scriplet calls "rm -rf" without checking what is being removed.
Users could theoretically replace these symlinks with directory and put
some data there.  Uninstalling wildfly could cause data loss.

Problem 4:
Some directories are created as linux-x86_64 and linux-i686.
What about ARM? it's also a primary architecture.

Problem 5:
Why there is strict requirement on JDK7 (java-1.7.0-openjdk-devel)?
We also have JDK8 in Fedora, which could be used to run Wildfly.
IMO change this to java-devel or java-devel >= 1:1.7

Problem 6:
Documentation should be installed into /usr/share/doc/wildfly,
not /usr/share/wildfly/docs.  Files like copyright.txt, LICENSE.txt,
README.txt should also be installed into /usr/share/doc/wildfly
(unless they are needed at runtime, which I doubt).

Problem 7:
Configuration files should be marked as %config(noreplace) or %config
and put in /etc.  This includes appclient/configuration,
domain/configuration, standalone/configuration, bin/jboss-cli.xml

Problem 8:
I think that some directories form /usr/share/wildfly should be symlinked to /var/lib/..., for example /usr/share/wildfly/standalone/data ->
/var/lib/wildfly/standalone/data

Problem 9:
Documentation takes 4.8 MiB of disk space.  It is big enough to be
moved to separate wildfly-doc subpackage.

Problem 10:
There are multiple dangling symlinks. Some dependencies are probably
broken. I suggest installing wildfly in minimal chroot
(with yum --installroot) and checking dangling symlinks.

rpmlint:
  binary package: 25 rpmlint errors, 317 warnings
  SRPM: 22 warnings

I'm not postring full rpmlint outout yet -- it's too long.
There are certainly some false-positives, but there are multiple
justified warnings and errors, which should be fixed.

(In reply to Mikolaj Izdebski from comment #4)
> Patches look OK (sanity check only).
> 
> Problem 1:
> wildfly uses jboss-as user (as explained in a comment in the spec),
> but that user points to jboss-as home, not wildfly.
> You should file a bug against setup package to update
> /usr/share/doc/setup/uidgid (or contact ovasik directly).

I contacted ovasik.

> Problem 2:
> It is a good idea to check if all JARs were replaced by symlinks
> and faild the build if not. Not doing so may have a secutity
> implications. For example if some JAR is not replaced with symlink
> and it has a security bug, updating the dependency wouldn't be enough
> -- wildfly would be left with old, vulnerable version of dependency.

I'm removing the jars that were not replaced by symlinking later in the spec file. After that I'm manually linking these exceptions. After the build I check if there are some missing symlinks or if the symlinks are broken.

> Problem 3:
> %preun scriplet calls "rm -rf" without checking what is being removed.
> Users could theoretically replace these symlinks with directory and put
> some data there.  Uninstalling wildfly could cause data loss.

Is this an issue? This directory is created by the wildfly package and shouldn't be used by anyone else, never. If you have a better idea how could I symlink it - please share it with me.

> Problem 4:
> Some directories are created as linux-x86_64 and linux-i686.
> What about ARM? it's also a primary architecture.

Upstream does not ship any ARM binaries. These directories exist in the upstream binary package:

linux-i686/    linux-x86_64/  macosx-i686/   macosx-x86_64/ win-i686/      win-x86_64/

I'm not sure if upstream even considered running on ARM. I need to consirm it and find the proper directory structure if it should be linked on ARM too.

> Problem 5:
> Why there is strict requirement on JDK7 (java-1.7.0-openjdk-devel)?
> We also have JDK8 in Fedora, which could be used to run Wildfly.
> IMO change this to java-devel or java-devel >= 1:1.7

I'll change it.

> Problem 6:
> Documentation should be installed into /usr/share/doc/wildfly,
> not /usr/share/wildfly/docs.  Files like copyright.txt, LICENSE.txt,
> README.txt should also be installed into /usr/share/doc/wildfly
> (unless they are needed at runtime, which I doubt).

I'll fix it.

> Problem 7:
> Configuration files should be marked as %config(noreplace) or %config
> and put in /etc.  This includes appclient/configuration,
> domain/configuration, standalone/configuration, bin/jboss-cli.xml

I'll fix it.

> Problem 8:
> I think that some directories form /usr/share/wildfly should be symlinked to
> /var/lib/..., for example /usr/share/wildfly/standalone/data ->
> /var/lib/wildfly/standalone/data

I'll look at it.

> Problem 9:
> Documentation takes 4.8 MiB of disk space.  It is big enough to be
> moved to separate wildfly-doc subpackage.

I'll fix it.

> Problem 10:
> There are multiple dangling symlinks. Some dependencies are probably
> broken. I suggest installing wildfly in minimal chroot
> (with yum --installroot) and checking dangling symlinks.

Yes, I forgot to do this before I submitted the review.

> rpmlint:
>   binary package: 25 rpmlint errors, 317 warnings
>   SRPM: 22 warnings
> 
> I'm not postring full rpmlint outout yet -- it's too long.
> There are certainly some false-positives, but there are multiple
> justified warnings and errors, which should be fixed.

I'll try to fix all required stuff.

Thanks!

(In reply to Marek Goldmann from comment #5)
> > Problem 2:
> > It is a good idea to check if all JARs were replaced by symlinks
> > and faild the build if not. Not doing so may have a secutity
> > implications. For example if some JAR is not replaced with symlink
> > and it has a security bug, updating the dependency wouldn't be enough
> > -- wildfly would be left with old, vulnerable version of dependency.
> 
> I'm removing the jars that were not replaced by symlinking later in the spec
> file. After that I'm manually linking these exceptions. After the build I
> check if there are some missing symlinks or if the symlinks are broken.

Currently all JARs are properly replaced by symlinks, but in future
(for example after wildfly update or change to some other packages)
some JARs may be left not replaced.  Do you want to manually verify if JARs
are replaced properly with every rebuild of wildfly?  It's much safer
(and easier) if you just add a single line verifying if all JARs were replaced.

> > Problem 3:
> > %preun scriplet calls "rm -rf" without checking what is being removed.
> > Users could theoretically replace these symlinks with directory and put
> > some data there.  Uninstalling wildfly could cause data loss.
> 
> Is this an issue? This directory is created by the wildfly package and
> shouldn't be used by anyone else, never. If you have a better idea how could
> I symlink it - please share it with me.

Packages should not remove files not owned or created by them.
You should check if the file you want to remove is a symlink and then
try to remove it, for example:

[ -L filename ] && rm -f filename

Using wildcards or removing whole directories is best avoided.

> > Problem 4:
> > Some directories are created as linux-x86_64 and linux-i686.
> > What about ARM? it's also a primary architecture.
> 
> Upstream does not ship any ARM binaries. These directories exist in the
> upstream binary package:
> 
> linux-i686/    linux-x86_64/  macosx-i686/   macosx-x86_64/ win-i686/     
> win-x86_64/
> 
> I'm not sure if upstream even considered running on ARM. I need to consirm
> it and find the proper directory structure if it should be linked on ARM too.

Fedora packagers should make every effort to support all primary
architectures [1]. As ARM will be a primary architecture I would strongly
recommend trying to support it too, even if it's not supported by upstream.

[1] http://fedoraproject.org/wiki/Packaging:Guidelines#Architecture_Support

I fixed some issues mentioned above. This is a WIP. I'll continue in Sep, since I have vacations now :)

Changes:

- Use the wildfly user instead of jboss-as
- Created doc subpackage
- Fixed the way we remove the arch specific symlinks while uninstalling the package
- Fixed some config file locations

Spec URL: http://goldmann.fedorapeople.org/package_review/wildfly/4/wildfly.spec
SRPM URL: http://goldmann.fedorapeople.org/package_review/wildfly/4/wildfly-8.0.0-0.4.Alpha3.fc19.src.rpm

Note that WildFly is an implementation of the Java Enterprise Edition 7 specification.

I've just had a quick look at this. It FTBFS in rawhide. The following dependency packages are unavailable:
aether-connector-asynchttpclient - maybe needs to be aether-transport-http?
aether-connector-file - should be aether-transport-file probably.
jbossws-native - looks to be retired

Gerard,

Both aether-connector-file and aether-connector-file are commented out. The jbossws-native package was retired after I submitted this src.rpm. I'll look at the spec file and fix the issues Mikołaj found next week and submit a new package.

Here is the new version:

Spec URL: http://goldmann.fedorapeople.org/package_review/wildfly/5/wildfly.spec
SRPM URL: http://goldmann.fedorapeople.org/package_review/wildfly/5/wildfly-8.0.0-0.5.Alpha3.fc19.src.rpm

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5892416

A few notes:

1. The symlinks in modules/system/layers/base/org/jboss/as/web/main/lib/... are created for x86_64 and i386 arches only. This will be removed with Alpha4 and later since jbossweb was deprecated and undertow will be used as the only web server in Alhpa4+. See https://community.jboss.org/wiki/800Alpha4ReleaseNotes. The whole %post and %postun sections will be removed once I package Alpha4+.

2. I fixed the EOL issues reported by rpmlint.

3. There are no dangling symlinks after installing it in a minimal chroot. Every such issue reported in rpmlint is a false positive for this package.

4. The permission issues is a false-positive too, since these permissions are needed to run WildFly AS (security + ability to run it as a regular user from different directory)

5. I've added a line that checks if all jars were replaced by symlinks if not - it'll fail the build.

6. I removed the jbossweb-native BR

Most recent Koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5914038

Created attachment 795715 [details]
rpmlint output

I am attaching rpmlint output as it is required by package review policy.
The text is too long to be added inline in a comment.

"dangling-symlink" warnings are false positives. Checked with:
for f in `rpm -ql wildfly`; do [ -L $f -a ! -e $f ] && echo $f; done

"non-standard-uid", "non-standard-gid", "non-standard-dir-perm" and "non-readable" warnings can be ignored. Wildfly installs files owned by its own user.

"patch-not-applied" warnings are false positives. All patches are applied with git.

"devel-dependency java-devel" error is a false-positive. This dependency is correct and expected.

Other warnings (no-manual-page-for-binary, name-repeated-in-summary, dangerous-command-in-%post, dangerous-command-in-%preun, spelling-error) are justified, but the issues are minor and IMO are not review blockers, although it would be nice to fix them.

Rpmlint output with false-positives filtered out:

Checking: wildfly-8.0.0-0.5.Alpha3.fc21.noarch.rpm
          wildfly-javadoc-8.0.0-0.5.Alpha3.fc21.noarch.rpm
          wildfly-doc-8.0.0-0.5.Alpha3.fc21.noarch.rpm
          wildfly-8.0.0-0.5.Alpha3.fc21.src.rpm
wildfly.noarch: W: name-repeated-in-summary C WildFly
wildfly.noarch: W: only-non-binary-in-usr-lib
wildfly.noarch: W: no-manual-page-for-binary jboss-cli
wildfly.noarch: W: no-manual-page-for-binary wildfly-cp
wildfly.noarch: W: dangerous-command-in-%post ln
wildfly.noarch: W: dangerous-command-in-%preun rm
wildfly-javadoc.noarch: W: spelling-error Summary(en_US) Javadocs -> Java docs,\
 Java-docs, Avocados
wildfly.src: W: name-repeated-in-summary C WildFly
wildfly.src: W: strange-permission wildfly-cp.sh 0775L
wildfly.src: W: invalid-url Source0: wildfly-8.0.0.Alpha3-CLEAN.tar.xz
4 packages and 0 specfiles checked; 25 errors, 375 warnings.


Rpmlint (installed packages)
----------------------------
[root@f18 /]# rpmlint wildfly wildfly-doc wildfly-javadoc
wildfly.noarch: W: name-repeated-in-summary C WildFly
wildfly.noarch: W: only-non-binary-in-usr-lib
wildfly.noarch: W: no-manual-page-for-binary jboss-cli
wildfly.noarch: W: no-manual-page-for-binary wildfly-cp
wildfly.noarch: W: dangerous-command-in-%post ln
wildfly.noarch: W: dangerous-command-in-%preun rm
wildfly-javadoc.noarch: W: spelling-error Summary(en_US) Javadocs -> Java docs,\
 Java-docs, Avocados
3 packages and 0 specfiles checked; 25 errors, 353 warnings.

Licensing review
================

License scan in sources:

LGPLv2.1+   7025
unspecified  566
ASL-v2.0     142
GPLv2         10

There are 10 files under GPLv2 with classpath exception. This needs to be reflected in license tag.

As far as I can see the parts under LGPL are in fact LGPLv2+, so this should be reflected in  license tag.

The license tag should therefore be: "LGPLv2+ and ASL 2.0 and GPLv2 with exceptions"

Apache license text is not installed. It should be. Also NOTICE file is not installed.

The same license file /usr/share/wildfly/LICENSE.txt is owned by multiple packages. Each package should have its own license file(s) installed in /usr/share/doc/<pkgname>, where <pkgname> is name of binary package, for example wildfly or wildfly-javadoc.

Summary of licensing issues:
1) fix license tag
2) install Apache license text
3) install NOTICE file
4) let each package cary its own copy of license files

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable


===== MUST items =====

Generic:
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.
     See comment #15.
[x]: License file installed when any subpackage combination is installed.
[!]: If the package is under multiple licenses, the licensing breakdown must
     be documented in the spec.
     See comment #15.
[x]: Package requires other packages for directories it uses.
[!]: Package must own all directories that it creates.
     See below.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
[x]: 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 is included in %doc.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Java:
[x]: Packages have proper BuildRequires/Requires on jpackage-utils
[x]: Javadoc documentation files are generated and included in -javadoc
     subpackage
[x]: Javadoc subpackages should not have Requires: jpackage-utils
[x]: Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlink)

Maven:
[x]: If package contains pom.xml files install it (including depmaps) even
     when building with ant
[x]: If tests are skipped during package build explain why it was needed in a
     comment
[x]: Pom files have correct Maven mapping
[x]: Maven packages should use new style packaging
[x]: Old add_to_maven_depmap macro is not being used
[x]: Packages DOES NOT have Requires(post) and Requires(postun) on jpackage-
     utils for %update_maven_depmap macro
[x]: Package DOES NOT use %update_maven_depmap in %post/%postun
[x]: Packages use %{_mavenpomdir} instead of %{_datadir}/maven2/poms

===== SHOULD items =====

Generic:
[x]: Reviewer should test that the package builds in mock.
[x]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: SourceX tarball generation or download is documented.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[-]: Files in /run, var/run and /var/lock uses tmpfiles.d when appropriate
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

Java:
[x]: Package uses upstream build method (ant/maven/etc.)
[x]: Packages are noarch unless they use JNI


There are unowned directories:
  /var/lib/wildfly
  /var/cache/wildfly
  /etc/wildfly
  /var/log/wildfly
  /var/cache/wildfly/standalone
  /var/cache/wildfly/domain
  /etc/wildfly/appclient
  /usr/share/wildfly

There are some other minor issues mentioned in above comments which don't qualify as blockers.
After the licensing stuff and directory ownership are fixed I'll be happy to approve this package.

Thanks for the detailed review!

I hopefully fixed the issues, see below.

Spec URL: http://goldmann.fedorapeople.org/package_review/wildfly/6/wildfly.spec
SRPM URL: http://goldmann.fedorapeople.org/package_review/wildfly/6/wildfly-8.0.0-0.6.Alpha3.fc19.src.rpm

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5915241

The issues were fixed.
Package approved.

Thank you!

New Package SCM Request
=======================
Package Name: wildfly
Short Description: WildFly Application Server
Owners: goldmann
Branches: f20

Git done (by process-git-requests).

Big day! WildFly is imported and built for Fedora 20 and Rawhide.

To all involved - big thank you! Without you it wouldn't be possible.

wildfly-8.0.0-0.6.Alpha3.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/wildfly-8.0.0-0.6.Alpha3.fc20

wildfly-8.0.0-0.7.Alpha3.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/wildfly-8.0.0-0.7.Alpha3.fc20

wildfly-8.0.0-0.7.Alpha3.fc20 has been pushed to the Fedora 20 stable repository.