add minetest to builtin standards

[BuckarooBanzay]

This PR adds the minetest lua api to the builtin standards

fixes #107

Open tasks:

• implementation review (technical, from the luacheck maintainer(s))
• content review from the minetest-community (modders)
• add constants and missing fields from add minetest to builtin standards #108 (comment)
• showcase / demos

[S-S-X]

Can use this for testing https://hub.docker.com/r/mineunit/luacheck but isn't supposed to stay or maintained afterwards so just if it might make testing simpler.

[alerque]

I think the luajit failure in CI is unrelated to this PR, but I also don't see any tests being added in this PR for the new functionality. At least one to check that the new preset value is being recognized and put in effect would be nice to see.

Otherwise this seems to be coming along fine. Obviously I can't judge the correctness of the API info you've entered so some feedback from other users would be useful there, but architecturally it seems like you're on the right track.

[wsor4035]

as a user skimming over this, the api's look correct

[BuckarooBanzay]

At least one to check that the new preset value is being recognized and put in effect would be nice to see.

can you point me to a location where such a test would fit in (there's quite a lot of test-stuff 😲)

Obviously I can't judge the correctness of the API info you've entered so some feedback from other users would be useful there

I'll arrange something 👍

[alerque]

I actually didn't see any where to point you towards such a test, so I made one myself. We're getting the builtin standards function tested in a few round a bout ways by testing various config parsing and rockspec lints etc., but nothing was very direct. I just pushed a commit here with a test like I was imagining should exist.

I don't see much point in trying to flesh out the sample to test every possible rule since setting that up would likely just propagate any errors you made in the API surface from the std definitions to tests anyway. If we had some sort of reference document from a different source to test against that would be cool but not required. As far as I'm concerned this local test is enough to verify that the std set is getting applied and also isn't breaking anything else. What exact rules are being applied is much more up to you and the minetest community.

[S-S-X]

Few things (it is likely that there's still many similar missing things, list is just grabbed from random luacheck run):

• Missing: LIGHT_MAX, CONTENT_UNKNOWN, CONTENT_AIR, CONTENT_IGNORE from minetest
• Missing: VoxelArea:new(...)
• Missing: registered_on_? tables on minetest (opinion: shouldn't be included. only mentioned in documentation but are not exactly documented for API use).

[BuckarooBanzay]

A question from the minetest-discord:

Anyways, this looks reasonable. (Haven't done the grunt work of checking this vs. our API docs though). What I'm slightly worried about: How do we keep this up to date after it's merged?

@appgurueu not sure about that, i think collecting the api-changes over major releases and creating a PR here would be a reasonable approach

I actually didn't see any where to point you towards such a test, so I made one myself.

@alerque thanks for that 🎉

If we had some sort of reference document from a different source to test against that would be cool but not required.

i created a PoC PR in on of our mod-reporitories: mt-mods/pipeworks#109 we usually just pull in the luacheck-action directly but i changed it there to use @S-S-X docker image with the modifications from here

Few things (it is likely that there's still many similar missing things, list is just grabbed from random luacheck run):

@S-S-X Right, i think there are a few more constants that aren't in the official docs (yet?) will add that the next few days 👍

[S-S-X]

few more constants that aren't in the official docs

Not sure how things that are not in official docs should be handled. I mean there's a lot of stuff that is technically available but isn't meant to be part of official public API.

I think those probably should either be first fixed on Minetest side (added to docs) or not added to spec here.

Has happened before that some things that weren't documented been silently dropped from core because it wasn't part official API so no reason for deprecation process.

[alerque]

I think those probably should either be first fixed on Minetest side (added to docs) or not added to spec here.

Yes. I would definitely suggest:

• Targeting the last stable release, not unreleased development APIs. The presets here should clearly identify what version is being targeted. In between releases we can keep a long-running branch or PR around that has the changes from HEAD, and we can set it up so that it is usable as a GH action itself. Then it can get merged when a release happens.

• Don't include any undocumented / unofficial APIs that are not considered stable upstream. This will create to much churn as they come and go and also contribute towards programmer confusion as it blurs the lines between scopes. I would expect any use of undocumented/unofficial/unreleased APIs to need to be commented or add exceptions to the RC file to allowed them on a case by case basis.

• Anyone targeting old versions of the API will have somewhat limited options, but using an old version of Luacheck closer to their target might be viable.

By the way you can probably save all of the shenanigans to get a working GH Action in @S-S-X's branch just by tweaking the action.yml file to point at this branch's Dockerfile rather than a pre-compiled container. We would just need to revert that commit before merging this.

[S-S-X]

Actually I forgot one simple thing, luacheck does already read config from cwd by default. This simple fact makes it completely unnecessary to rewrite/modify runner config for anything besides marketplace, and for custom marketplace listing there wouldn't be any real benefits as it would still be exactly same job.

Simply put, that image I've added was meant only for easy local command line testing and still should be used only for that (until it gets deleted / this pr done).

[BuckarooBanzay]

Targeting the last stable release, not unreleased development APIs. The presets here should clearly identify what version is being targeted. In between releases we can keep a long-running branch or PR around that has the changes from HEAD, and we can set it up so that it is usable as a GH action itself. Then it can get merged when a release happens.

Agreed 👍

Don't include any undocumented / unofficial APIs that are not considered stable upstream. This will create to much churn as they come and go and also contribute towards programmer confusion as it blurs the lines between scopes. I would expect any use of undocumented/unofficial/unreleased APIs to need to be commented or add exceptions to the RC file to allowed them on a case by case basis.

The only undocumented and included api's are those here: https://github.com/minetest/minetest/blob/master/builtin/game/constants.lua
i still decided to include them because:

• they have been relied on for years (over a decade even!)
• they aren't likely to change and may even find their way into the official docs

By the way you can probably save all of the shenanigans to get a working GH Action in @S-S-X's branch just by tweaking the action.yml file to point at this branch's Dockerfile rather than a pre-compiled container. We would just need to revert that commit before merging this.

I already switched the proof-of-concept PR to @S-S-X's action.
As for a showcase for mods/repositories that benefit by using the new minetest builtin, you can take a look at:

• The official "minetest-mods" org: https://github.com/minetest-mods, quite a handful of them are already using some kind of luacheck workflow (some use factorio's and some even plainly use apt-get to get the luacheck binary)
• The unofficial "mt-mods" org: https://github.com/mt-mods, most of them switched to the lunarmodules action already

I'm marking this PR as "ready" now, feel free to ping me up if something is missing 👍

[S-S-X]

Updated my dev/validation action so it is based on action definition and stripped down dockerfile provided by lunarmodules/luacheck here.
Basically just made it grab changes from this branch and use docker builder instead of compiled image. This was just to make it similar to actual deployed image / allow validating some aspects of final deployed image if this PR gets merged.

[appgurueu]

may even find their way into the official docs

Please open a PR for that so they actually do.

---

Odd, the LuaJIT regression test using busted is segfaulting, but I think that's not the fault of this PR. The luajit in that action should probably be upgraded to a self-compiled latest master. I vaguely remember having segfaults with older luajit versions + busted too, but I never got to tracking them down.

[S-S-X]

Tried mtg, looks good to me. Complains about registered_on_placenodes and registered_on_respawnplayers stuff which I think is correct behavior for these but maybe someone likes to validate:

• Commit S-S-X/minetest_game@8a570ac
• Luacheck https://github.com/S-S-X/minetest_game/actions/runs/7920243283/job/21622836205

[appgurueu]

Complains about registered_on_placenodes and registered_on_respawnplayers stuff which I think is correct behavior for these but maybe someone likes to validate [...]

I don't think this is correct behavior. Quoting lua_api.md:

All callbacks registered with [Global callback registration functions] are added to corresponding minetest.registered_* tables.

(the documentation unfortunately forgets to mention that an s is also appended)

Anyways, I've made the following PR to fix this: BuckarooBanzay#1

Edit: Looks like we're inconsistent about the s's. My PR should be fixed now. I'll open an issue on Minetest.

[alerque]

I still don't understand why all the convoluted multi-repository approaches to setting a CI test. As I mentioned about it should be a one-liner to make THIS ACTUAL PR the test, not some other repository with possibly different Lua setups, different action names, and modified config handling.

Can we adjust some/all of the proof of concept tests to be exactly like they would be in production, just substituting uses: lunarmodules/luacheck@v1 with uses: BuckarooBavzay/luacheck@minetest-builtin? With the commit I just pushed to us this repo's Dockerfile instead of cached container images, that should work to run tests in as close to a final environment as possible. You can keep the branch around until a published version has this minetest stuff available for people or I'll just add a branch that the GH Action can be run from. Honestly I'll probably be pushing out a release soon after this lands so it shouldn't be much of a transition period.

[alerque]

(And thanks for the help @appgurueu and @S-S-X, it is nice that this is getting a workout before release rather than after.)

[S-S-X]

Can we adjust some/all of the proof of concept tests to be exactly like they would be in production, just substituting uses: lunarmodules/luacheck@v1 with uses: BuckarooBavzay/luacheck@minetest-builtin?

Yeah why not, proposed that as a change suggestion on first PoC PR already but then noted it would need an update here so decided to just throw it in another repo as that's easiest and fastest way for me personally.

I don't think this is correct behavior. Quoting lua_api.md:

All callbacks registered with [Global callback registration functions] are added to corresponding minetest.registered_* tables.

Yes that's true too, I've just seen these as a things not yet meant to fully be a part of official public API.
Reason is that AFAIK changing those tables also requires you to manually update callback_origins which in turn I think is primarily meant to hold debugging information for internal use / stack traces / etc..
Requires as in unless you're fine with wrong information in error messages.

I guess reading registered_on_* however could be fine and that maybe could be added at this point without waiting for Minetest release that introduces docs for those, but in that case either PR callback_origins to also be part of public API or be sure that registered_on_* contents are defined read only.

[appgurueu]

Yes that's true too, I've just seen these as a things not yet meant to fully be a part of official public API.

Well, regardless of what they are meant to be, they are documented as part of the public API. The API does not state whether these tables are readonly, however.

Reason is that AFAIK changing those tables also requires you to manually update callback_origins which in turn I think is primarily meant to hold debugging information for internal use / stack traces / etc.. Requires as in unless you're fine with wrong information in error messages.

Good point, however:

Not inserting a callback into callback_origins won't lead to "wrong" information. It will just lead to the engine being unable to "identify" the offending mod (substituting "?" in the error message instead). It's not a big deal, tbh.

(Technically, use patterns are possible which mutate minetest.registered_* without having to manually update minetest.callback_origins, such as minetest.register_* followed by swap(minetest.registered_*, 1, #minetest.registered_*) where function swap(t, i, j) t[i], t[j] = t[j], t[i] end)

I guess reading registered_on_* however could be fine and that maybe could be added at this point without waiting for Minetest release that introduces docs for those

Well, the registered_* callbacks are implicitly documented (as quoted). It is not documented whether they can be mutated though, we should probably clarify that.

but in that case either PR callback_origins to also be part of public API or be sure that registered_on_* contents are defined read only

Frankly, I think callback_origins is poor design and we should probably get rid of it. You can't "identify" the offending mod like that. It just leads to reports going to the wrong mods. It would probably be best to get rid of this inaccurate blaming entirely.

When you get a crash, you get a stacktrace. This may or may not involve multiple mods. If anything, we should infer (possibly) responsible mods (plural) from the stacktrace, rather than always blaming the mod which registered the callback.

[alerque]

I'll rebase this to fix commit messages and drop the temporary action runner commit after the ongoing discussion re hook tables is resolved.

[BuckarooBanzay]

I'll rebase this to fix commit messages and drop the temporary action runner commit after the ongoing discussion re hook tables is resolved.

This should be resolved now, let me know if there is something missing or if i can help with something regarding the merge/rebase.

[appgurueu]

The question is: Are you targeting 5.8 or 5.9 here? I just see a link to "master" in the current sources, so I assume this is based on 5.9-dev, in which case we should probably wait until 5.9 releases (which will also give us some time to resolve the hook issue)?

If this were for 5.8, this should be good as-is. Even if the docs are inaccurate, this matches what the code does (and hence what you will find in correct mods).

We will not be able to break any of these "hook" table names before 6.0, though we may deprecate some names in favor of new names in 5.9.

[alerque]

From my standpoint targeting the last stable release plus anything that is widely considered de facto is probably best, then put out an update when there is a new target available with official changes that coders ought to be taking under advisement anyway.

But I also don't know how the release cycle and workflows look like for minetest, so I'll deffer to community consensus/what people are willing to contribute.

[BuckarooBanzay]

If this were for 5.8, this should be good as-is. Even if the docs are inaccurate, this matches what the code does (and hence what you will find in correct mods).

@appgurueu yes, that's the idea, waiting for the next minetest release is not an option IMO: no one knows when that will be and if the naming issue will be closed by then.

From my standpoint targeting the last stable release plus anything that is widely considered de facto is probably best, then put out an update when there is a new target available with official changes that coders ought to be taking under advisement anyway.

@alerque this is my plan, i'll whip up another PR if the minetest api should change

[alerque]

Does that mean ya'll are done with this PR?

[BuckarooBanzay]

Does that mean ya'll are done with this PR?

yeah, i think this should be ok 👍

[BuckarooBanzay]

@alerque is there anything i can do to help you merging this?

afaik, this line here:

luacheck/action.yml

Line 10 in 8675c60

image: Dockerfile

should just be reverted to point at docker://ghcr.io/lunarmodules/luacheck:v1.1.2 is that correct?

[appgurueu]

For the record, I've opened a PR to document the current state of affairs properly on the Minetest side of things: minetest/minetest#14509

[alerque]

I think pulling out development debug remnants is the last thing to do here.