Merge ~bryce/ubuntu/+source/fetchmail:fix-2035-hirsute into ubuntu/+source/fetchmail:ubuntu/devel

I've rebased this MP on the new fetchmail that sync'd in recently.

The new test you added to call "make check" is not working for me locally:

autopkgtest [15:36:23]: test command1: make check
autopkgtest [15:36:23]: test command1: [-----------------------
make: *** No rule to make target 'check'. Stop.
autopkgtest [15:36:24]: test command1: -----------------------]
autopkgtest [15:36:24]: test command1: - - - - - - - - - - results - - - - - - - - - -
command1 FAIL non-zero exit status 2

The "operation" test and the others are working fine:

autopkgtest [15:39:20]: @@@@@@@@@@@@@@@@@@@@ summary
command1 FAIL non-zero exit status 2
installation PASS
service PASS
operation PASS

I am not a Python expert but the code looks good. Good job! The only thing I might add is that I'd use also "set -x" in the shell script to print the commands being executed, they can be handy when debugging failures.

Moreover, I'd strongly recommend to forward the "operation" test to Debian.

Thank you for the review feedback!

I've noted your two suggestions, to add set -x and the issue with the make check. Both quite sensible review feedback, but both are causing some weird stuff I don't quite understand.

Easier piece first. The weirdness is that 'make check' works fine for me. But, the solution almost certainly is to run ./configure first. This causes no issue on my end so I've added it and pushed to this MP. Please re-test and verify it works for you. I'm fine with pushing this, but kinda would like to understand why it's needed.

Harder piece is that adding 'set -x' should be a completely cosmetic change, yet it results in autopkgtest failing for me with it set. I've pushed the changes with this set, and would appreciate if you would run the tests and see if it also fails for you, and then if it does, if you have ideas what's causing it? If you're not sure, I'd like to propose chalking it up to some sort of bash bug, punt, and omit 'set -x' for now?

Without having seen or tested this, 'set -x' outputs to stderr, so you will need the allow-stderr restriction on the test.

Hi Bryce,

Awesome work with this MP!

As for the "make check" part, it still doesn't work for me. I've pasted the logs below.
Simplest way to reproduce is to run sbuild and then autopkgtest via:
`autopkgtest -U -s --apt-pocket=proposed -B ../*.deb -- lxd ubuntu-daily:devel`

8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<
checking for gcc... no
checking for cc... no
checking for cl.exe... no
configure: error: in `/tmp/autopkgtest.8xasIM/build.tnJ/real-tree':
configure: error: no acceptable C compiler found in $PATH
See `config.log' for more details
autopkgtest [14:31:35]: test command1: -----------------------]
autopkgtest [14:31:36]: test command1: - - - - - - - - - - results - - - - - - - - - -
command1 FAIL non-zero exit status 1
8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<

I started to take a look at it and after installing gcc, here's where it fails[1]: https://paste.ubuntu.com/p/3hQqkD46rF/.

But I further studied the log and it shows that you also need python, python3, and build-essential packages in order to ./configure to work.

After installing those 4 packages, the next problem I hit was:
`configure: error: SSL support enabled, but OpenSSL not found`

This means, installing libssl-dev. And then there were no errors. Yay! \o/

So the final diff that I know have, looks like:
8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<
--- a/debian/tests/control
+++ b/debian/tests/control
@@ -1,5 +1,5 @@
 Test-Command: ./configure && make check
-Depends: @
+Depends: @, build-essential, gcc, libssl-dev, python, python3
 Restrictions: allow-stderr

 Tests: installation
8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<

With this diff applied, I now have the tests passing:

8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<
make check-TESTS
make[3]: Entering directory '/tmp/autopkgtest.EaEic9/build.kRF/real-tree'
make[4]: Entering directory '/tmp/autopkgtest.EaEic9/build.kRF/real-tree'
PASS: t.smoke
SKIP: t.validate-xhtml10
SKIP: t.validate-xhtml
PASS: t.x509_name_match
PASS: t.realpath
PASS: t.tls-aux
=========================================================
Testsuite summary for fetchmail 6.4.16
=========================================================
# TOTAL: 6
# PASS: 4
# SKIP: 2
# XFAIL: 0
# FAIL: 0
# XPASS: 0
# ERROR: 0
=========================================================
make[4]: Leaving directory '/tmp/autopkgtest.EaEic9/build.kRF/real-tree'
make[3]: Leaving directory '/tmp/autopkgtest.EaEic9/build.kRF/real-tree'
make[2]: Leaving directory '/tmp/autopkgtest.EaEic9/build.kRF/real-tree'
Making check in po
make[2]: Entering directory '/tmp/autopkgtest.EaEic9/build.kRF/real-tree/po'
make[2]: Nothing to be done for 'check'.
make[2]: Leaving directory '/tmp/autopkgtest.EaEic9/build.kRF/real-tree/po'
make[1]: Leaving directory '/tmp/autopkgtest.EaEic9/build.kRF/real-tree'
autopkgtest [15:11:35]: test command1: -----------------------]
autopkgtest [15:11:35]: test command1: - - - - - - - - - - results - -
command1 PASS
8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<

-----------------------------------------------

Now, onto the next test: "operation".
R...

TBH I am not sure if we will benefit from the "make check" test considering autopkgtest is more about testing the installed version of the package. Which kind of issue we could spot with it?

About the "set -x" in the operation test, what happens is what Sergio mentioned, the commands will be printed to stderr by default. We have three options here:

1) Do nothing and leave it as-is, without printing the commands;
2) Add the allow-stderr restriction as Utkarsh mentioned above;
3) Add "exec 2>&1" to the top of the shell script to pipe everything to stdout.

I'd go with option 3 (I have done that in some of my packages in Debian), but feel free to pick whatever you think makes more sense to you.

> TBH I am not sure if we will benefit from the "make check" test
> considering autopkgtest is more about testing the installed
> version of the package. Which kind of issue we could spot with it?

My thought process here was that running the upstream testsuite is a standard practice for inclusion in autopkgtests. fetchmail has a handful of test cases, triggered by running make check, via Perl's standard testing tooling:

PASS: t.smoke
SKIP: t.validate-xhtml10
SKIP: t.validate-xhtml
PASS: t.x509_name_match
PASS: t.realpath
PASS: t.tls-aux

Admittedly, these test cases are super trivial, which is why I went ahead with creating the POP3 test. But, perhaps future development will add more tests, and in any case they seemed to pass (for me), so figured it couldn't hurt to hook them up. It doesn't look like 'make check' is triggered during the fetchmail build process, so to me it seemed appropriate to do in a DEP8.

'set -x' implies 'allow-stderr' will be needed. Duh. Yeah why was I overthinking this.

However, I would prefer not to set allow-stderr, because actually it's been quite useful in development to be strict about stderr messages, and I took efforts to deal with all "regular" stderr messages, so if more pop up I'd like to treat them as actual problems. I am guessing the same problem will exist with adding "exec 2>&1", although that does seem like a clever approach I'll try to remember for other situations.

However, I think it may be okay to just go with option #1. In the mock server, I sprinkled dbg() commands at keep points to assist in spotting where errors might occur. Most of the problems I ran into during development were in either serve() or process(), which is why they have 10 dbg()'s between the two of them.

So, to sum up, I'd like to stay strict with treating stderr content as erroneous, and just rely on being verbose with debug messages for tracing program flow.

I've pushed latest changes, please re-review.

As mentioned in the last 2 comments, I'm opting to take the make check dependencies from Utkarsh's analysis, and opting to take Lucas' #1 suggestion (drop 'set -x'). With these changes, autopkgtest passes both inside a build container (via `sudo autopkgtest ./fetchmail_6.4.16-1ubuntu1.dsc -- null`) and externally with an ephemeral container (via `autopkgtest -U -s -o dep8-fetchmail ./fetchmail_6.4.16-1ubuntu1.dsc -- lxd images:ubuntu/hirsute/amd64`)

Thanks again!

> My thought process here was that running the upstream testsuite is a standard
> practice for inclusion in autopkgtests. fetchmail has a handful of test
> cases, triggered by running make check, via Perl's standard testing tooling:

IMO running upstream more code level tests (like unit tests) makes sense when you are testing a library, where when it is installed you want it behaving well when imported by other programs. AFAICS fetchmail ships only the binaries, docs, and config files. In short, we do not care if the library is behaving as expected because there is no library, what really matters is what we ship as a .deb. That's what autopkgtest was created for, test the installed binary packages.

I am not telling you to not do this, but this is how I rationale about testing installed version of the packages.

> Admittedly, these test cases are super trivial, which is why I went ahead with
> creating the POP3 test. But, perhaps future development will add more tests,
> and in any case they seemed to pass (for me), so figured it couldn't hurt to
> hook them up. It doesn't look like 'make check' is triggered during the
> fetchmail build process, so to me it seemed appropriate to do in a DEP8.

Maybe a good idea is to submit a salsa MR to call "make check" during the build process.

I am fine with you not using "set -x", you already have some debug info as you said.

autopkgtest is happy for me now:

autopkgtest [18:51:00]: @@@@@@@@@@@@@@@@@@@@ summary
command1 PASS
installation PASS
service PASS
operation PASS

Thanks Lucas, I'll forward those changes to Debian.

$ dput ubuntu fetchmail_6.4.16-1ubuntu1_source.changes
Checking signature on .changes
gpg: /home/bryce/pkg/Fetchmail/project-c2035-fetchmail-dep8-tests/fetchmail_6.4.16-1ubuntu1_source.changes: Valid signature from E603B2578FB8F0FB
Checking signature on .dsc
gpg: /home/bryce/pkg/Fetchmail/project-c2035-fetchmail-dep8-tests/fetchmail_6.4.16-1ubuntu1.dsc: Valid signature from E603B2578FB8F0FB
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading fetchmail_6.4.16-1ubuntu1.dsc: done.
  Uploading fetchmail_6.4.16-1ubuntu1.debian.tar.xz: done.
  Uploading fetchmail_6.4.16-1ubuntu1_source.buildinfo: done.
  Uploading fetchmail_6.4.16-1ubuntu1_source.changes: done.
Successfully uploaded packages.
$ git ubuntu tag --upload
$ git push pkg upload/6.4.16-1ubuntu1
Enumerating objects: 22, done.
Counting objects: 100% (22/22), done.
Delta compression using up to 12 threads
Compressing objects: 100% (17/17), done.
Writing objects: 100% (17/17), 4.99 KiB | 1.25 MiB/s, done.
Total 17 (delta 8), reused 0 (delta 0), pack-reused 0
To ssh://git.launchpad.net/ubuntu/+source/fetchmail
 * [new tag] upload/6.4.16-1ubuntu1 -> upload/6.4.16-1ubuntu1

Looks like fetchmail isn't in salsa for some reason. I filed a bug report instead.

Awesome, works at my end as well! Great work!

Bryce,

as upstream fetchmail maintainer: thanks for writing such a test, I am considering taking it into upstream fetchmail 6.5 (Git branch legacy_6x).

One remark, writing network-based self-tests for FreeBSD's OpenVPN package I figured that hardcoding ports as done here can cause false-negative test results so I suggest to either retry with waiting and random different ports and random delay times a few times, and ultimately going for a three-state result (at least upstream I can exit 77 to "SKIP" a test within the automake testing framework).

https://gitlab.com/fetchmail/fetchmail/-/issues/32

https://gitlab.com/fetchmail/fetchmail/-/tree/legacy_6x
(not yet integrated at the time of this writing)