Add pre-commit check for MANIFEST membership

[dginev]

Example effect, when adding a new binding and forgetting to update the MANIFEST:

Some files are missing from MANIFEST, please add them: 
	lib/LaTeXML/Package/new.sty.ltxml;
COMMIT ABORTED

Hopefully can make the @brucemiller 's reviewer life a little more pleasant!

[dginev]

I was considering making this a GitHub PR bot, but realized the "change suggestions" flow isn't that pleasant, at best the bot can leave a comment, and that's already too late and inconvenient.

Since I have the pre-commit hook always on when committing nowadays, adding the manifest check in there is very natural for the dev flow, and saves the public embarrassment aspect of it all :>

[bfirsh]

Might be worth running this check as part of Travis too, if for whatever reason the pre-commit hook doesn't run. That would have made me fix it in #1078.

[dginev]

Oh, that's a curious idea. I could move it under Util::Test and add a simple test entry for the check.

[dginev]

Funny story - adding a manifest test actually revealed files missing from the MANIFEST. I used a standard Perl method in the test, so the SKIP file is respected.

I also updated the pre-commit hook to always run the manifest check, so we're doubly safe.

[dginev]

added some exclusions I needed for my local setup, bunch of aux files.

[dginev]

@brucemiller if you merge this PR next, I can quickly run through all my other open PRs and ensure new files are properly added to the manifest. Cool incentive? :>

[brucemiller]

Hmm...tempting offer!

But I'm wondering; shouldn't this be more whitelist than blacklist? Ie. except for a few top-level files, only files in t and lib should appear in the MANIFEST (if committed, of course). Isn't the MANIFEST primarily for the benefit of the tarball?

[dginev]

@brucemiller you'd have to take that particular point up with the ExtUtils developers. I relied on the mature tooling of ExtUtils::Manifest and it only offers a SKIP blacklist option for refining the manifest filter.

I could switch to a whitelist, but then I think I would have to rely on my own code to post-filter the results of fullcheck(). But the bigger problem is that this PR is supposed to tell you when you miss a file that was meant to be added to the manifest -- and if you're adding a new subdirectory to latexml, a whitelist approach will just treat it as ignorable and ok. The blacklist approach will pick it up and ask you to either add a rule for it to the SKIP or the MANIFEST file, so that you explicitly handle it, as is preferable.

Example: we add a new directory, "extended_tests" at the root level of LaTeXML, that are only ran with a special flag. If I had a whitelist approach, it would never notice it isn't in the manifest. The current PR would alert you that you should add it to the manifest or skip it explicitly. And then one could ask themselves the question whether the extended tests belong in the distribution or not, and so forth...

[brucemiller]

Indeed, the exclusions are already covered in SKIP. Thanks!!!

[brucemiller]

Hmm... this makes sense as a pre-commit hook, if it's checking committed files. But I'm having second thoughts about being part of make test: it's pretty radical if you have any scratch work lying around.

[dginev]

@brucemiller it is indeed very strict, but only as long as you haven't added your scratch examples to SKIP.

If you're at a point of running make test on a branch, one would think the files in it are ready to be mentioned in the manifest?

[dginev]

Another thing to keep in mind is that you may avoid this entirely by using a more precise (and faster) make test invocation. For instance, if you're only extending grammar coverage, you could run:

make test TEST_FILES=t/70_parse.t 

[brucemiller]

As Dirk Gently says: Everything is Connected. So, I always run a full make test before I commit, branch or not. And having to maintain SKIP over local, temporary changes seems more obtrusive to being obliged to maintain MANIFEST over intentional, permanent ones, no?

[dginev]

@brucemiller I understand it looks obtrusive, especially if you already have a lot of scratch files at the local checkout. Or if you end up moving the same file several times and you need to rename it in the manifest each time.

That said, those ought to be rare events for most branches, so I am personally OK with having the extra strictness. If you hate it, feel free to remove it.

I recall the main reason I loved the idea of having a standalone test is that Travis-CI (and now Appveyor) will fail if we forget to update the manifest. So even if you do both of:

• not run make test
• commit with --no-verify and push

You still won't trick the maintainer, because Travis will immediately go red and complain. I have pushed "small changes" with --no-verify, and not testing in my life, though I would claim a long time ago when I was young and careless, but I'd rather trust a machine to double-check any PR open for latexml.

[dginev]

Indeed, the real main use case is that if a brand new contributor shows up and files a new pull request for their new binding, Travis will educate them about our manifest issues, and you don't have to double-check the pull request every time.

[dginev]

OK, double-checked, both Travis and Appveyor set environmental variables, notably CI is set to True, so I can make the manifest test only execute if we're in a continuous integration setting. Does that sound reasonable?

[brucemiller]

Hmmm... let me see how cleanupable skip (and development habits) are...

[brucemiller]

Since I'd still like to do things like add files & testcases and run make test before committing, I'm still likely to accept the other PR. But in the interest of general hygiene, it seemed like a good time to cleanup things, and improve .gitignore and MANIFEST.SKIP at which point I noticed... or rather failed to notice...

So, does the pre-commit hook actually use the skip file? I don't see it, and it's complaining about committing .gitignore

[dginev]

The pre-commit hook doesn't use the SKIP file in master, and I am oddly confused where my change to it disappeared into. I recall updating it at a later point, when I discovered fullcheck() but it's probably buried in my stash or a side branch or ... I will update that in the travis PR if that's ok with you.

[dginev]

A bit saner now, here is the commit (in the open PR) 77155b0