Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 31, 2021

Conversation

fortysixandtwo
Copy link
Contributor

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
Copy link
Owner

gkdr commented Jan 15, 2021

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 😬 )

@fortysixandtwo
Copy link
Contributor Author

fortysixandtwo commented Jan 15, 2021

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
Copy link
Contributor Author

fortysixandtwo commented Jan 15, 2021

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

When running the build in parallel moving '*.g*'
may fail because files got moved by another build job
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants