reformatting failed due to src/plugins/specload/specload/remove.quickdump

[markus2330]

https://build.libelektra.org/blue/organizations/jenkins/libelektra/detail/PR-3678/4/pipeline

113/215 Test #131: testscr_check_formatting ......................***Failed    0.09 sec



ELEKTRA CHECK FORMATTING



————————————————————————————————————————————————————————————

Warning — Reformatting command `reformat-c` failed



ClangFormat:   

Version Info:  0.9.4

Major Version: 

Please install clang-format 11

————————————————————————————————————————————————————————————



————————————————————————————————————————————————————————————

Warning — Reformatting command `reformat-markdown` failed



Please install `npx` (Included in npm >= 5.2.0)

————————————————————————————————————————————————————————————



————————————————————————————————————————————————————————————

Warning — Reformatting command `reformat-javascript` failed



Please install `npx` (Included in npm >= 5.2.0)

————————————————————————————————————————————————————————————



————————————————————————————————————————————————————————————

Warning — Reformatting command `reformat-cmake` failed



cmake-format: 

Version:      

Please install `cmake-format` version 0.6.3

————————————————————————————————————————————————————————————



————————————————————————————————————————————————————————————

Warning — Reformatting command `reformat-java` failed



ClangFormat:   

Version Info:  0.9.4

Major Version: 

Please install clang-format  or later

————————————————————————————————————————————————————————————



————————————————————————————————————————————————————————————

Warning — Reformatting command `reformat-shell` failed



shfmt:   

Version: 

Please install `shfmt` version 2

————————————————————————————————————————————————————————————



error: The reformatting check detected code that **does not** fit the guidelines given in `doc/CODING.md`.

If you see this message on one of the build servers, you can either install one or multiple of the following tools:



- [`clang-format`](https://clang.llvm.org/docs/ClangFormat.html) to format C, C++ and Java source code,

- [`cmake_format`](https://github.com/cheshirekow/cmake_format) to format CMake code,

- [`prettier`](https://prettier.io) to format JavaScript & Markdown code, and

- [`shfmt`](https://github.com/mvdan/sh) to format Shell code



. Afterwards you can use the following scripts to fix the formatting problems



- `reformat-c` to format C/C++ source files,

- `reformat-cmake` to format CMake files,

- `reformat-java` to format Java files,

- `reformat-javascript` to format JavaScript files,

- `reformat-markdown` to format Markdown files, and

- `reformat-shell` to format files that contain shell code



. If you do not want to install any of the tools listed above you can also use the `patch` command after this message

to fix the formatting problems. For that please



1. copy the lines between the long dashes (`—`),

2. store them in a file called `format.patch` **in the root of the repository**



. After that use the following command to apply the changes:



    sh -c '

    line_prefix="$(head -n1 format.patch | sed -nE '"'"'s/(^[0-9]+:).*/\1_/p'"'"' | wc -c | sed -E '"'"'s/[ ]*//g'"'"')"

    { test "$line_prefix" -gt 1 && cut -c"$line_prefix"- format.patch || cat format.patch ; } | patch -p1

    '



.





————————————————————————————————————————————————————————————



diff --git a/src/plugins/specload/specload/remove.quickdump b/src/plugins/specload/specload/remove.quickdump

index ccedf5000..e69de29bb 100644

Binary files a/src/plugins/specload/specload/remove.quickdump and b/src/plugins/specload/specload/remove.quickdump differ



————————————————————————————————————————————————————————————



No local changes to save

testscr_check_formatting.sh RESULTS: 1 test(s) done 1 error(s).

[mpranj]

Confirming this happens sporadically. Initially only on the Alpine builds, now also on other instances.

[markus2330]

Maybe we can exclude binary files from passing to the reformatters? Or we define a black list?

[mpranj]

I do not think we should add a blacklist for this specific reason.

I suspect that testscr_check_formatting sometimes runs parallel to testmod_specload, and testmod_specload possibly modifies the file and then restores from backup. I think would be better if we do not modify anything inside the source folders during the tests.

If the approach above is too much work it should also be possible to exclude these specific files from the git diff.

[markus2330]

Yes, good observation, that sounds very plausible.

I agree that tests should not modify files in the source tree (but write to temporary files instead).

As temporary solution we could mark testscr_check_formatting to be not executed in parallel? Or is testmod_specload really the only test which writes into the source tree?

[mpranj]

This is starting to occur more often. I marked check_formatting not to run in parallel with any other tests in febf6cd350321a4e2021f90371a2d6bced525565 as a quick fix.

Real fix to be done: febf6cd350321a4e2021f90371a2d6bced525565 should be reverted and testmod_specload should not modify files in the source tree.

[stale[bot]]

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

[markus2330]

@kodebach can you describe what needs to be done here to make this a FLOSS issue?

[kodebach]

1. I disagree with @mpranj and think that not running check_formatting in parallel with anything else is appropriate. Since check_formatting does a diff, we it should run alone to ensure nothing else is making modifications, even if they are temporary.
2. However, I do agree that tests (or in fact CI in general) shouldn't write to source files. IMO we should actually make the entire source tree read-only in CI jobs. Only for specific jobs like check_formatting should the source be writeable and those things should IMO run as completely separate CI jobs.
3. Regarding the specific specload issue. I think by implementing #4444 this issue will go away. At that point specload would be a completely read-only backend plugin and therefore would never write to any files.

[markus2330]

IMO we should actually make the entire source tree read-only in CI jobs. Only for specific jobs like check_formatting should the source be writeable and those things should IMO run as completely separate CI jobs.

Good idea but I am afraid it is hard to implement. Simply letting check_formatting run in parallel and from time to time get failures if builds/tests do insane things (like modifications of the source) is the much more simple thing to do.

I think by implementing https://github.com/ElektraInitiative/libelektra/issues/4444 this issue will go away.

Obviously a correct reimplementation would fix every issue. But I think we shouldn't wait for such events, as it might never happens.

@kodebach Probably, in this case we should simply remove the offending test case (that makes these changes in the source repo).

Then, we can revert https://github.com/ElektraInitiative/libelektra/commit/febf6cd350321a4e2021f90371a2d6bced525565 or, if an easy implementation exists, make build jobs have a read-only source tree, to detect/prevent further occurrences of such problems.

[kodebach]

Probably, in this case we should simply remove the offending test case

I think that would be most of the testmod_specload tests, since they are mostly their to check the "spec protection" features, i.e. which parts of the spec can be changed...

If we want to avoid getting more untested code, we could also make specload read-only and but the keep the rest. So just remove the set function and anything that relies on it.

[markus2330]

Yes, good idea.

So this task is:

1. make specload readonly (remove setter)
2. remove its tests
3. renable reformatting in parallel https://github.com/ElektraInitiative/libelektra/commit/febf6cd350321a4e2021f90371a2d6bced525565

[kodebach]

remove its tests

Only remove the tests that check setter functionality (test_add, test_edit, test_remove) and the parts of other tests that test kdbSet. There should still be basic tests to make sure kdbGet works.

I think there are also some tests (e.g. testscr_check_gen) and examples that use specload. I don't think any of them use the setter part of specload, but we may need to remove some extra stuff.

[github-actions[bot]]

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

[github-actions[bot]]

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart: