Check manifest

[mattias-p]

Context

Fixes #902.

Changes

This PR adds t/manifest.t which runs make distcheck.

How to test this PR

Make sure we don't interfere with the release process:

git clean -dfx   # Start out with a clean repository
perl Makefile.PL # Generate the top-level Makefile
make all         # Generate MO files and modules.txt
make distcheck   # Verify that distcheck doesn't report any missing or unrecognized files

Run the unit tests in the source directory:

make distcheck     # Make sure distcheck doesn't report any missing or unrecognized files
prove t/manifest.t # Verify the new test passes
touch example.txt  # Create a file but don't add it to MANIFEST
prove t/manifest.t # Verify the new test fails

Run the unit tests in the dist file:

make distcheck                               # Make sure your repo is clean
make dist                                    # Create the dist file
cpanm --test-only Zonemaster-Engine-*.tar.gz # Run the tests included in the dist file

[matsduf]

After installation of Test::NoWarnings:

$ uname -a
FreeBSD fbsd-122-bygg-101 12.2-RELEASE-p1 FreeBSD 12.2-RELEASE-p1 GENERIC  amd64

$ make distcheck
"/usr/local/bin/perl" "-Iinc" "-MExtUtils::Manifest=fullcheck" -e fullcheck

$ prove t/manifest.t 
t/manifest.t .. 
    #   Failed test 'make distcheck exits with value 0'
    #   at t/manifest.t line 33.
    #          got: '2'
    #     expected: '0'

    #   Failed test 'make distcheck gives empty output'
    #   at t/manifest.t line 34.
    #          got: 'usage: make [-BeikNnqrstWwX] 
    #             [-C directory] [-D variable] [-d flags] [-f makefile]
    #             [-I directory] [-J private] [-j max_jobs] [-m directory] [-T file]
    #             [-V variable] [-v variable] [variable=value] [target ...]
    # '
    #     expected: ''
    # Looks like you failed 2 tests of 2.
t/manifest.t .. 1/2 
#   Failed test 'distcheck'
#   at t/manifest.t line 35.
# Looks like you failed 1 test of 2.
t/manifest.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/2 subtests 

Test Summary Report
-------------------
t/manifest.t (Wstat: 256 Tests: 2 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=1, Tests=2,  0 wallclock secs ( 0.01 usr  0.00 sys +  0.05 cusr  0.00 csys =  0.05 CPU)
Result: FAIL

[mattias-p]

usage: make [-BeikNnqrstWwX]

That's really weird not so weird after all.

[matsduf]

It works now under FreeBSD:

$ prove t/manifest.t 
t/manifest.t .. ok   
All tests successful.
Files=1, Tests=2,  0 wallclock secs ( 0.01 usr  0.00 sys +  0.12 cusr  0.02 csys =  0.14 CPU)
Result: PASS

[matsduf]

It also works to capture a missing file:

$ touch example.txt

$ prove t/manifest.t 
t/manifest.t .. 
    #   Failed test 'make distcheck gives empty output'
    #   at t/manifest.t line 34.
    #          got: 'Not in MANIFEST: example.txt
    # '
    #     expected: ''
    # Looks like you failed 1 test of 2.
t/manifest.t .. 1/2 
#   Failed test 'distcheck'
#   at t/manifest.t line 35.
# Looks like you failed 1 test of 2.
t/manifest.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/2 subtests 

Test Summary Report
-------------------
t/manifest.t (Wstat: 256 Tests: 2 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=1, Tests=2,  1 wallclock secs ( 0.01 usr  0.01 sys +  0.12 cusr  0.02 csys =  0.15 CPU)
Result: FAIL

[matsduf]

I made a distribution file (make dist) and then I moved it to another directory and tested with cpanm and it did not work well.

$ cpanm -v --test-only Zonemaster-Engine-v4.1.1-ada0c3d-2021-05-17.tar.gz 
cpanm (App::cpanminus) 1.7042 on perl 5.032000 built for amd64-freebsd-thread-multi
Work directory is /home/matsd/.cpanm/work/1620888902.4301
(...)
t/manifest.t .............. 
t/manifest.t .............. 1/2     #   Failed test 'make distcheck gives empty output'
    #   at t/manifest.t line 34.
    #          got: 'Not in MANIFEST: share/Zonemaster-Engine.pot
    # '
    #     expected: ''
    # Looks like you failed 1 test of 2.

#   Failed test 'distcheck'
#   at t/manifest.t line 35.
# Looks like you failed 1 test of 2.
t/manifest.t .............. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/2 subtests 
(...)

[matsduf]

I think one alternative would be to move t/manifest.t from MANIFEST to MANIFEST.SKIP. The test is not needed to be done at installation time, is it?

[mattias-p]

I thought of excluding manifest.t as well. It's certainly an easy way out. But to me it feels a bit like a hack. And I feel we have enough hacks in the build system already. For instance I believe such hacks are the cause for this problem in the first place.

I looked into what's going on with this failure. I believe I have a fix that involves removing a hack instead of adding one. And while writing to explain the solution I may have I came across a way to make it better, so I'm looking into that.

[matsduf]

I thought of excluding manifest.t as well. It's certainly an easy way out. But to me it feels a bit like a hack. And I feel we have enough hacks in the build system already. For instance I believe such hacks are the cause for this problem in the first place.

I noticed that MANIFEST.SKIP is excluded from the distribution. I guess t/manifest.t would work as expected if that is included.

[mattias-p]

I noticed that MANIFEST.SKIP is excluded from the distribution. I guess t/manifest.t would work as expected if that is included.

I thought of that as well. "Best practice" seems to be to exclude MANIFEST.SKIP from the dist file. I'm not entirely sure why but the recommendations I've found are consistent on that point.

[matsduf]

I thought of that as well. "Best practice" seems to be to exclude MANIFEST.SKIP from the dist file. I'm not entirely sure why but the recommendations I've found are consistent on that point.

It is probably reasonable to exclude MANIFEST.SKIP.

There is already a case when a test script is excluded, t/po-files.t. I cannot see what the problem is of excluding t/manifest.t too. It is only meaningful in a context where MANIFEST.SKIP is available.

[ghost]

I have been able to test it successfully.
It looks like some parts are similar to #929. Do you plan on rebasing after #929 is merged ?

[mattias-p]

@pnax Yeah, exactly.

[matsduf]

I have been able to test it successfully.

@pnax, when I tested it failed when I tried to install an installation package based on this PR. Did you try that?

[ghost]

when I tested it failed when I tried to install an installation package based on this PR. Did you try that?

I used the following commands:

make dist
cpanm --test-only Zonemaster-Engine-v4.1.1.tar.gz

And it worked (after upgrading Zonemaster::LDNS, otherwise I had failing DNSSEC tests).

[mattias-p]

I rebased onto current develop. I believe no other changes are necessary. Please test again.

[matsduf]

This works as it should and it will be a help that missing update of MANIFEST/MANIFEST.SKIP is discovered already by Github actions (or Travis).

But what is the purpose of including the manifest.t in MANIFEST? It has no meaning when the distribution package has been created, because only files in MANIFEST are included. Just as po-files.t is in MANIFEST.SKIP this file belong there. I agree that the saving would be small, but complexity does not increase. One saving is that Test::NoWarnings would not have to be installed for cpanm installations.

I approve anyway.