Add OMP critical for make_archive

[zoziha]

Fixes #707 .

Description

When building an archive, it involves generating and deleting the .rsp file. When compiling in parallel, this will cause an error to be reported. It can be solved by adding !$OMP critical.

This PR ensures that when generating the archive file, only a single thread is used, and it is set as a critical process, ensuring that this part of the code is not affected by other threads.

[zoziha]

See .rsp file operation in make_archive.

It would be great if this PR could also be included in 0.6.0. (It is a tiny modification)

[LKedward]

Thanks for fixing this @zoziha - I agree it would be good to get this in for v0.6.0! I encounter this error sporadically but I don't entirely understand the cause. I wonder if the critical section should go in fpm_filesystem::delete_file (around the entire contents) to narrow it down - can you try this to see if it has the same affect as your fix here?

[zoziha]

I wonder if the critical section should go in fpm_filesystem::delete_file (around the entire contents) to narrow it down.

Move the critical section to fpm_filesystem::delete_file, and the problem will still occur in the actual measurement.
From a process point of view, generating .resp files and deleting .resp files is a closed loop. It seems that moving the critical section from fpm_backend::build_target(first commit, eb3cd47) to fpm_compiler::make_archive(second commit, e0f1674) can narrow it down a little bit.

[LKedward]

Thanks for testing that out, okay yeah that's interesting. The reason I'm perplexed is that there should only be one archive object and hence only one thread that ever executes that code. So surely a critical section shouldn't have any effect? I'm now wondering if the critical section is indirectly fixing the issue by introducing a delay or synchronisation.

[zoziha]

I can't really understand this issue. It stands to reason that only one archive file needs to be created in the build_target loop, and only one thread should be executing.
Tested in a limited number of examples, it seems that #707 is only occurred generally on Windows; and tested on WSL2, no critical section is added and no error is occurred. (Perhaps this could be a bug only in Windows-GFortran-Openmp?)

However, in any case, this PR ensures that when generating the archive file, only a single thread is used, and it is set as a critical process, ensuring that this part of the code is not affected by other threads.

I encounter this error sporadically.

@LKedward , do you also encounter this problem on Windows?

[LKedward]

@LKedward , do you also encounter this problem on Windows?

Sorry yes, I only encounter it on Windows (msys2). So I think you're right that it might be a bug with the Windows filesystem or with mingw64.

@awvwgk - how important is it that we delete the response file, could we simply leave it in place to avoid this error?

[awvwgk]

how important is it that we delete the response file, could we simply leave it in place to avoid this error?

Preferably we can remove temporary files we create, especially if they are not part of the overall build state (like object files or the digests). But if it is easier, we can just leave it around.

[zoziha]

During the use of these days, this PR has indeed avoided the occurrence of #707, and similar errors have not occurred again on my computer, but indeed this may be a bug related to Msys2-gfortran-openmp, the specific reason is unknown.

There is often only one archive file that needs to be created at present, and it is acceptable to set a critical section for it, which makes opening the .resp file and deleting the .resp file form a whole. I don't really understand the actual effect of this PR :\

Since it's a tiny modification, we can improve it in the future when the real reason is found, also because it's small enough to let it pass now if we can.

[LKedward]

Thanks both. Okay good point, in that case I'm happy to move forward with this. Can you add a comment to the function to explain the need for the critical section and perhaps reference the Github issue number?

[awvwgk]

Looks good to me.