Comparing changes

So far, we have used "black" for formatting the test code, which is rather strict and opinionated and when used inline in Nix expressions it creates all sorts of trouble. One of the main annoyances is that when using strings coming from Nix expressions (eg. store paths or option definitions from NixOS modules), completely unrelated changes could cause tests to fail, since eg. black wants lines to be broken. Another downside of enforcing a certain kind of formatting is that it makes the Nix expression code inconsistent because we're mixing two spaces of indentation (common in nixpkgs) with four spaces of indentation as defined in PEP-8. While this is perfectly fine for standalone Python files, it really looks ugly and inconsistent IMO when used within Nix strings. What we actually want though is a linter that catches problems early on before actually running the test, because this is *actually* helping in development because running the actual VM test takes much longer. This is the reason why I switched from black to pyflakes, because the latter actually has useful checks, eg. usage of undefined variables, invalid format arguments, duplicate arguments, shadowed loop vars and more. Signed-off-by: aszlig <aszlig@nix.build> Closes: #72964

(cherry picked from commit a2c9220568648b4528154ebd8e657add243ed0b4)

Our test driver exposes a bunch of variables and functions, which pyflakes doesn't recognise by default because it assumes that the test script is executed standalone. In reality however the test driver script is using exec() on the testScript. Fortunately pyflakes has $PYFLAKES_BUILTINS, which are the attributes that are globally available on all modules to be checked. Since we only have one module, using this environment variable is fine as opposed to my first approach to this, which tried to use the unstable internal API of pyflakes. The attributes are gathered by the main derivation of the test driver, because we don't want to end up defining a new attribute in the test driver module just to being confused why using it in a test will result in an error. Another way we could have gathered these attributes would be in mkDriver, which is where the linting takes place. However, we do have a different set of Python dependencies in scope and duplicating these will again just cause confusion over having it at one location only. Signed-off-by: aszlig <aszlig@nix.build> Co-Authored-By: aszlig <aszlig@nix.build>

Note: I didn't execute it entirely because I'd have to build chromium for this, but the diff appears fine.

The new linter basically does def testScript # ... before calling `pyflakes`. As this test-script is empty, it would lead to a syntax-error unless `pass` is added.

Linter error was: f-string is missing placeholders Signed-off-by: aszlig <aszlig@nix.build>

Linter error: use ==/!= to compare constant literals (str, bytes, int, float, tuple) Signed-off-by: aszlig <aszlig@nix.build>

Signed-off-by: aszlig <aszlig@nix.build>

Linter errors reported: 6:32 f-string is missing placeholders 7:26 f-string is missing placeholders 8:32 f-string is missing placeholders 30:32 f-string is missing placeholders 31:26 f-string is missing placeholders 32:32 f-string is missing placeholders 48:32 f-string is missing placeholders 49:26 f-string is missing placeholders 50:32 f-string is missing placeholders 76:32 f-string is missing placeholders 77:26 f-string is missing placeholders 78:32 f-string is missing placeholders Signed-off-by: aszlig <aszlig@nix.build>

Signed-off-by: aszlig <aszlig@nix.build>

There were a bunch of unnecessary f-strings in there and I also removed the "# fmt: on/off" comments, because we no longer use Black and thus won't need those comments anymore. Signed-off-by: aszlig <aszlig@nix.build>

@roberth

This switches the linting of the NixOS test driver script from Black (which is a code *formatter*) to PyFlakes. Contrary to Black, which only does formatting and a basic syntax check, PyFlakes actually performs useful checks early on before spinning up VMs and evaluating the actual test script and thus becomes actually useful in development rather than an annoyance. One of the reasons why Black has been an annoyance[1] was because it assumed that the files that it's formatting aren't inlined inside another programming language. With NixOS VM tests however, we inline these Python scripts in the testScript attribute. With some of them using string antiquotations, things are getting rather ugly because Black now no longer formats static code but generated code from Nix being used as a macro language. This becomes especially annoying when an antiquotation contains an option definition from the NixOS module system, since an unrelated change might change its length or contents (eg. suddenly containing a double quote) for which Black will report an error. While issue #72964 has been sitting around for a while (and probably annoyed everyone involved), nobody has actually proposed an implementation until @roberth did a first pull request[2] yesterday which added a "skipFormatter" attribute, which contrary to skipLint silently disabled Black. This has led to very legitimate opposition[3] from @flokli: > As of now, this only adds an option that does exactly the same as the > already existing one. > > black does more than linting, yes. Last September it was proposed to > switch from black to to a more permissive (only-)linter. > > I don't think adding another option (skipFormatter) that currently > does exactly the same as skipLint will help out of this confusion. > > IMHO, we should keep skipLint, but use a linter that doesn't format, > at least not enforce the line length (due to the nix interpolation we > do). This was written while I was doing an alternative implementation and pretty much sums up the work I'm merging here, which switches to PyFlakes, which only checks for various errors in the code (eg. undefined variables, shadowing, wrong comparisons and more) but does not do *any* formatting. Since Black didn't do any of the checks performed by PyFlakes (except a basic syntax check), the existing test scripts needed to be fixed. Thanks to @blaggacao, @Ma27 and @roberth for helping with testing and fixing those scripts. [1]: #72964 [2]: #122197 [3]: #122197 (review)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comparing changes

Open a pull request

Commits on May 9, 2021

This comparison is taking too long to generate.