tox and GitHub checks are not aligned

[kurtmckee]

After opening #1555 the regex linter warned me about a problem in my patch. This was not caught during extensive testing using tox, and indicates that tox and the GitHub checks are not aligned with each other.

tox must be updated to use the same regex linter that GitHub checks is using so that contributors can catch problems before submitting PR's.

[Anteru]

Right, but the regex linter check requires make support, which tox doesn't, so it's not entirely clear to me how you want the tox check to run the same checks as the GitHub actions. The GitHub actions are a superset of tox; eventually I expect things to shift even further in GitHub's favor (i.e. once we get make check to work.)

In any case, I don't see how we can get the regex linter run on Windows out of the box. Do you have any suggestions how to fix this?

[birkenfeld]

Regexlint doesn't need make, it's just an external package and you call a script contained there.

I guess you could use regexlint as a library and create a "test" that calls it. It's an abuse of unit tests though, in my opinion.

Or does tox have a facility for integration of other types of checks?

[kurtmckee]

Yes, I've created an initial patch that runs on Windows. Tox supports running arbitrary commands and treating their return codes as a pass/fail so no need for regexlint to be part of the unit tests. It'll simply be a different tox environment that gets run.

[Anteru]

Ah my bad, I thought it required some Linux-ism to run. Should we include regexlint as a git submodule and run it through tox? It seems like a welcome addition (and potentially a better path forward for moving the other make check calls to?)

[kurtmckee]

No need to make it a git submodule. Tox can specify it as a dependency to be installed, and pip supports checking out and installing a package from a git repo if that's required (though regexlint is available PyPI).

…

On September 27, 2020 4:57:14 PM UTC, "Matthäus G. Chajdas" ***@***.***> wrote: Ah my bad, I thought it required some Linux-ism to run. Should we include `regexlint` as a git submodule and run it through `tox`? It seems like a welcome addition (and potentially a better path forward for moving the other `make check` calls to?) -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #1556 (comment)

[Anteru]

Ah, perfect. Let me look into this and see what I can do.

[kurtmckee]

Regarding migrating commands out of make, tox might not be the right tool for arbitrary build or release commands. For example, tox is probably not the right tool for managing the build of the Pygments demo.

doit may be more appropriate -- it has strong support for specifying input file dependencies and output files, for skipping steps if the inputs and outputs aren't going to change, and running arbitrary commands. I've found it to be an powerful tool for automating builds and releases.

[birkenfeld]

There's no need to replace make; even if tox runs the linter (which I approve, of course) the standalone make target is still useful.

[Anteru]

I was mostly hoping to reduce duplication, i.e. have make check call tox going forward, once all tests are there -- but that depends on how easy it is to run arbitrary commands in tox.

I don't want to remove the make files wholesale; only the tests if that's feasible.

[birkenfeld]

Sure, as long as you can still call the different things individually (i.e. make check, make regexlint etc.)

As for regexlint, I'd prefer to keep using our fork instead of upstream, since we can easily tweak some rules there when needed.

[kurtmckee]

I updated #1557 to depend on Pygments' regexlint git master branch.

regexlint depends on Pygments, so tox is installing the latest Pygments release in the testing environment. I think this is probably unnecessary, but please evaluate. It may be worthwhile to remove that dependency from regexlint. Here's the output from tox when I ran tox -e lint a moment ago (I manually wrapped the lines):

lint installed:
Pygments @ file:///C:/Users/Kurt/Documents/dev/pygments/.tox/.tmp/package/11/Pygments-2.7.1.dev20200928.zip,
regexlint @ git+https://github.com/pygments/regexlint.git@3d7426ae369f139f2589a13939bd8723be15eacc

[Anteru]

I just tried this locally and it seems to work. I did notice however that changing a rule like true|false|NULL to true|truer|false|NULL does not seem to trigger regexlint any more (I thought it specifically was looking for prefixes in alternative expressions?)

In any case, this seems to work, though it's significantly slower compared to running make regexlint. Timing make regexlint shows that the command takes

$> time make regexlint
# Removed output

real    0m2.655s
user    0m30.383s
sys     0m1.103s

In comparison, tox takes an eternity -- certainly not something I'd want to run locally during development:

$> time tox -e lint
# Removed output

real    1m4.879s
user    0m35.793s
sys     0m9.915s

Frankly I'd call this prohibitively slow. It's definitely not something I'd want make regexlint to use by default.

[birkenfeld]

Uh, that difference should be investigated. It looks like missing parallelism, although user time is significantly higher as well.

[Anteru]

As far as I can tell most of the time is spent in downloading/installing the package. The check itself is not much slower. This could be biased due to me running this on WSL2, but I thought the gap is big enough to not make tox the default path used by make regexlint. I'm not entirely sure what to profile here -- the test themselves seem to take comparable amounts of time.

[birkenfeld]

Got it.

[kurtmckee]

I'll check on the performance issue. You're right that it's all happening in the setup phase prior to actually running regexlint. I think tox supports skipping building a clean environment so they may be a win available here.

[Anteru]

Thanks. My suspicion is that the (dependent) Pygments installation takes most of the time, and WSL2 certainly doesn't help. If it's a one-time cost that's fine for local use, but it won't help the CI as it starts from scratch every time.

[birkenfeld]

There's no reason for the CI to change anyway.