Move 3 newPackage options to beginDocumentation

[mahrud]

This is a draft for now to see what breaks. Comments welcome.

• fixed newPackage to permit terminal null entries
• moved 3 newPackage options to beginDocumentation
• adjusted existing packages with optional components
• corrected package options in TropicalTorics
• removed FirstPackage stuff from Polymake.m2
• unexported two undefined gfanInterface symbols

Closes #3369.

[d-torrance]

Would newPackage(String, Nothing) make sense instead to solve the specific issue of trailing commas?

With the new behavior, I'd be a little concerned that something like newPackage("Foo", 1, 2, 3, 4, 5) would be allowed.

[mahrud]

I feel like methods newPackage showing (newPackage, Sequence) is a lot less surprising than (newPackage, String, Nothing). I can assert that the other entries are null if you want?

[d-torrance]

Sure, that works

[d-torrance]

I use this option in the Debian package to fix some reproducibility issues and to cache some examples that occasionally crash, so I'd prefer to keep it around.

[mahrud]

UseCachedExampleOutput? Can you say more about how you use it? Maybe I don't understand it well. If cached examples are present, wouldn't M2 automatically use them? (I think it does, e.g. see the single realrank example that I added)

[d-torrance]

I think it needs to be true in order to copy any cached examples over:

M2/M2/Macaulay2/m2/installPackage.m2

Lines 556 to 562 in ec9e9ac

	-- use distributed example results
	else if pkgopts.UseCachedExampleOutput
	and not opts.RerunExamples and fileExists outf' and cmphash(outf', inputhash) then (
	if fileExists errf then removeFile errf;
	copyFile(outf', outf);
	verboseLog("using cached " | desc)
	)

I use it in the Debian package by patching several packages so that OptionalComponentsPresent is true, and then copying the cached examples from this directory over to the corresponding package directories before building the docs.

[d-torrance]

The builds are failing right now because the RInterface tests are loaded before this call to beginDocumentation() and so the OptionalComponentsPresent package option doesn't exist yet, which causes a problem when calling check.

A band-aid fix for this specific package would be to switch the order of the two load lines at the bottom of RInterface.m2, but I wonder if TEST should raise an error if the option doesn't exist yet, so we know that we're calling it after beginDocumentation().

[d-torrance]

Actually, that's not quite right. We're calling end since R isn't found before beginDocumentation(). This package is unusual in that regard.

But regardless, there should probably be some kind of check (either in TEST or check) to make sure that the OptionalComponentsPresent option exists to avoid strange "key not found" error messages.

[d-torrance]

I just pushed a fix for the RInterface-specific issue.

[mahrud]

Hmm, I think it shouldn't matter where beginDocumentation() is called. Actually, I tend to prefer defining tests before calling beginDocumentation(), because I've ran into weird issues where the tests weren't loaded since M2 stopped at beginDocumentation(). I'm not sure what specific situation caused this though.

For this issue, I think I might prefer treating the absence of OptionalComponentsPresent as it being true. I think this would solve the failure, no?

[d-torrance]

Yeah, you're right -- it doesn't matter since we won't be calling check until after the package is loaded.

Yes, your idea is much better than manually setting it for a specific package like I was doing!

[d-torrance]

I just pushed an alternate fix based on your suggestion. One of the builds was failing even after the RInterface-specific fix, so it must not be the only package where this was an issue.

[mahrud]

For now, I just wanted to make sure all tests passed, but I would like to simplify this further before moving it out of draft mode. Here are a few goals, but feel free to add more:

• keep beginDocumentation() simple, ideally one option only.
• maybe allow use of cached examples opportunistically, or even provide a function for generating a single cached example, so we can cache only the necessary examples (e.g. realrank is the only one from CoincidenceRootLoci that needs to be cached)
• simplify the logic in the background, maybe debug mystery issue I described above with tests disappearing after beginDocumentation()
• think about whether CacheExampleOutputs should be transitive.
• think about whether readPackage should know whether optional dependencies are present, because with this PR it will not
• maybe have one distinguished package for each optional dependency so we don't check for existence of the same software in multiple places
• along the way, stop using type or command and use findProgram
• bring back the newPackage options for backwards compatibility, but have them give a deprecation warning.

[d-torrance]

or even provide a function for generating a single cached example, so we can cache only the necessary examples

This would be awesome. I have a pretty hacky function that does this for the Debian package, but a cleaner solution would be great.