On Thu, 2014-09-18 at 15:48 -0400, Harlan Lieberman-Berg wrote:
> Thank you for submitting piqi for review. I took a look at it, and it
> looks good. One quick clarification - have you send the patch upstream?
> I couldn't find mention of it in their issues or in their repo.
Further review indicates the following possible blockers from an
ftp-team perspective (I'm not on the team):
Some files have a copyright notice but no license text, I'm not sure if
it is clear that we have permission to distribute these files, please
ask upstream to clarify the situation by updating the headers.
debian/copyright is missing copyright holders for some files.
debian/copyright is missing licenses for some files, some of these
licenses (BSD) require binary distributions to convey the license text,
so it would be a violation of the license for us to ship the package in
Debian. Some of the affected files are dual licensed under the Apache
license and the other license. You also need to document the copyright
and license for the embedded code copies if they are in the orig.tar.gz.
Please review every file, I would suggest using the mc package, turn on
lynx mode and press the Ctrl+X Q key combo to get file contents viewing.
https://ftp-master.debian.org/REJECT-FAQ.html
In addition, some things that would be nice to fix/do:
It appears there are several embedded code copies, these should probably
be removed from the upstream git repository and the system versions
used, if upstream does not want to do that, please report the embedded
code copies that are used to the security team.
deps/
tests/extensions
https://wiki.debian.org/EmbeddedCodeCopies
There appear to be several generated files in the source package, I
would suggest asking upstream to remove them from their VCS and add
their names to .gitignore, generated files should always be built from
source (unless there are very good reasons not to). If upstream does not
want to do that, please remove these files in debian/rules, before
dh_auto_configure and before/after dh_auto_clean. If there are very good
reasons, please document them in debian/README.source.
piqilib/piqi_c.mli
piqilib/piqi_c.ml
piqilib/piqi_piqi.ml
piqilib/piqi_impl_piqi.ml
piqilib/piqi_boot.ml
doc/piqi.1
No need to include changelog entries in the patch description.
For patches that have been sent upstream you can use the DEP-3 Bug or
Forwarded headers to indicate where the patch has been sent.
The override_dh_auto_configure looks unnecessary, misses some configure
options usually passed by dh_auto_configure, including breaking
cross-compiling with dpkg-buildpackage -a<arch>.
Please ask upstream to build and install the manual page and docs by
default.
A couple of sections of README.md are not useful to users of the binary
package, you might want to remove those.
The vim plugin doesn't get installed.
You could de-duplicate the license info in debian/copyright like this:
Files: *
Copyright: 2010-2014 Anton Lavrik <alavrik@piqi.org>
License: Apache-2.0
Files: debian/*
Copyright: 2014 Matthew Maurer <maurer@matthewmaurer.org>
2013 Motiejus Jakštys <desired.mta@gmail.com>
License: Apache-2.0
License: Apache-2.0
<license text>
Automated checks:
$ lintian
P: piqi source: debian-watch-may-check-gpg-signature
$ codespell --quiet-level=3
./CHANGES:23: Compatiblity ==> Compatibility
<lots, most are probably false positives>
$ fdupes -q -r .
./examples/empty.piq
./examples/empty.piqi
./tests/pp/t.piq.pp-expand-abbr
./tests/pp/t.piq.pp-expand-abbr-parse-literals
./piqilib/piqi_c.ml
./piqilib/piqi_c.mli
$ find \( -iname \*.c -o -iname \*.cxx -o -iname \*.cpp -o -iname \*.cc -o -iname \*.h -o -iname \*.hpp -o -iname \*.hh -o -iname \*.hxx \) -print0 | xargs --no-run-if-empty --null -n1 include-what-you-use
piqilib/piqi_c_impl.c should add these lines:
#include <caml/compatibility.h> // for copy_int64
#include <caml/config.h> // for int64
piqilib/piqi_c_impl.c should remove these lines:
- #include <caml/alloc.h> // lines 44-44
- #include <caml/callback.h> // lines 46-46
- #include <caml/fail.h> // lines 6-6
- #include <caml/memory.h> // lines 43-43
The full include-list for piqilib/piqi_c_impl.c:
#include <caml/compatibility.h> // for copy_int64
#include <caml/config.h> // for int64
#include <caml/fail.h> // for caml_failwith
#include <caml/mlvalues.h> // for value, String_val
#include <errno.h> // for errno
#include <stdint.h> // for uint64_t
#include <stdio.h> // for NULL
#include <stdlib.h> // for strtoull
---
tests/cpp/test.cpp should add these lines:
#include <bits/fcntl-linux.h> // for O_RDONLY
#include <string> // for operator<<, string
tests/cpp/test.cpp should remove these lines:
- #include <stdlib.h> // lines 4-4
- #include <sys/stat.h> // lines 2-2
- #include <sys/types.h> // lines 1-1
The full include-list for tests/cpp/test.cpp:
#include <bits/fcntl-linux.h> // for O_RDONLY
#include <fcntl.h> // for open
#include <google/protobuf/text_format.h> // for TextFormat
#include <unistd.h> // for close
#include <iostream> // for cout, ostream
#include <string> // for operator<<, string
#include "piqi.piqi.pb.h" // for piqi
---
./tests/cpp/piqi.piqi.pb.h:7:10: fatal error: 'string' file not found
#include <string>
^
tests/cpp/piqi.piqi.pb.h should add these lines:
#include <assert.h> // for assert
#include <stddef.h> // for NULL
tests/cpp/piqi.piqi.pb.h should remove these lines:
- #include <google/protobuf/extension_set.h> // lines 25-25
- #include <google/protobuf/generated_enum_reflection.h> // lines 26-26
- #include <google/protobuf/generated_message_util.h> // lines 22-22
- #include <google/protobuf/message.h> // lines 23-23
- #include <google/protobuf/repeated_field.h> // lines 24-24
- #include <google/protobuf/unknown_field_set.h> // lines 27-27
The full include-list for tests/cpp/piqi.piqi.pb.h:
#include <assert.h> // for assert
#include <google/protobuf/stubs/common.h>
#include <stddef.h> // for NULL
---
In file included from ./tests/cpp/piqi.piqi.pb.cc:11:
In file included from /usr/include/google/protobuf/io/coded_stream.h:124:
In file included from /usr/include/x86_64-linux-gnu/sys/param.h:26:
/usr/include/limits.h:123:16: fatal error: 'limits.h' file not found
# include_next <limits.h>
^
tests/cpp/piqi.piqi.pb.h should add these lines:
#include <assert.h> // for assert
#include <stddef.h> // for size_t, NULL
namespace google { namespace protobuf { class Descriptor; } }
namespace google { namespace protobuf { class EnumDescriptor; } }
namespace google { namespace protobuf { namespace io { class CodedInputStream; } } }
namespace google { namespace protobuf { namespace io { class CodedOutputStream; } } }
tests/cpp/piqi.piqi.pb.h should remove these lines:
- #include <google/protobuf/extension_set.h> // lines 25-25
- namespace piqi_org { namespace piqi { class piq_format; } } // lines 38-38
- namespace piqi_org { namespace piqi { class piqi; } } // lines 47-47
- namespace piqi_org { namespace piqi { class piqi_list; } } // lines 51-51
- namespace piqi_org { namespace piqi { class piqi_typedef; } } // lines 39-39
The full include-list for tests/cpp/piqi.piqi.pb.h:
#include <assert.h> // for assert
#include <google/protobuf/generated_enum_reflection.h> // for NameOfEnum, etc
#include <google/protobuf/generated_message_util.h>
#include <google/protobuf/message.h> // for Metadata, Message
#include <google/protobuf/repeated_field.h> // for RepeatedPtrField
#include <google/protobuf/stubs/common.h> // for uint32, uint8, int32, etc
#include <google/protobuf/unknown_field_set.h> // for UnknownFieldSet
#include <stddef.h> // for size_t, NULL
#include <string> // for string, basic_string
namespace google { namespace protobuf { class Descriptor; } }
namespace google { namespace protobuf { class EnumDescriptor; } }
namespace google { namespace protobuf { namespace io { class CodedInputStream; } } }
namespace google { namespace protobuf { namespace io { class CodedOutputStream; } } }
namespace piqi_org { namespace piqi { class alias; } } // lines 45-45
namespace piqi_org { namespace piqi { class any; } } // lines 49-49
namespace piqi_org { namespace piqi { class field; } } // lines 41-41
namespace piqi_org { namespace piqi { class function; } } // lines 50-50
namespace piqi_org { namespace piqi { class import; } } // lines 48-48
namespace piqi_org { namespace piqi { class list; } } // lines 46-46
namespace piqi_org { namespace piqi { class option; } } // lines 43-43
namespace piqi_org { namespace piqi { class piqi_enum; } } // lines 44-44
namespace piqi_org { namespace piqi { class record; } } // lines 40-40
namespace piqi_org { namespace piqi { class variant; } } // lines 42-42
---
tests/cpp/piqi.piqi.pb.cc should add these lines:
#include <google/protobuf/wire_format_lite.h> // for WireFormatLite, etc
#include <string.h> // for NULL, memset
#include <utility> // for pair
tests/cpp/piqi.piqi.pb.cc should remove these lines:
The full include-list for tests/cpp/piqi.piqi.pb.cc:
#include "piqi.piqi.pb.h"
#include <google/protobuf/descriptor.h> // for Descriptor (ptr only), etc
#include <google/protobuf/generated_message_reflection.h>
#include <google/protobuf/io/coded_stream.h> // for CodedInputStream, etc
#include <google/protobuf/reflection_ops.h> // for ReflectionOps
#include <google/protobuf/stubs/common.h> // for uint32, uint8, etc
#include <google/protobuf/stubs/once.h> // for GoogleOnceInit, etc
#include <google/protobuf/wire_format.h> // for WireFormat, etc
#include <google/protobuf/wire_format_lite.h> // for WireFormatLite, etc
#include <google/protobuf/wire_format_lite_inl.h>
#include <string.h> // for NULL, memset
#include <algorithm> // for swap
#include <utility> // for pair
---
--
bye,
pabs
https://wiki.debian.org/PaulWise
Attachment:
signature.asc
Description: This is a digitally signed message part