zonemaster / zonemaster-backend

The Zonemaster Backend - part of the Zonemaster project
Other
14 stars 23 forks source link

Adds missing files in MANIFEST #1136

Closed matsduf closed 11 months ago

matsduf commented 12 months ago

Purpose

Make MANIFEST clean.

Context

The files were added by #1092 and #1121, respectively, but not added to MANIFEST.

How to test this PR

Building should not complain on missing files in MANIFEST.

tgreenx commented 12 months ago

Shouldn’t this sort of thing be caught by continuous integration jobs, though?

It appears that it was removed by https://github.com/zonemaster/zonemaster-backend/pull/1001 due to being "broken". I definitely think that we should fix and restore it.

matsduf commented 11 months ago

It appears that it was removed by #1001 due to being "broken". I definitely think that we should fix and restore it.

I think we need something like this: https://github.com/zonemaster/zonemaster-engine/blob/master/t/manifest.t

matsduf commented 11 months ago

@tgreenx and @marc-vanderwal: Today we update MANIFEST and MANIFEST.SKIP manually. Instead we could run make manifest and having MANIFEST being automatically updated with anything not matching MANIFEST.SKIP. That could be included in make all.

marc-vanderwal commented 11 months ago

There are two possible strategies for handling the MANIFEST file.

Either we keep it under version control as we currently do. This comes with the implicit assumption that the MANIFEST be constantly in sync with the rest of the files under version control. That means it must be updated manually, and it is an error to commit a change that adds or deletes a file under version control and fails to update the MANIFEST. An automated test checking the package contents against the MANIFEST is therefore very desirable.

ExtUtils::MakeMaker provides make distcheck, that will do just that. It doesn’t set the exit status to 1 if there is a discrepancy, however, so it isn’t suitable for running in a CI pipeline; but a similar result can be achieved by hooking into ExtUtils::Manifest::fullcheck() directly from a Perl one-liner, like so:

# With the current working directory being the repository root
perl -MExtUtils::Manifest=fullcheck -e '($m,$e)=fullcheck; exit !(!@$m & !@$e)'

The alternative is not to keep the MANIFEST under version control and only keep MANIFEST.SKIP. Only then does it make sense to have the MANIFEST generated automatically during CI jobs or preparation of packages. Then it isn’t necessary to check the installed files against MANIFEST anymore, because then, the assumption is that MANIFEST.SKIP is always right. But I don’t really like this route, because then, there is no way of ensuring that the files being installed by the package are exactly what is necessary, no more and no less, at each PR.

I’m perfectly fine with the first solution, provided that we restore some kind of check to ensure that the MANIFEST stays in sync.

(PS: Yes, I have to admit that making the shortest one-liner that works as intended is a fun exercise.)

matsduf commented 11 months ago

@marc-vanderwal, in Zonemaster-Engine we have a working solution for checking if MANIFEST has been updated. I uses make distcheck and checks if the output is empty or non-empty. See https://github.com/zonemaster/zonemaster-engine/blob/master/t/manifest.t. It works under CI (Githbu actions).

In this repository we have no such check, and as far as I remember we have never had such check. I tried in #1137, but I could not make it work. I could not see that the MO files are generated, and then make distcheck will fail. Here we use Travis for CI.

The value of manually updating MANIFEST so I suggest that we switch to running make manifest before make dist. Of course then we do not need to keep MANIFEST under version control. We should still update MANIFEST.SKIP as needed.