gkdr / libomemo

Implements OMEMO in C.
MIT License
55 stars 16 forks source link

Makefile: Fix possible race condition in test* targets #31

Closed fortysixandtwo closed 3 years ago

fortysixandtwo commented 3 years ago

When running the build in parallel moving '.g' may fail because files got moved by another build job

At least that's what I suspect happens. I came across this because CI pipelines were randomly (about 1 in 4 tmes in CI) failing at the mv *.g* $(TDIR) command. So I added a couple of ls to figure the issue out. See f.e. https://salsa.debian.org/DebianOnMobile-team/libomemo/-/jobs/1343392#L1204 The problem in a nutshell can be seen here:

ls -la
total 155
drwxrwxr-x 7 root root  4096 ene 14  2021 .
drwxr-xr-x 3 root root  4096 ene 14  2021 ..
drwxr-xr-x 2 root root  4096 ene 14  2021 build
-rw-rw-r-- 1 root root  1175 dic  3  2020 CHANGELOG.md
drwxrwxr-x 7 root root  4096 ene 14  2021 debian
-rw-rw-r-- 1 root root    36 dic  3  2020 .gitignore
-rw-rw-r-- 1 root root  1071 dic  3  2020 LICENSE
-rw-rw-r-- 1 root root  5258 ene 14  2021 Makefile
drwxrwxr-x 3 root root  4096 ene 14  2021 .pc
-rw-rw-r-- 1 root root  1636 dic  3  2020 README.md
drwxrwxr-x 2 root root  4096 dic  3  2020 src
drwxrwxr-x 2 root root  4096 ene 14  2021 test
-rw-r--r-- 1 root root 13648 ene 14  2021 test_libomemo.gcda
-rw-r--r-- 1 root root 80408 ene 14  2021 test_storage.gcno
ls *.g*
test_libomemo.gcda
test_storage.gcno
ls -l test
total 524
-rw-rw-r-- 1 root root   3236 dic  3  2020 test_crypto.c
-rw-r--r-- 1 root root    928 ene 14  2021 test_crypto.gcda
-rw-r--r-- 1 root root  10112 ene 14  2021 test_crypto.gcno
-rwxr-xr-x 1 root root  55288 ene 14  2021 test_crypto.o
-rw-rw-r-- 1 root root  55581 dic  3  2020 test_libomemo.c
-rw-r--r-- 1 root root 141940 ene 14  2021 test_libomemo.gcno
-rwxr-xr-x 1 root root 245344 ene 14  2021 test_libomemo.o
-rw-rw-r-- 1 root root   5866 dic  3  2020 test_storage.c
mv *.g* test
ls -la
total 58
drwxrwxr-x 7 root root 4096 ene 14  2021 .
drwxr-xr-x 3 root root 4096 ene 14  2021 ..
drwxr-xr-x 2 root root 4096 ene 14  2021 build
-rw-rw-r-- 1 root root 1175 dic  3  2020 CHANGELOG.md
drwxrwxr-x 7 root root 4096 ene 14  2021 debian
-rw-rw-r-- 1 root root   36 dic  3  2020 .gitignore
-rw-rw-r-- 1 root root 1071 dic  3  2020 LICENSE
-rw-rw-r-- 1 root root 5258 ene 14  2021 Makefile
drwxrwxr-x 3 root root 4096 ene 14  2021 .pc
-rw-rw-r-- 1 root root 1636 dic  3  2020 README.md
drwxrwxr-x 2 root root 4096 dic  3  2020 src
drwxrwxr-x 2 root root 4096 ene 14  2021 test
ls *.g*
ls: no se puede acceder a '*.g*': No existe el fichero o el directorio

Having rerun the CI pipeline (because I haven't had the problem on my machine) a few times with the patch applied I never got a failed build.

gkdr commented 3 years ago

thanks! i never ran this in parallel, which is probably why i never had this problem. never really thought about it, but mv does actually error if there is nothing to move, interesting. and execing via find doesn't?

(i think the core problem is that everything gets run in the main dir, so it could also be solved by compiling the tests to and running them from the test dir so nothing needs to be moved after. what do you think about that? i'll merge this anyway since it was very hacky in the first place :grimacing: )

fortysixandtwo commented 3 years ago

find exits with 0/doesn't error out if it it doesn't find anything.

Your suggestion seems like a good idea. I just went the low resistance path, mainly because I didn't fully understand and didn't want to break the coverage reports :)

fortysixandtwo commented 3 years ago

I just noticed an issue with this PR. While building it did move my .git/ and .gitignore into the tests directory which confused me lots ;) Updated the MR