Taking this one too.

Same here, it's not "safe" to use macros before they are defined. You should move the %global preamble into an order (and place) such that everything is definied when it's used (i.e. move javahl_version definition above its usage in javahl_tag definition, move subclipse_tag definition below Version definition).

Possibly also move these two lines from %build into %prep to the rest of the pom.xml file modifications?

%pom_remove_plugin ":bnd-maven-plugin" base cmdline javahl svnkit
%pom_remove_plugin ":maven-jar-plugin" base cmdline javahl svnkit

There's also a lot of dangling symlinks in /usr/share/eclipse/droplets/subclipse/plugins/ that link to various JARs shipped by other packages.
Should there be explicit "Requires: foo" for the packages providing the actual JAR files? [list attached below]

You also should include the Apache-2.0 license (either from javahl or svnclientadapter) files with %license, and describe the license breakdown above the License tag, with something along the lines of:

# EPL-1.0: subclipse
# ASL 2.0: javahl and svnclientadapter
# CC-BY: ???
License: EPL-1.0 and ASL 2.0 and CC-BY


[list of dangling JAR symlinks]:

/usr/share/eclipse/droplets/subclipse/plugins/org.tigris.subversion.clientadapter
.javahl_1.11.1/lib/javahl.jar /usr/share/java/svn-javahl.jar
.svnkit_1.8.12.3/lib/antlr-runtime-3.2.jar /usr/share/java/antlr32/antlr-runtime-3.2.jar
.svnkit_1.8.12.3/lib/jna-platform.jar /usr/share/java/jna/jna-platform.jar
.svnkit_1.8.12.3/lib/jna.jar /usr/lib/java/jna/jna.jar
.svnkit_1.8.12.3/lib/jsch.agentproxy.connector-factory.jar /usr/share/java/jsch-agent-proxy/jsch.agentproxy.connector-factory.jar
.svnkit_1.8.12.3/lib/jsch.agentproxy.core.jar /usr/share/java/jsch-agent-proxy/jsch.agentproxy.core.jar
.svnkit_1.8.12.3/lib/jsch.agentproxy.pageant.jar /usr/share/java/jsch-agent-proxy/jsch.agentproxy.pageant.jar
.svnkit_1.8.12.3/lib/jsch.agentproxy.sshagent.jar /usr/share/java/jsch-agent-proxy/jsch.agentproxy.sshagent.jar
.svnkit_1.8.12.3/lib/jsch.agentproxy.svnkit-trilead-ssh2.jar /usr/share/java/jsch-agent-proxy/jsch.agentproxy.svnkit-trilead-ssh2.jar
.svnkit_1.8.12.3/lib/jsch.agentproxy.usocket-jna.jar /usr/share/java/jsch-agent-proxy/jsch.agentproxy.usocket-jna.jar
.svnkit_1.8.12.3/lib/jsch.agentproxy.usocket-nc.jar /usr/share/java/jsch-agent-proxy/jsch.agentproxy.usocket-nc.jar
.svnkit_1.8.12.3/lib/sequence-library.jar /usr/share/java/sequence-library/sequence-library.jar
.svnkit_1.8.12.3/lib/sqljet.jar /usr/share/java/sqljet.jar
.svnkit_1.8.12.3/lib/svnkit-javahl16.jar /usr/lib/java/svnkit/svnkit-javahl16.jar
.svnkit_1.8.12.3/lib/svnkit.jar /usr/share/java/svnkit/svnkit.jar
.svnkit_1.8.12.3/lib/trilead-ssh2.jar /usr/share/java/trilead-ssh2.jar

(In reply to Fabio Valentini from comment #2)
> Same here, it's not "safe" to use macros before they are defined. You should
> move the %global preamble into an order (and place) such that everything is
> definied when it's used (i.e. move javahl_version definition above its usage
> in javahl_tag definition, move subclipse_tag definition below Version
> definition).
> 

Done

> Possibly also move these two lines from %build into %prep to the rest of the
> pom.xml file modifications?
> 
> %pom_remove_plugin ":bnd-maven-plugin" base cmdline javahl svnkit
> %pom_remove_plugin ":maven-jar-plugin" base cmdline javahl svnkit
> 

Done

> There's also a lot of dangling symlinks in
> /usr/share/eclipse/droplets/subclipse/plugins/ that link to various JARs
> shipped by other packages.
> Should there be explicit "Requires: foo" for the packages providing the
> actual JAR files? [list attached below]
> 

No, the symlinks are satisfied by the generated requirement on svnkit and its transitive dependencies. These warnings are bogus IMO. Rpmlint could easily check for broken symlinks after installing the binary RPMs into a chroot, but it doesn't. For example, you can do this to make sure there are no broken symlinks:

$ mock -r fedora-33-x86_64 --enablerepo=local --init
$ mock -r fedora-33-x86_64 --enablerepo=local --install ./eclipse-subclipse-4.3.0-6.fc33.noarch.rpm
$ mock -r fedora-33-x86_64 --shell
<mock-chroot> sh-5.0# find -L /usr/share/eclipse/ -type l

(output of "find" here will be empty if all symbolic links correctly resolve)


> You also should include the Apache-2.0 license (either from javahl or
> svnclientadapter) files with %license, and describe the license breakdown
> above the License tag, with something along the lines of:
> 
> # EPL-1.0: subclipse
> # ASL 2.0: javahl and svnclientadapter
> # CC-BY: ???
> License: EPL-1.0 and ASL 2.0 and CC-BY
> 
> 

Huh, git archeaology reveals that more than 10 years ago this package used to ship the (CC-BY licensed) Subversion Book as documentation... The sub-package containing the book was obsoleted more than a decade ago and the license tag was never corrected. Amazing what a fresh pair of eyes can do for an old package...

Fixed now:

Spec URL: https://fedorapeople.org/~mbooth/reviews/eclipse-subclipse.spec
SRPM URL: https://fedorapeople.org/~mbooth/reviews/eclipse-subclipse-4.3.0-6.fc33.src.rpm

> Huh, git archeaology reveals that more than 10 years ago this package used to ship the (CC-BY licensed) Subversion Book as documentation... The sub-package containing the book was obsoleted more than a decade ago and the license tag was never corrected. Amazing what a fresh pair of eyes can do for an old package...

I do my best to unearth such tidbits :-)


Small nit-picks:

- JavaHL is not included in the latest version (1.11.1 vs. 1.14.0)
- Fix pom xml "4.2.0 modelVersion" declarations no longer necessary:

# Fix pom xml declarations
# PR sent upstream here: https://github.com/subclipse/subclipse/pull/138
sed -i -e 's/4\.2\.0/4.0.0/g' {.,features,bundles}/pom.xml

This PR has been merged, and the Change is Live in the packaged version :-)

- no Javadocs built (are they not useful? hard to build? broken? not important, but I thought I'd note their absence)


1) You can put the JavaHL update on your TODO list, and remove the no longer necessary "4.2.0 pom modelVersion" fixes before importing the package.


2) ./subclipse-4.3.0/features/feature.subclipse/licenses/Ganymed.txt: Expat License BSD 3-clause "New" or "Revised" License
Is this the license file for the Ganymed / trilead / jenkins SSH2 implementation? It's not bundled in this package, so this can be ignored. Right?


Other than that:

- latest version packaged (4.3.0) - JavaHL update pending?
- package builds and installs successfully on fedora rawhide
- license correct and break-down documented, texts shipped with %license
- appdata for plugin installed and validated
- jar files shipped with sources are removed in %prep
- %build and %install script look "interesting" but good otherwise
- correct ExcludeArch tag for Eclipse packages
- in general, conforms to Packaging Guidelines

Package APPROVED.

(In reply to Fabio Valentini from comment #4)
> > Huh, git archeaology reveals that more than 10 years ago this package used to ship the (CC-BY licensed) Subversion Book as documentation... The sub-package containing the book was obsoleted more than a decade ago and the license tag was never corrected. Amazing what a fresh pair of eyes can do for an old package...
> 
> I do my best to unearth such tidbits :-)
> 
> 
> Small nit-picks:
> 
> - JavaHL is not included in the latest version (1.11.1 vs. 1.14.0)
> - Fix pom xml "4.2.0 modelVersion" declarations no longer necessary:
> 
> # Fix pom xml declarations
> # PR sent upstream here: https://github.com/subclipse/subclipse/pull/138
> sed -i -e 's/4\.2\.0/4.0.0/g' {.,features,bundles}/pom.xml
> 
> This PR has been merged, and the Change is Live in the packaged version :-)

The PR was merged just a few hours ago (I only sent it yesterday) and is not yet available in any tagged release. The packaged version is subclipse 4.3.0 that was tagged in 2019.

> 
> - no Javadocs built (are they not useful? hard to build? broken? not
> important, but I thought I'd note their absence)
> 

No, we don't build separate javadoc subpackages for Eclipse plug-ins. In general they are not libraries against which users should build applications, and in the cases they are, Eclipse plug-ins tend to ship their javadocs as an additional plug-in bundle that extends Eclipse's built-in help system. So we trust the upstream project to ship documentation bundles as necessary.

> 
> 1) You can put the JavaHL update on your TODO list, and remove the no longer
> necessary "4.2.0 pom modelVersion" fixes before importing the package.
> 

I will investigate the javahl update. I don't recall if this just a case of "it was the latest version at the time" or if this specific version was chosen because it was a known-good version and other versions had problems (svnkit fell into this latter category).

> 
> 2) ./subclipse-4.3.0/features/feature.subclipse/licenses/Ganymed.txt: Expat
> License BSD 3-clause "New" or "Revised" License
> Is this the license file for the Ganymed / trilead / jenkins SSH2
> implementation? It's not bundled in this package, so this can be ignored.
> Right?
> 

That's right. The convention is to ship license texts for all the third-party bundles they ship in their upstream update site for their plug-ins to work. Since RPM installs directly and third-party bundles are packaged separately, this can be ignored.

> 
> Other than that:
> 
> - latest version packaged (4.3.0) - JavaHL update pending?
> - package builds and installs successfully on fedora rawhide
> - license correct and break-down documented, texts shipped with %license
> - appdata for plugin installed and validated
> - jar files shipped with sources are removed in %prep
> - %build and %install script look "interesting" but good otherwise
> - correct ExcludeArch tag for Eclipse packages
> - in general, conforms to Packaging Guidelines
> 
> Package APPROVED.


Thanks for the review!

Built for f33/rawhide:

https://koji.fedoraproject.org/koji/buildinfo?buildID=1597696
https://koji.fedoraproject.org/koji/buildinfo?buildID=1597694