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

Re: RFS: squirrel-sql



Hi Niels,

First, thanks for taking the time to make this initial review.

> Had a short look.  The uscan/watch file does not generate a tarball;
> instead it fails with:
> svn: XML parsing failed: (411 Length Required)

Tarball is generated from upstream svn tag.
Have changed protocol to https in svn url in orig-tar.sh. This should
resolve the problem.
FYI, I managed to reproduce in clean wheezy/sid chroot only by
overriding defaults for http-library in ~/.subversion/servers.

$ sudo pbuilder login
# apt-get install subversion
# svn --version
svn, version 1.6.12 (r955767)
   compiled Jan  5 2011, 22:18:21
# svn list http://squirrel-sql.svn.sourceforge.net/svnroot/squirrel-sql/tags
RELEASE-0_1/
....
# echo "http-library = serf" >> ~/.subversion/servers
# svn list http://squirrel-sql.svn.sourceforge.net/svnroot/squirrel-sql/tags
svn: XML parsing failed: (411 Length Required)

> At very least the "you may not modify the graphics" fails DFSG and have
> a feeling that its "usage" restriction might also violate DFSG.
>  You probably want to ping upstream about these.

I think these files are actually not needed in squirrel anymore. I have
contacted upstream authors about this.
So, removed these from the tarball and added +dfsg to version.

> Files:
>
app/src/main/resources/net/sourceforge/squirrel_sql/client/resources/images/eclipse/*
> License: CPL-1.0

Added IBM Corporation and others to Copyright attribute.

> Then there is:
> """
> Files:
>
fw/src/main/java/net/sourceforge/squirrel_sql/fw/completion/PopupManager.java
> What is the status here?  Personally I am not comfortable with a "it
> might be this or that license".
...
> then said license is missing from d/copyright file.

Keeping the file as-is currently. Added previously missing license to
copyright.

> Then I believe dep-5 copyright files require you to use " ." for "empty
> lines" in licenses files

Fixed in copyright.

> The "-rm -fr debian/tmp" line in the clean target is redundant; dh_clean
> (from debhelper) will take care of removing that.

Fixed in rules.

> 001-squirrelsql-jgoodies.patch contain a number of changes that are
> merely whitespace changes (usually removing an extra space at the end of
> a line etc); these are rather disturbing particularly in large patches
> as they appear often enough to distract you from the real changes.

This particular patch should resolve api compatibility problems with
jgoodies-forms version available in Debian. Mentioned whitespace changes
come from jgoodies-forms upstream code used for this patch. The
following should uncover the details.

$ svn cat
svn://svn.debian.org/svn/pkg-java/trunk/squirrel-sql/debian/patches/001-squirrelsql-jgoodies.patch>001-squirrelsql-jgoodies.patch
$ svn cat -r491
https://svn.java.net/svn/forms~svn/trunk/src/core/com/jgoodies/forms/builder/DefaultFormBuilder.java>DefaultFormBuilder.orig.java
$ svn cat
https://squirrel-sql.svn.sourceforge.net/svnroot/squirrel-sql/tags/squirrelsql-3.2.0/app/src/main/java/net/sourceforge/squirrel_sql/client/gui/builders/DefaultFormBuilder.java>DefaultFormBuilder.java
$ svn cat
https://squirrel-sql.svn.sourceforge.net/svnroot/squirrel-sql/tags/squirrelsql-3.2.0/app/src/main/java/net/sourceforge/squirrel_sql/client/gui/builders/I15dPanelBuilder.java>I15dPanelBuilder.java
$ patch < 001-squirrelsql-jgoodies.patch
patching file DefaultFormBuilder.java
patching file I15dPanelBuilder.java
$ diff DefaultFormBuilder.java DefaultFormBuilder.orig.java

Please let me know if this explanation is not enough. I think there
should be no problem to patch with minimalistic subset of changes then.

> I am not really sure what the standing on using the "List" as author of
> patches.  It seems most appropriate that you list the actual authors

Fixed in new patch headers.

Please note the package files for experimental can be also found on
mentors.debian.net now:
URL: http://mentors.debian.net/debian/pool/main/s/squirrel-sql/

-- 
Vladimir Kotov, vladimir at kotov.lv


Attachment: signature.asc
Description: OpenPGP digital signature


Reply to: