wmllint proposals

[Groggy_Dice]

With a new development cycle underway, I have been thinking about what new debugging capabilities can be added to wmllint this time around. Here are some suggestions I believe would enhance and improve wmllint.

I am not a programmer and I don't know Python, so I can't submit patches myself. In the meantime, I am using shell commands to accomplish many of the things I am proposing to enable wmllint to do. I am including them here, in case some people find them useful for 1.10 (or if these features aren't added to wmllint at all). Of course, I'm limited by my amateur knowledge, and I'm sure real programmers will see better ways of doing many of these things.

1 Convert old UMC paths from data/campaigns to data/add-ons

Since the outdated binary path was perhaps the most bedeviling bug I encountered during my first port (Prudence 1.6 to 1., trying to figure out why that custom unit's sprite wouldn't show up, this feature is dear to my heart. I already brought this up once before, when I spotted non-working code in wmllint that seemed to be aimed at doing this, but I didn't know Python well enough to realize that it was deliberately neutered, not buggy (aside from referring to the directory as add_ons instead of add-ons).

The reason this feature hasn't been activated is that it would "clobber mainline" campaigns, which still use the data/campaigns path. But even if wmllint isn't going to alter the paths, it could give a warning like, "Path may need hand fixup from data/campaigns to data/add-ons," even if this goes against the ethos that mainline campaigns shouldn't give off any error messages.

However, I think it is possible to make wmllint update UMC paths, while still being safe for mainline. First, you would define a list of mainline campaigns, just as there is a list of the standard usage classes. Based on the format of other variables, it would look something like the code below. When wmllint encounted a data/campaigns/ path, it would then check against the list of mainline campaigns. If it did not match, wmllint would convert the path; if it did, it would skip it.

Code: Select all

mainline = ("An_Orcish_Incursion",
            "Delfadors_Memoirs",
            "Descent_Into_Darkness",
            "Eastern_Invasion",
            "Heir_To_The_Throne",
            "Legend_of_Wesmere",
            "Liberty",
            "Northern_Rebirth",
            "Sceptre_of_Fire",
            "Son_Of_The_Black_Eye",
            "The_Hammer_of_Thursagan",
            "The_Rise_Of_Wesnoth",
            "The_South_Guard",
            "tutorial",
            "Two_Brothers",
            "Under_the_Burning_Suns",
            )

For now, I have my own command-line path fixer that seems to do a thorough and safe job of converting add-on paths. Of course, "EMMELL" could be replaced by any string unusual enough that it is not going to already be occurring in any files.

Code: Select all

echo "Enter the path to your Wesnoth directory and hit enter. This path can be relative - \".\" if this terminal is already in the Wesnoth directory, \"..\" if it is one directory deep inside Wesnoth."
read WESNOTHDIR

## Eliminate trailing slash, if any
WESNOTHDIR=$(echo $WESNOTHDIR | sed 's;/$;;')

echo "Enter the path to your userdata directory and hit enter. This path can be relative, and can also accept wildcards (*)."
read UMCDIR

## Eliminate trailing slash, if any
UMCDIR=$(echo $UMCDIR | sed 's;/$;;')

ML=$(ls $WESNOTHDIR/data/campaigns) ; 
for AA in $(grep -r "data/campaigns" $UMCDIR | sed ':umc s@\(^[^:]*:\)\([^\v]*\)\(data/campaigns/.*\)@\1\2\v\1\3@ ; t umc' | sed 's/\v/\n/g' | grep "data/campaigns" | grep -v "data/campaigns/$ML" | sed 's/\(^[^:]*\).*/\1/' | grep "\.cfg$" | sort -u ) ; do 
echo "$ML" | sed 's;.*;\\@&@s#/campaigns/&#EMMELL&#g;' | sed \$\ 'a\s;data/campaigns;data/add-ons;g'|sed \$\ 'a\s@EMMELL@/campaigns/@g' | sed -f - -i.back "$UMC" ; done

(I also presented a command in that earlier post, but I've revised it. First, I found that it's actually faster to 'grep -r' everything rather than filter with 'find -name "*.cfg"', so I switched. Second, the old command operated on all files with the "data/campaigns" string, even if all the paths were for mainline, meaning the net result would be no change. I dislike having my timestamps altered unless there's actually a change, so I've gone to the trouble of making sure to exclude files with only mainline paths. Theoretically, the old command also could have missed a path if there were a line that had both a mainline path and a UMC path; although there doesn't actually appear to be such a case, I've gone to somewhat elaborate lengths to make sure that this command will still work.)

2 Strip userdata/ from paths

Speaking of paths, some misguided authors put userdata/ in their paths, causing problems not just for non-Windows users, but fellow Windows users who chose to put userdata in My Games. Why not have wmllint remove this mistake?

Meantime, I run this command to delete userdata/ from paths:

Code: Select all

read UMCDIR
## Look at what lines and files will be changed first
grep -r "^[^#]*userdata/[dac]" $UMCDIR | grep "cfg:"
## Then act
grep -rl "^[^#]*userdata/[dac]" $UMCDIR | grep "cfg$" | xargs -d \\n sed ':umc s@^\([^#]*[^./]\)\.\?\.\?/\?user[dat/]*\(data/[ac]\)@\1\2@ ; t umc' -i.back

(In case you're wondering why I don't simply replace "userdata/data/", there's a set of add-ons in 1.4 that use "userdata/campaigns" instead of "userdata/data/campaigns" in their paths.)

A sidenote: my command strips a leading "../", if there is one. From what I can tell, Wesnoth does the same thing whether a path begins with "../" or not; but I can't be certain that "../" is ALWAYS unnecessary. If in fact every instance is purposeless, wmllint could strip that too.

3 Search for undefined #ifdefs

While taking a look at someone's effort to port Dark_Elves a few weeks back, I ran across a scenario that was using #ifdef MEDIUM instead of NORMAL. This is a common error, and I've also seen designers capitalize only the first letter of a difficulty #ifdef. But actually, wmllint could look for all of an add-on's defines, and check the #ifdefs against them. If it couldn't find a define for a particular ifdef, it could give an error message like "possibly undefined ifdef."

Currently, I have worked out a command to match an add-on's defines and ifdefs (and ifndefs, I almost forgot about them). Then it filters the remaining #ifdefs against the game's built-in defines, and __UNUSED__, which seems to be a standard way of creating a deliberately undefined #ifdef.

This produces results that are probably different from what wmllint would produce, since it only operates on one add-on at a time. If an add-on called on an era or other dependency with additional defines, my command misses them and produces false positives. It also doesn't parse the Wesnoth core, as would typically be done in a wmllint dryrun, meaning for example that ENABLE/DISABLE_[level 4 unit] #ifdefs retained by custom units are considered undefined unless those are included in the _main.cfg's extra_defines. (Whether that's a false positive or not may be a matter of opinion.)

Code: Select all

read UMCDIR
for UMC in $(find $UMCDIR -maxdepth 0 -type d) ; do 
DIF=$(grep -rhs "define.*=\|difficulties.\?=\|#define" $UMC.cfg $UMC | sed -e 's/.*=//' -e 's/"//g' -e 's/.*#define \([^ ]*\).*/\1/' -e 's/ *#.*//' -e 's/[ \t]*//g' -e 's/\[/\\[/' -e 's/\r//'|sed -e ":x N ; s/\n/,/ ; t x" | sed 's/,/\\|/g') ; 
grep -rsn "^[^#]*#ifn\?def" $UMC/* $UMC.cfg| grep -vw "$DIF" | grep -v "#ifn\?def MULTIPLAYER\|#ifn\?def DEBUG_MODE\|#ifn\?def EDITOR\|#ifn\?def WESNOTH_VERSION\|#ifdef __[EOM_]*UNUSED__" | grep -vi "changelog[.cfgtx]*:\|readme[.cfgtx]*:\|\.cfg[-.~]" ; done

To give some idea of how useful (or not) such a feature would be, I am attaching a list of hits I got from my add-ons collection. I've decided to give the results raw, including known false positives, to enable an honest assessment of benefits versus drawbacks. For instance, I know full well that "#ifdef CAMPAIGN_INVASION_FROM_THE_UNKNOWN_EPISODE_II" isn't really undefined (but coming up with code that wouldn't see that as undefined would be pretty tricky). A large proportion of hits come from apparently deliberately disabled defines in Sceptre of Life, and LSB_COMMENT from Return to Noelren appears to be deliberately undefined, as other examples.

But there are plenty of real hits. Besides the instances of #ifdef MEDIUM that first inspired me, there are attempts at #ifdef MULTIPLAYER that came out as MULITPLAYER (Ooze) or MULTILAYER (Elves and Wose), for example. There is even one minor hit in a mainline campaign scenario (UTBS). There are several wmllint checks that produce false positives yet are considered on balance to be useful, and I think the same would be true here.

ifdefs.7z

#ifdef search

(6 KiB) Downloaded 233 times

4 orc portraits
In 1.8, when Wesnoth transitioned from the five old cartoon-style orcish warlord portraits, the developers only considered three portraits suitable replacements. Thus, in changing references to the five old portraits to the three new, two were doubled up.

By 1.10, new orc portraits were available, but wmllint was not updated. Thus, I suggest modifying wmllint to use some of the new portraits, adding more variety to the portraits used for orcish leaders. Although this mod is too late for the mainline campaigns and other campaigns already converted to 1.8, there are many unported earlier campaigns.

This is one way wmllint could be modified, reflecting my partiality for the grunt-5 and grunt-6 portraits. You could change the particular portraits (and their order) based on your own taste.

Code: Select all

        # Changes after 1.8rc1
        ("portraits/orcs/warlord.png", "portraits/orcs/transparent/warlord.png"),
        ("portraits/orcs/warlord2.png","portraits/orcs/transparent/warlord.png"),
        ("portraits/orcs/warlord3.png","portraits/orcs/transparent/grunt-2.png"),
        ("portraits/orcs/warlord4.png","portraits/orcs/transparent/grunt-2.png"),
        ("portraits/orcs/warlord5.png","portraits/orcs/transparent/grunt-3.png"),

        # Changes after 1.10
        ("portraits/orcs/warlord.png", "portraits/orcs/transparent/warlord.png"),
        ("portraits/orcs/warlord2.png","portraits/orcs/transparent/grunt-5.png"),
        ("portraits/orcs/warlord3.png","portraits/orcs/transparent/grunt-6.png"),
        ("portraits/orcs/warlord4.png","portraits/orcs/transparent/grunt-2.png"),
        ("portraits/orcs/warlord5.png","portraits/orcs/transparent/grunt-3.png"),

I have some other suggestions, but I'm still writing them up. That may be for the best, as it gives time for these ideas to be properly digested and considered.

[Groggy_Dice]

Encouraged by the overwhelming feedback on my previous proposals, I'm back with a couple more suggestions!

5 Drop the .mask extension "fatal" error exit

Nowadays, probably the biggest single source of wmllint "crashes" isn't actually a crash, but a deliberate exit. If a scenario calls a file that doesn't end in .mask as a mask, wmllint exits with a "fatal error." The rationale for this behavior is given in wmllint's introduction:

Code: Select all

# Standalone terrain mask files *must* have a .mask extension on their name
# or they'll have an incorrect usage=map generated into them.

However, it doesn't appear that Wesnoth's engine has a problem with masks that have a usage=map attribute. Whether a file is used as a map or mask seems to depend solely on whether it is invoked by a map_data= or mask= key, not what value the usage= key has. In fact, at least one campaign has a scenario where the same file is called as both a map and a mask (War_of_the_Dragon/scenarios/11Temple_of_the_Deep.cfg).

Given that this "issue" isn't a problem for the game itself, it seems too drastic to have wmllint abruptly terminate in mid-operation. Indeed, one developer has intimated that this could be about wmllint author ESR's personal coding standards, not the BfW engine: "...it could just as well be some standard esr likes to impose on everyone who wants to benefit from wmllint." Now, I appreciate what ESR has done for Wesnoth and open source, and that wmllint is his baby. However, wmllint is, after all, copylefted collaborative code, not a one-person project where personal preferences prevail. Furthermore, ESR has reportedly been stepping back from his Wesnoth involvement lately. Given this, I don't see why his wishes should be given a veto, if other developers think there should be a change.

What exactly should be done? One answer might be to simply delete the code altogether. However, in this example, I've commented out the sys.exit line, and softened the error message for those people who actually care about their mask files getting a usage=map value.

Code: Select all

## ORIGINAL
            elif 'mask=' in line and not (refname.endswith("}") or refname.endswith(".mask")):
                print \
                      '"%s", line %d: fatal error, mask file without .mask extension (%s)' \
                      % (filename, lineno+1, refname)
                sys.exit(1)

## REPLACEMENT

            elif 'mask=' in line and not (refname.endswith("}") or refname.endswith(".mask")):
                print \
                      '"%s", line %d: mask file without .mask extension - wmllint will generate usage=map instead of usage=mask (%s)' \
                      % (filename, lineno+1, refname)
                # We are not exiting over the .mask extension issue anymore
                #sys.exit(1)

I've tested this with a dryrun on several campaigns, and War of the Dragon, and Sceptre of Life, no longer crash, without affecting wmllint's other operations.

Of course, the text in the introduction will also need to be removed or changed. Here is a possible rewrite.

Code: Select all

# If you would like wmllint to generate usage=mask instead of usage=map in your
# mask files, you *must* name them with the .mask extension.

6 Fix the Invasion of Arendia map bug

While I was looking at the mask exit code, I noticed that the code right below it dealt with excluding certain map_data= lines, including one circumstance described as "pathological." Some time back, I reported that Invasion of Arendia crashed wmllint. Two unfinished scenarios had the line 'map_data=""', and the unfilled quotation marks choked wmllint. But now I see how to fix it!

Although the Arendia bug seems to be a variant on the "pathological" condition, my fix is more similar to the syntax of the other exclusions.

Code: Select all

        # Exclude map_data= lines that are just 1 line without
        # continuation, or which contain {}.  The former are
        # pathological and the parse won't handle them, the latter
        # refer to map files which will be checked separately.
        if map_only or (("map_data=" in line or "mask=" in line)
                        and line.count('"') in (1, 2)
                        and line.count('""') == 0    # <-- This is my new line
                        and line.count("{") == 0
                        and  line.count("}") == 0    # By the way, why the extra space?
                        and not within('time')):

I've tested this with a dryrun on Invasion of Arendia and several other add-ons, and wmllint no longer barfs, while seeming to do everything else it was doing before. The main objection I can see to adding this bugfix is that since only one add-on is known to have this bug, some might think it's not worth the overhead.

Meanwhile, I'm continuing to work on my other ideas!

[Groggy_Dice]

7 Recognize units created by the {NAMED_*UNIT} macros

In theory, the "unknown 'XXX' referred to by id" message ought to be a valuable message alerting designers to misspelled names or characters that they forgot to create or recall. Unfortunately, it may be the wmllint message most prone to high false positives, because it doesn't recognize units created or recalled by macros.

To deal with this, wmllint allows you to add a magic comment telling it to recognize an id. However, having to do more work defeats the purpose of using a macro in the first place, and the tedium of slogging through false positives and adding magic comments make this error message often seem like more trouble than it's worth.

But, since wmllint doesn't do macro expansion, what can be done?

Well, many units are created through the {NAMED_*UNIT} macros. So I've added two regular expression rules to my wmllint, one for when the id is enclosed by parentheses and/or quotes, and the other for when a space is the delimiter. I inserted them just above the "wmllint: recognize" code (which served as my model).

Code: Select all

## NEW CODE
        m = re.search('{NAMED_[A-Z_]*UNIT +[0-9]+ .+ [0-9]+ +[0-9]+ +[("]+([^")]+)[)"] +.+}', lines[i])
        if m:
            present.append(string_strip(m.group(1)).strip())
        m = re.search('{NAMED_[A-Z_]*UNIT +[0-9]+ .+ [0-9]+ +[0-9]+ +([^("][^ ]+) +.+}', lines[i])
        if m:
            present.append(string_strip(m.group(1)).strip())
## BACK TO OLD CODE
        m = re.search("# *wmllint: recognize +(.*)", lines[i])
        if m:
            present.append(string_strip(m.group(1)).strip())

I've tested this with dryruns, and wmllint is no longer giving off "unknown 'XXX' referred to by id" messages from these macros, as it was before.

Now, it occurred to me that it might be more efficient to search once for {NAMED_*UNIT, then search the hits, than to have two regular expression searches. I also use "m =", even though it is already being used for "wmllint: recognize". I saw other examples of such reuse, so I assumed there would be no problem.

So I tested some variant code. It works, but I don't know if it is actually searching just the results, or conducting three searches (an attempt to substitute "named" for "lines" in the subsearches produced a Python crash).

Code: Select all

        named = re.search('{NAMED_[A-Z_]*UNIT +[0-9]+', lines[i])
        if named:
            name1 = re.search('{NAMED_[A-Z_]*UNIT +[0-9]+ .+ [0-9]+ +[0-9]+ +[("]+([^")]+)[)"] +.+}', lines[i])
            if name1:
                present.append(string_strip(name1.group(1)).strip())
            name2 = re.search('{NAMED_[A-Z_]*UNIT +[0-9]+ .+ [0-9]+ +[0-9]+ +([^("][^ ]+) +.+}', lines[i])
            if name2:
                present.append(string_strip(name2.group(1)).strip())

I have some ideas for how wmllint could handle custom macros that create or recall units that I will share later, but I'm afraid they will require actual knowledge of Python to implement.