(Anton, there is a question below for you, that's why you are in To:))
On Fri, Mar 09, 2018 at 01:13:54AM -0600, Kurt Kremitzki wrote:
> Control: tags -1 - moreinfo
>
> Alright, I've addressed all the points brought up by you two (thanks for the
> feedback so far!)
>
> I have done a thorough check of the licenses, updated d/copyright, and found
> a few problematic Qt Commercial license files, which I reported to upstream.
> [1]
>
> But, besides the files mentioned in [1], I think everything else is ready
> for review.
>
> I've verified FreeCAD works well with this, and previously my WIP Netgen 6.2
> package worked well but is currently experiencing issues which I think are
> unrelated. The only other dependent package is deal.ii which I am not
> familiar with and will have to investigate later as part of the phase-out of
> liboce.
>
> 1. https://www.opencascade.com/content/packaging-again-debian#comment-20398
>
Hi,
here's a review...(it is not sorted by priority or like)
- d/patches
- did you check with upstream whether they would accept the patches,
especially fix-install-dir-references.diff.
- d/rules:
- override_dh_auto_configure -> the comment refers to an non-existing
file. Is the ignoring of cmake's return value still needed?
- see below for dh_doxygen.
- d/control
- there is a missing B-D on graphviz, otherwise doxygen will fail
- (bonus area:) It would be great if doxygen could be put to
B-D-indep, so only installed then building the -all packages
I did not check how much effort this is, it is not required for an
upload, but as doxygen has a huge dependency chain, this is nice for
the buildds.
- for the doxygen cleanup, prefer using dh_doxygen, and you should
do that in a override for dh_installdocs(1)
- d/changelog
- the line with dbgsym can go, as those are automatic and did not
require a change in the sources.
- d/changelog.gz / changelog.html.gz
I'm not sure if we should ship those in the debian directory.
But first the technical things:
One copy should be enough, do not ship both html and text. (I would prefer
the text version) Especially, as the html version has problems: it
sources files from external websites (css, google-analytics) which is a
breach of privacy and not acceptable for Debian. Then, if you ship
them as changelogs, they are not to be installed using *.docs, they
should be installed using dh_installchangelogs(1), because this tool
will "do the right thing" and install it into every package, which is
require by the Policy*. You should not compress the changelogs in the
debian directory, the tool mentioned above will do that for you.
(* I'm omitting here the other possibility to use
symlinks between packages, which could be used to deuplicate them, IMHO
not worth the effort)
Said, that I'm not sure if we should the upstream changelog in the
debian directory; It will be an easy error to make to miss updating it
as it will always be manual. If you want to keep it, this would be
needed to be documented in README.Source, and we have many packages
without upstream changelogs, so don't let you get nuts by this
linitian messages ;) It would be better to ask upstream to put the
changelog into their releases tarballs (which then needs to be NOT
behind a login, IIRC)
I leave it up to you whether you want to have the changelog manually
or if you want to ditch it completly. (if you keep it, the fixes
described above will be needed)
- lintian overrides
- usually overrides should only be done if there is a problem with
linitian detecting the correct things, IOW it should not be
used to "silence" things, e.g
If upstream does not support something, just keep the message...
(e.g no-upstream changelog, testsuite-autopkgtest-missing,
debian-watch-does-not-check-gpg-signature)
E.g those overrides should be removed:
- source-contains-autogenerated-visual-c++-file
- no-upstream-changelog
- spelling-errors-in-binary overrides
Note hat this is about binaries, not about comments in the source
code! (as your override comment suggests)
At least a few of them are legitimate, at least the random spot
checks I did on. Disclaimer: I did not check context if this would
change code behaviour. Needs to be checked with upstream
I egrep'ed on:
- tranformations
- convertion
- unkown
I'd recommend to get in touch with upstream, maybe providing them a
patch to apply. (This will not be required for the upload, but
spelling should be fixed at least long term)
- script-not-executeable
You writd in the override:# /usr/share/occt/bin/*.sh are reference scripts
Can you expand what you mean? Are they examples? Are they
needed? For what are they needed?
One of the scripts references DRAWEXE which is listed
in d/non-installed
- symbol files
I think you should run the symbols through c++filt, see
dpkg-gensymbols(1) and https://wiki.debian.org/UsingSymbolsFiles
(example
https://salsa.debian.org/multimedia-team/zita-convolver/blob/master/debian/libzita-convolver3.symbols
This will make them more stable on archs with slightly different name
mangling...
(Just a note: getting the symblos fine on all archs will like need
more than one iteration.. so don't be worried if we get symbol errors
in the beginning)
libocct-modeling-data-7.2 seems to export those symbols:
Version2@Base 7.2.0
Version@Base 7.2.0
Version_1@Base 7.2.0
Version_2@Base 7.2.0
Version_3@Base 7.2.0
I guess this is not intended, otherwise the probability to collide
with something else is very high... (I did not check the source on
this)
Same for libocct-ocaf-7.2 -- PLUGINFACTORY.
I guess that should be at least in some namespace.
(Please analyze and if neccessary remove those symbols by hand from
the symbol files at least for the time being)
- test suite
You write in the lintian override that runs only under Windows with a
custom occt-draw.. Can you elaborate? What is the difference to the
occt-draw we ship?
- the man page should be sent upstream and ask them to include it.
So it will benefit non-Debian distributions as well.
- occt-doc
maybe better libocct-doc to make it more clear that this is the
documentation for the libraries?
- the xpm images...
Where are they from? What is the copyright?
- (This is more a question to Anton as he suggested it:)
Anton, why did you suggest experimental? There shouldn't be a
collosion to the existing oce packages, with the exception of the
-dev packages they are co-installable.
IMHO we could go directly to sid...
What have I missed?
- d/copyright
still missing a Files: section for debian/*
- in the repository: delete those stray *.substvars files, they are
build results.
OK, that's for now.. I'm quite impressed about the good quality of the
package, it is not to far away from being uploadable...
What is still missing from my side is a complete d/copyright review,
but I need a break right now... Will continue later.
PS: I plan to send pull requests later to fix a bit of the above...
(As I offered already co-maintainance :), but I think the copyright
review is more important right now on my side, so don't hesitate to
start fixing!
--
tobi
