RFS: libinfluxdb-http-perl 0.04 (new package)

[Gabriel Filion]

Hello,

I've created a new package for InfluxDB::HTTP as the
libinfluxdb-http-perl package and I don't have DM or DD access. I think
that it should be mostly ready for review.

https://salsa.debian.org/perl-team/modules/packages/libinfluxdb-http-perl

Can I gently ask for some help with reviewing my work on this package?


One detail that I left out: there's a t/ directory with one file that
contains tests. I've tried to add build-deps for running the test suite
but I ended up needing another library, Object::Result, which is not
packaged in Debian and I didn't want to fall into a rabbit hole. So for
now the package does not run tests with autopkgtest.

Cheers!

[gregor herrmann]

On Sun, 19 Jun 2022 16:13:00 -0400, Gabriel Filion wrote:

> I've created a new package for InfluxDB::HTTP as the libinfluxdb-http-perl
> package and I don't have DM or DD access. I think that it should be mostly
> ready for review.
> https://salsa.debian.org/perl-team/modules/packages/libinfluxdb-http-perl

Nice to see your first package coming along :)

> Can I gently ask for some help with reviewing my work on this package?

Some notes:
- I wouldn't install the README.pod as it is the same as the POD (and
the manpage)
% diff -u <(pod2text README.pod) <(pod2text lib/InfluxDB/HTTP.pm)
`perldoc InfluxDB::HTTP' and `man InfluxDB::HTTP' should be enough
:)
- W: libinfluxdb-http-perl source: no-nmu-in-changelog
W: libinfluxdb-http-perl source: source-nmu-has-incorrect-version-number 0.04-1
That's because you use a different mail adress in d/control and
d/changelog.
- I: libinfluxdb-http-perl: synopsis-is-a-sentence "Perl way to interact with InfluxDB."
--> remove full stop at the end
- I: libinfluxdb-http-perl: typo-in-manual-page usr/share/man/man3/InfluxDB::HTTP.3pm.gz line 151 reponse response
Upstream might be happy about a patch :)

> One detail that I left out: there's a t/ directory with one file that
> contains tests. I've tried to add build-deps for running the test suite but
> I ended up needing another library, Object::Result, which is not packaged in
> Debian and I didn't want to fall into a rabbit hole. So for now the package
> does not run tests with autopkgtest.

Now that is interesting; typically we have t/*.t or the old test.pl,
but t/test.pl is something, hm, unusual.
And it's not run during build (and would probably need a running
influxd …)

So you can also remove the libtest-simple-perl build dependency
(coming from META.*; not that this changes anything, as it's provided
by src:perl).

As for autopkgtests, I'd still add the
"Testsuite: autopkgtest-pkg-perl"
field, as there are more tests than running the smoke tests.

And they work. "Work" not in "pass" but in the sense of "find an issue" :)

# Can't locate JSON/MaybeXS.pm in @INC (you may need to install the JSON::MaybeXS module) (@INC contains: /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.34.0 /usr/local/share/perl/5.34.0 /usr/lib/x86_64-linux-gnu/perl5/5.34 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.34 /usr/share/perl/5.34 /usr/local/lib/site_perl) at /usr/share/perl5/InfluxDB/HTTP.pm line 13.

So libjson-maybexs-perl is missing in Depends (and the upstream
metadata).

Next one:
# Can't locate LWP/UserAgent.pm in @INC (you may need to install the LWP::UserAgent module) (@INC contains: /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.34.0 /usr/local/share/perl/5.34.0 /usr/lib/x86_64-linux-gnu/perl5/5.34 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.34 /usr/share/perl/5.34 /usr/local/lib/site_perl) at /usr/share/perl5/InfluxDB/HTTP.pm line 14.

So let's add libwww-perl to Depends.

And let's look at the actual code:

use JSON::MaybeXS;
use LWP::UserAgent;
use Object::Result;
use URI;

We have liburi-perl but *drumroll* we're back to packaging Object::Result
which you wanted to avoid :)

I added a note to d/changelog.


Some more details:
* I've added a debian/upstream/metadata file (dh-make-perl didn't find
the github repo but lintian-brush did).
* d/control:
Maintainer: Debian Perl Group <pkg-perl-m...@lists.alioth.debian.org>
Uploader: Gabriel …
Vcs-Browser: https://salsa.debian.org/perl-team/modules/packages/libinfluxdb-http-perl
Vcs-Git: https://salsa.debian.org/perl-team/modules/packages/libinfluxdb-http-perl.git
(dh-make-perl has a --pkg-perl option)


Cheers,
gregor

--
.''`. https://info.comodo.priv.at -- Debian Developer https://www.debian.org
: :' : OpenPGP fingerprint D1E1 316E 93A7 60A8 104D 85FA BB3A 6801 8649 AA06
`. `' Member VIBE!AT & SPI Inc. -- Supporter Free Software Foundation Europe
`-

[Gabriel Filion]

Hi,

I'm finally reaching out to this small project

Thanks again Gregor for the insightful feedback you sent me back in june!

I've sent commits to implement the changes that were suggested:

On 2022-06-20 17:04, gregor herrmann wrote:
> On Sun, 19 Jun 2022 16:13:00 -0400, Gabriel Filion wrote:

>> https://salsa.debian.org/perl-team/modules/packages/libinfluxdb-http-perl

> Some notes:
> - I wouldn't install the README.pod as it is the same as the POD (and
> the manpage)
> % diff -u <(pod2text README.pod) <(pod2text lib/InfluxDB/HTTP.pm)
> `perldoc InfluxDB::HTTP' and `man InfluxDB::HTTP' should be enough
> :)

right, duplication is not great. done!

> - W: libinfluxdb-http-perl source: no-nmu-in-changelog
> W: libinfluxdb-http-perl source: source-nmu-has-incorrect-version-number 0.04-1
> That's because you use a different mail adress in d/control and
> d/changelog.

oh, fixed. I'm currently changing jobs anyway so I should use another
email address if I want to continue maintaining the package with the team.

> - I: libinfluxdb-http-perl: synopsis-is-a-sentence "Perl way to interact with InfluxDB."
> --> remove full stop at the end

done

> - I: libinfluxdb-http-perl: typo-in-manual-page usr/share/man/man3/InfluxDB::HTTP.3pm.gz line 151 reponse response
> Upstream might be happy about a patch :)

I've sent a PR upstream to fix this typo.

> So you can also remove the libtest-simple-perl build dependency
> (coming from META.*; not that this changes anything, as it's provided
> by src:perl).

ok, the dependency is removed.

> As for autopkgtests, I'd still add the
> "Testsuite: autopkgtest-pkg-perl"
> field, as there are more tests than running the smoke tests.

ah yes ok, so this will run some basic perl smoke tests I guess?

I've added this field to the control file.

> # Can't locate JSON/MaybeXS.pm in @INC (you may need to install the JSON::MaybeXS module) (@INC contains: /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.34.0 /usr/local/share/perl/5.34.0 /usr/lib/x86_64-linux-gnu/perl5/5.34 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.34 /usr/share/perl/5.34 /usr/local/lib/site_perl) at /usr/share/perl5/InfluxDB/HTTP.pm line 13.
>
> So libjson-maybexs-perl is missing in Depends (and the upstream
> metadata).
>
> Next one:
> # Can't locate LWP/UserAgent.pm in @INC (you may need to install the LWP::UserAgent module) (@INC contains: /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.34.0 /usr/local/share/perl/5.34.0 /usr/lib/x86_64-linux-gnu/perl5/5.34 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.34 /usr/share/perl/5.34 /usr/local/lib/site_perl) at /usr/share/perl5/InfluxDB/HTTP.pm line 14.
>
> So let's add libwww-perl to Depends.

added both

> And let's look at the actual code:
>
> use JSON::MaybeXS;
> use LWP::UserAgent;
> use Object::Result;
> use URI;
>
> We have liburi-perl but *drumroll* we're back to packaging Object::Result
> which you wanted to avoid :)

oh blah you're totally right. the Object::Result is used at runtime in
the library, so there's no escaping packaging it!

I took a quick look at the code from Object::Result and all dependencies
are already packaged in debian.

I'll try and create another new package for that second lib soon.

[gregor herrmann]

On Tue, 16 Aug 2022 02:40:37 -0400, Gabriel Filion wrote:

> I'm finally reaching out to this small project

Yay :)

> Thanks again Gregor for the insightful feedback you sent me back in june!

You're welcome.

> > - I: libinfluxdb-http-perl: typo-in-manual-page usr/share/man/man3/InfluxDB::HTTP.3pm.gz line 151 reponse response
> > Upstream might be happy about a patch :)
> I've sent a PR upstream to fix this typo.

Great.

> > So you can also remove the libtest-simple-perl build dependency
> > (coming from META.*; not that this changes anything, as it's provided
> > by src:perl).
> ok, the dependency is removed.

(Looking at the commit message: It's more that it's not used than
that Test::More is also in src:perl; the latter just means that the
separate libtest-simple-perl wouldn't have been installed as it's
also Provided.)

> > As for autopkgtests, I'd still add the
> > "Testsuite: autopkgtest-pkg-perl"
> > field, as there are more tests than running the smoke tests.
> ah yes ok, so this will run some basic perl smoke tests I guess?

See https://perl-team.pages.debian.net/autopkgtest.html for a
description of the tests.
(In this unusual case smoke.t is not run, as there is no t/*.t and no
test.pl, but both use.t and syntax.t caught the missing
dependencies.)

> > # Can't locate JSON/MaybeXS.pm in @INC (you may need to install the JSON::MaybeXS module) (@INC contains: /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.34.0 /usr/local/share/perl/5.34.0 /usr/lib/x86_64-linux-gnu/perl5/5.34 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.34 /usr/share/perl/5.34 /usr/local/lib/site_perl) at /usr/share/perl5/InfluxDB/HTTP.pm line 13.

> > # Can't locate LWP/UserAgent.pm in @INC (you may need to install the LWP::UserAgent module) (@INC contains: /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.34.0 /usr/local/share/perl/5.34.0 /usr/lib/x86_64-linux-gnu/perl5/5.34 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.34 /usr/share/perl/5.34 /usr/local/lib/site_perl) at /usr/share/perl5/InfluxDB/HTTP.pm line 14.

> added both

Cool.

> > And let's look at the actual code:
> >
> > use JSON::MaybeXS;
> > use LWP::UserAgent;
> > use Object::Result;
> > use URI;
> >
> > We have liburi-perl but *drumroll* we're back to packaging Object::Result
> > which you wanted to avoid :)

liburi-perl is still missing in Depends.
Well, and the not-yet-packaged libobject-result-perl as well.

> oh blah you're totally right. the Object::Result is used at runtime in the
> library, so there's no escaping packaging it!

Ack.

> I took a quick look at the code from Object::Result and all dependencies are
> already packaged in debian.
> I'll try and create another new package for that second lib soon.

Just shout when it's in git :)

[Gabriel Filion]

Hello again Perl Team,

I'm finally coming back to this during a local meetup. From what I could
see there are mostly two things that are still needed:
1) missing dependency -- that's now in debian!
2) configuring autopkgtest. that seems more difficult (more below)

On 2022-08-16 05:03, gregor herrmann wrote:
>>> And let's look at the actual code:
>>>
>>> use JSON::MaybeXS;
>>> use LWP::UserAgent;
>>> use Object::Result;
>>> use URI;
>>>

>>> We have liburi-perl but*drumroll* we're back to packaging Object::Result
>>> which you wanted to avoid 🙂

> liburi-perl is still missing in Depends.
> Well, and the not-yet-packaged libobject-result-perl as well.
>
>> oh blah you're totally right. the Object::Result is used at runtime in the
>> library, so there's no escaping packaging it!
> Ack.
>
>> I took a quick look at the code from Object::Result and all dependencies are
>> already packaged in debian.
>> I'll try and create another new package for that second lib soon.

> Just shout when it's in git 🙂

libobject-result-perl is now in the debian archive.
Both liburi-perl and libobject-result-perl are marked as dependencies in
the control file.


I have a local branch where I started to look at enabling autopkgtest,
which mostly just adds test-only dependencies to the control file..
However, when reading the source code of the file t/tests.pl I can see
that it's expecting to contact a running InfluxDB instance. There is no
perl library for testing influxdb (e.g. something like Test::InfluxDB)
that I can see that would make this possible to rewire.
I honestly don't know how to get InfluxDB running.. I'm only packaging
libinfluxdb-http-perl because it's a new dependency of the smokeping
package.

So I'm wondering if getting libinfluxdb-http-perl into the archive
without autopkgtest would be an option. If I can get some help it would
still be possible to enable those tests later.


cheers

[gregor herrmann]

On Sun, 01 Oct 2023 14:45:08 -0400, Gabriel Filion wrote:

> I have a local branch where I started to look at enabling autopkgtest, which
> mostly just adds test-only dependencies to the control file.. However, when
> reading the source code of the file t/tests.pl I can see that it's expecting
> to contact a running InfluxDB instance.

Some random thoughts and observations:

- autopkgtest-pkg-perl typically runs three tests; even if we can't run
the smoke test (but more on that below), having the others enabled
would be nice
- I noticed that the tests are also not run during build (as `make
test' won't run t/test.pl) which is also not so nice
- reading t/test.pl it looks like an influxdbd will be started by
the test script …

After playing a bit getting them to run during build was not
difficult in th end :)

Now for the autopkgtest's smoke test …
*scratches head*
*tries something with smoke-setup*

Oh, this works :)
(rename t/test.pl to test.pl, then it gets picked up)

Alright, pushed to an "enabletests" branch.


Side note: renaming ./t/test.pl to ./test.pl (upstream or with a
quilt patch) would probably make both the manual invocation in
d/rules and the smoke-setup trick obsolet. But they are also neither
hard to understand nor to maintain.

> So I'm wondering if getting libinfluxdb-http-perl into the archive without
> autopkgtest would be an option. If I can get some help it would still be
> possible to enable those tests later.

With my commits it seems to work, but if there are any concerns about
starting an influxdb instance during build or autopkgtests we can
also skip it. But maybe let's try and see first :)

[gregor herrmann]

On Sun, 19 Nov 2023 16:33:24 -0500, Gabriel Filion wrote:

> So I've added a patch that changes the rpc port in the test file and now I
> get a fully running build + autopkgtest process. I've also forwarded the
> patch upstream since I think it adds a bit of stability in their test suite.

Oh, cool.

> So, I think that I am ready to request again some help with reviewing the
> work for the package libinfluxdb-http-perl and then if all seems in order,
> for sponsoring the push to NEW.

Right; reviewed, made 2 super-tiny changes, and uploaded to NEW.

(PS: For the future: Please set the distribution to unstable in
d/changelog if you think a package is ready for helpful. Helps both
reviewers to avoid building twice :) and also dpt-ready-for-upload;
e.g. with `dch -r')