[Bug 1869270] Review Request: eclipse-subclipse - Subversion Eclipse plugin
https://bugzilla.redhat.com/show_bug.cgi?id=1869270 Mat Booth changed: What|Removed |Added Status|ASSIGNED|CLOSED Resolution|--- |NEXTRELEASE Last Closed||2020-08-21 10:19:55 --- Comment #6 from Mat Booth --- Built for f33/rawhide: https://koji.fedoraproject.org/koji/buildinfo?buildID=1597696 https://koji.fedoraproject.org/koji/buildinfo?buildID=1597694 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1869270] Review Request: eclipse-subclipse - Subversion Eclipse plugin
https://bugzilla.redhat.com/show_bug.cgi?id=1869270 Mat Booth changed: What|Removed |Added Link ID||Fedora Pagure ||releng/issue/9692 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1869270] Review Request: eclipse-subclipse - Subversion Eclipse plugin
https://bugzilla.redhat.com/show_bug.cgi?id=1869270
--- Comment #5 from Mat Booth ---
(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!
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1869270] Review Request: eclipse-subclipse - Subversion Eclipse plugin
https://bugzilla.redhat.com/show_bug.cgi?id=1869270
Fabio Valentini changed:
What|Removed |Added
Flags|fedora-review? |fedora-review+
--- Comment #4 from Fabio Valentini ---
> 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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1869270] Review Request: eclipse-subclipse - Subversion Eclipse plugin
https://bugzilla.redhat.com/show_bug.cgi?id=1869270 --- Comment #3 from Mat Booth --- (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 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1869270] Review Request: eclipse-subclipse - Subversion Eclipse plugin
https://bugzilla.redhat.com/show_bug.cgi?id=1869270 --- Comment #2 from Fabio Valentini --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1869270] Review Request: eclipse-subclipse - Subversion Eclipse plugin
https://bugzilla.redhat.com/show_bug.cgi?id=1869270 Fabio Valentini changed: What|Removed |Added Status|NEW |ASSIGNED CC||decatho...@gmail.com Assignee|nob...@fedoraproject.org|decatho...@gmail.com Flags||fedora-review? --- Comment #1 from Fabio Valentini --- Taking this one too. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
