Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds missing files in MANIFEST #1136

Merged
merged 1 commit into from
Nov 24, 2023
Merged

Conversation

matsduf
Copy link
Contributor

@matsduf matsduf commented Nov 23, 2023

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.

@matsduf matsduf added the T-Bug Type: Bug in software or error in test case description label Nov 23, 2023
Copy link
Contributor

@marc-vanderwal marc-vanderwal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Shouldn’t this sort of thing be caught by continuous integration jobs, though?

@tgreenx
Copy link
Contributor

tgreenx commented Nov 23, 2023

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

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

@matsduf
Copy link
Contributor Author

matsduf commented Nov 24, 2023

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
Copy link
Contributor Author

matsduf commented Nov 24, 2023

@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.

@matsduf matsduf merged commit a36dcf7 into zonemaster:develop Nov 24, 2023
1 check passed
@matsduf matsduf deleted the corrects-manifest branch November 24, 2023 15:31
@marc-vanderwal
Copy link
Contributor

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
Copy link
Contributor Author

matsduf commented Nov 27, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Bug Type: Bug in software or error in test case description
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants