Skip to content

Race condition in parallel automake build with checksrc and lib1521.c #15258

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

Closed
dfandrich opened this issue Oct 10, 2024 · 2 comments
Closed

Comments

@dfandrich
Copy link
Contributor

I did this

This build failed with ./lib1521.c:1:1: error: Missing copyright statement (COPYRIGHT) despite the mk-lib1521.pl script being designed to write a copyright statement at the beginning of the file.

I expected the following

There should be no such false error.

I suspect what's happening is that the mk-lib1521.pl and checksrc targets are running at the same time and in parallel, and since the former creates the lib1521.c file using shell redirection, an empty lib1521.c is created immediately, before the script even starts to run. Meanwhile, checksrc sees the .c file, doesn't find a copyright line inside and raises an error. If it checked a few milliseconds later, it would have been fine.

I'm not sure the best way to fix this. Some possibilities that came to mind:

  • Making checksrc wait until the test target is done would work, unless tests are not being built at all. That would also make the checksrc test happen at the end, after everything else is built.
  • Adding an explicit make dependency on lib1521.c would work, but it will spuriously break again the next time a new generated file is added and adding the new dependency is forgotten.
  • Giving checksrc an explicit ignore list would work, but has the same problem.
  • Changing mk-lib1521.pl to create the lib1521.c file internally would reduce the window where there is a problem, but not eliminate it.
  • Making checksrc ignore empty files would probably usually work, but it's possible on some platforms a buffer's worth of data could be flushed into the file before checksrc checks it, which makes the file non-empty but contains the start of the copyright header only and not the whole thing, which would still cause it to fail.
  • Creating lib1521.c in two stages, writing it first to a temporary file (not ending in .c) then renaming it to the final name after it's written. This makes the run jobs inconsistent (because checksrc will sometimes check lib1521.c and sometimes not) but with reproduceable results (because the checksrc check, if run, will always succeed).

Despite the slight imperfection of the latter solution, I see it as being the cleanest.

curl/libcurl version

curl 8.11.0-DEV

operating system

Ubuntu 22.04

@bagder
Copy link
Member

bagder commented Oct 10, 2024

I propose this:

index f9c44f0dd..be7fd5d9d 100644
--- a/tests/libtest/Makefile.am
+++ b/tests/libtest/Makefile.am
@@ -116,11 +116,11 @@ CHECKSRC = $(CS_$(V))
 CS_0 = @echo "  RUN     " $@;
 CS_1 =
 CS_ = $(CS_0)
 
 checksrc:
-       $(CHECKSRC)@PERL@ $(top_srcdir)/scripts/checksrc.pl -D$(srcdir) $(srcdir)/*.[ch]
+       $(CHECKSRC)@PERL@ $(top_srcdir)/scripts/checksrc.pl -D$(srcdir) "-W$(CLEANFILES)" $(srcdir)/*.[ch]
 
 if DEBUGBUILD
 # for debug builds, we scan the sources on all regular make invokes
 all-local: checksrc
 endif

@dfandrich
Copy link
Contributor Author

dfandrich commented Oct 10, 2024 via email

bagder added a commit that referenced this issue Oct 17, 2024

Verified

This commit was signed with the committer’s verified signature.
bagder Daniel Stenberg
By a rename from .tmp to .c once completed.

Reported-by: Dan Frandrich
Fixes #15258
@bagder bagder closed this as completed in 2ae8d9b Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants