[Pkg-javascript-devel] Review of istanbuljs packaging

[Per Andersson]

On Wed, Nov 21, 2018 at 01:30:23PM -0500, Nicolas Mora wrote:
> Hello,
> 
> Wow, thanks a lot for the very detailed review!
> 
> Most of the feedbacks are fixed or are going to be, but I have a couple of
> questions regarding some of them, and answers for your questions.

Allright, that's good.


> Le 2018-11-21 04:47, Per Andersson a écrit :
> 
> > node-async:
> > * In the commit 1f1a73863e91c7101df3c114ed5e4c967391dd24 there is more
> >   happening than just changing the .gitignore
> indeed, the problem was that the original gitignore told to ignore build and
> build-es, but there are source files in support/build that were ignored
> because of this, so in the same commit I changed the .gitinore and readded
> the missing files.
> I changed the commit message to become more explicit

Ok, but why are the files missing from the start? How does upstream
handle this?


> > * In the commit 7ef4626a5b559c52d13ed5fa57110fe8bfd77027 why does the
> >   target path need changing?
> Because the file webpack.config.js is in debian to avoid a useless patch,
> but the output files were generated under debian/dist instead of dist, so
> preferred changing the target path instead of the install file, but I can
> revert to the original version.

I myself prefer to change as little as possible in the upstream source
and do all the bits and bends in the debian/ dir.


> > node-fileset:
> > * remove override_dh_auto_build target if unused
> The problem I have is because of the Makefile present in the root dir. If I
> don't override dh_auto_build, then sbuild will run 'make' which leads to the
> command 'bake -h' which doesn't build anything, especially since there is
> nothing to build...
> 
> I couldn't find in the documentation another way to disable a dh_ command,
> so instead I overrode it with nothing, but if there's a better way, I'll
> change it asap!

No, that's all good and well. But you could comment why, be kind to your
collaborators and your future self. :-)


> > node-path-parse:
> > * You could add an autopkgtest running node test.js,
> Sorry for the rookie question but I couldn't find how to use autopkgtest
> properly for this case.
> How can I add an autopkgtest running node test.js? Simply by setting the
> dh_auto_test command to 'autopkgtest node test.js'?

You can add another shell script running executing "node test.js" in
debian/tests, see the autopkgtests README for more info [0]. Something
like the following would do the trick I think:

debian/tests/control:

    Tests: require, test-js
    Depends: node-path-parse

debian/tests/test-js:

    #!/bin/sh
    set -e
    node test.js


> > node-istanbuljs-1.x:
> > * Missing long descriptions in d/control
> The problem with istanbuljs is that the author is not very talkative on the
> descriptions of his packages, that's why I didn't put any long
> description...

It's common to write this yourself as part of package maintaining work:
descriptions, man pages etc.


> Also, as I asked on IRC, what version should I use for node-istanbuljs? The
> branch name is 1.x but the package.json files says 2.0.0 and it's probably a
> typo since 2.0 is the current release number. So how should I version this
> package? 1.x? 1? something else maybe?
> I prefer asking first instead of having to change it afterwards.

As we talked about it on IRC, something like 1+git$DATE.$HASH is common.


> Thanks again

My pleasure. Keep up the good work!


[0] https://people.debian.org/~mpitt/autopkgtest/README.package-tests.html


--
Per