Linter extensions

[mugling]

Summary

• Collection of usability enhancements for JSON linter.
• Adds make lint target for automatic re-linting
• Considerably improved peformance (via partial re-linting)
• Better diagnostics (exit codes, line numbers)

Automatic re-linting

• To check for all errors run make lint-check
• To automatically fix formatting errors run make lint

Performance

Previously lint.cache was global so updating a single entry from json_whitelist forced a full re-lint of all files. This PR only lints changed files which is significantly faster - sufficient to suggest installing make lint as a git hook.

Diagnostics

JSON syntax errors now include the line and column:

parse error: Expected value before ',' at line 11, column 18
Syntax error in data/json/items/gun/223.json
make: *** [Makefile:881: lint] Error 65

Formatting errors now include this as well as a suggested change:

Linting data/json/items/gun/223.json: ERROR: Format error at line 11
<     "to_hit": -1,'
>     "to_hit" : -1,'
make: *** [Makefile:870: obj/lint.cache] Error 65

Shell exit codes are passed through to make with anything other than EX_DATAERR 65 indicating a problem with the linter or build process itself (file permissions etc).

Miscellaneous

• json_whitelist now supports # comments and blank lines
• Sort order for entries (if any) can be specified in json_whitelist

Jenkins buildbot

Is not affected. make lint-check does not depend on jq although if @narcotiq agrees that's possible in a further PR I'd like to replace the make json-check target with a check via jq

[mugling]

Ready

[mugling]

@Coolthulhu knowing your on windows at the moment and that there is a need to unblock other PR's do you have any objection to this being merged now. We can monitor the buildbot carefully - it shouldn't be affected at all (beyond producing better diagnostic messages) but if it was we can revert this PR in it's entirety?

I am going to work further on windows support (probably a perlcc compiled version of the linter initially) but that obviously won't be ready today.

[Coolthulhu]

I do have a Perl set up on mingw, so I can test it.
I'll try to merge a batch of PRs sometime later today.

[mugling]

Good stuff. I'm torn between packaging the linter for windows or working out a set of instructions for ActivePerl (does that still exist)?

Long term we might rewrite the linter in C but not until coverage is 100% (eg. the JSON corpus could function as one giant unit test)

[DangerNoodle]

This would be helpful, yes. Expanded diagnostics will also be useful, as I have still been getting used to handling some things with regards to pull requests.

In my pull request #18346, before I removed my attempt to add a change to Vehicle Additions Pack, all I had to go on was this error:

Linting data/mods/blazemod/blaze_blob_construct.json: make: *** [obj/lint.cache] Error 65
make: *** Waiting for unfinished jobs....
Build step 'Execute shell' marked build as failure
[WS-CLEANUP] Deleting project workspace...[WS-CLEANUP] Skipped based on build state FAILURE
Finished: FAILURE

[mugling]

before I removed my attempt to add a change to Vehicle Additions Pack, all I had to go on was this error:

In that case all your fields are valid so you either have a syntax or formatting error. Wait for this PR and it should give you the exact line or better still run make lint locally

[DangerNoodle]

Thank you. I assume I can run make lint in Git Shell?

Earlier commits also mention an instance of "unmatched context" regarding num_tests which I am unsure about the meaning.

[mugling]

That message means either you entered an invalid field or the ruleset doesn't cover that. Can you post the offending section?

[DangerNoodle]

It was when I reformatted the blob construction and terrain entries added by Blaze, by outright copy-pasting from valid entries and editing them as needed. In the case of the terrain file:

        "bash": {
            "str_min": 1, "str_max": 1, "num_tests": 2,
            "sound": "squish!",
            "sound_fail": "squish!",
            "ter_set": "t_pit",
            "items": [
                { "item": "bfeed", "count": [ 250, 500 ] }
            ]
        }

Placement of num_tests was due to copying wholesale from the clay and sand mounds.

Apologies for bringing this up in an unrelated pull request.

[mugling]

Is num_tests actaully a valid field though?

[DangerNoodle]

It does not seem to generate load errors in the game itself, since that field is used to the vanilla terrain entries.

It is also strange that my copying presumably badly-formatted entries from vanilla construction and terrain generated linting errors when used in a Blazemod file, but not in the source files, which also received changes in that pull request.

[mugling]

It does not seem to generate load errors in the game itself, since that field is used to the vanilla terrain entries.

Doesn't mean it's valid though. May well do absolutely nothing?

[DangerNoodle]

Not sure, to be honest. Later on I will attempt to re-implement the file changes minus num_tests, and test out make lint to see what I am missing.

[mugling]

Linter is working as intended. A quick grep num_tests src/* shows no matches.

[DangerNoodle]

I will have to figure out what I did wrong then. Thank you for the help.

[mugling]

I do have a Perl set up on mingw, so I can test it.
I'll try to merge a batch of PRs sometime later today.

How did you get on?

[mugling]

@Coolthulhu this PR too if you can, it unblocks a number of other authors PR's

[Coolthulhu]

Missed this somehow.

Also, I was supposed to mergetest this yesterday, but then suddenly real life happened. Sorry.

[DangerNoodle]

Hope it was not something serious @Coolthulhu.

[Coolthulhu]

Conflicts with #18342
I picked the version here

[mugling]

Can we push this one and then rebase #18342 - the latter is a trivial conflict and this one is much more important

[Coolthulhu]

I'm getting segfaults:

$ tools/lint.sh tools/format/examples/ammo.json
tools/lint.sh: line 27:  9128 Segmentation fault      $JQ '.' $file > /dev/null
Syntax error in tools/format/examples/ammo.json

Though it could be caused by an exotic environment. Msys supplies it's own [ function.

[mugling]

Do you have jq installed and in your path?

[Coolthulhu]

Yes. Though it might be vulnerable on windows - I got segfaults while trying to concatenate all files (concatenating the files and then parsing them separately worked).

[mugling]

Comment out the calls to jq in your local copy. Should allow you to otherwise verify

[Coolthulhu]

jq "sort_by(.)" tools/format/examples/ammo.json
Segmentation fault

It looks like windows and jq don't really like each other...

[mugling]

Could be worth reporting upstream. FWIW the lint-check target isn't (yet) dependent upon jq

[Coolthulhu]

Found the reason:
jqlang/jq#1205

I'm using the 64 bit version, but it seems that same thing happens to me. I managed to get around it by cding to the directory first.

[Coolthulhu]

I can't really test it right now. The tools have gotten quite complex and require a *nix system to be executed.
This is a big problem. We need either workaround for Windows or looser restrictions; this is impairing contributions.
A homebrewed script in one chosen language (say, Lua) would be preferable, even if it had less functionality and slower execution. Alternatively make formatting non-mandatory and apply it separately (automatically after merges?).

[mugling]

The same definitely applies to astyle. This PR would cause Jenkins to produce meaningful debug messages aiding contributions. Merging it doesn't worsen anything.

Automatic merges afterwards wouldn't work because how do we handle committed JSON that contains bad fields - this happened yesterday in another PR and was detected by the linter.

A rewrite in C can happen at some later date but not before the ruleset is complete. Increasing the scope without completing the original goal first is doomed to failure.

This should really be merged now...

[Coolthulhu]

This should really be merged now...

I can't test it though. If you want to merge it, it's all on you.
If not, you can get someone who has a *nix at hand, like BevapDin or Narc (intentionally not pinging) to make sure it works outside your specific setup.

[mugling]

I can't test it though. If you want to merge it, it's all on you.

Fair enough argument although as Jenkins is already broken I don't think I can make it any worse. If for any reason it does we can revert. None of this PR results in code running on user installations.

Sorry to ping @narc0tiq but as a separate issue Jenkins appears to have run out of disk space

java.io.IOException: No space left on device