-
Notifications
You must be signed in to change notification settings - Fork 189
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
Add file locking to support parallel runs. #427
Conversation
My fork's Actions have returned this error while executing error while running 'cmake --build build/doctest -- -j1':
In file included from /home/runner/work/CPM.cmake/CPM.cmake/examples/doctest/main.cpp:3:
/home/runner/work/CPM.cmake/CPM.cmake/build/doctest/_deps/doctest-src/doctest/doctest.h:4299:47: error: size of array ‘altStackMem’ is not an integral constant-expression
4299 | static char altStackMem[4 * SIGSTKSZ];
| ^
gmake[2]: *** [CMakeFiles/CPMExampleDoctest.dir/build.make:76: CMakeFiles/CPMExampleDoctest.dir/main.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:132: CMakeFiles/CPMExampleDoctest.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2 However, I am unable to look into this problem because my local development environment is unable to build even default CPM's master branch or the latest release. Building the CMake Error at /home/<user>/Documents/Programming/C++/CPM.cmake/test/cmake-build-debug/fetchcontent_dependency/_deps/fibonacci-src/cmake/CPM.cmake:34 (if):
5: if given arguments:
5:
5: "NOT" "/home/<user>/Documents/Programming/C++/CPM.cmake/cmake" "MATCHES" "/home/<user>/Documents/Programming/C++/CPM.cmake/test/cmake-build-debug/fetchcontent_dependency/_deps/fibonacci-src/cmake"
5:
5: Regular expression
5: "/home/<user>/Documents/Programming/C++/CPM.cmake/test/cmake-build-debug/fetchcontent_dependency/_deps/fibonacci-src/cmake"
5: cannot compile
5: Call Stack (most recent call first):
5: /home/<user>/Documents/Programming/C++/CPM.cmake/test/cmake-build-debug/fetchcontent_dependency/_deps/fibonacci-src/CMakeLists.txt:10 (include) I would like to fix this problem, but I don't know where to being. It seems as if these For reference, line 34 of this if(NOT ${CPM_DIRECTORY} MATCHES ${CMAKE_CURRENT_LIST_DIR}) Looking through the rest of the file makes it obvious that it is completely different from any of the other current ones. Even the copyright comment is different saying As for the first problem, it seems unrelated to my change, but I suppose it must be because it must not be in the master branch. I am just a little confused and would appreciate guidance. |
This should only be done when |
Only synchronize if CPM_SOURCE_CACHE is defined.
I did implement only locking the download directory but that caused half of the integration tests to fail. I do not know enough about how those work or what they check for to be able to debug them, so I reverted back to locking the |
What! Why does it do anything different here? All of the environments behave differently! I probably just have things setup wrong. Thanks for looking into this. Should I try to make locking just the download directory work, or will this be merged as-is when someone gets a chance to review it? |
TBH I don't have much experience with CMake locks, so I don't know what edge cases to look out for (e.g. will things be unlocked if CMake errors/crashes etc?) in this PR. @iboB do you have more experience and could take over? |
If CMake crashes then the locks will be released. The locks only exist within the scope of the process according to CMake's documentation.
|
It uses file locking so it cannot possibly outlive the CMake process. It's fairly reliable (I have come to use it to secure the auto generation of in-source files). As for locking more locally, I meant the directory right above the download one (which is module specific but not version specific). Obviously generating it in the download directory itself will lead to a dirty git directory which can cause troubles as CPM does not attempt a checkout on dirty git repos. Keep in mind the cmake.lock files are not cleaned up after the fact. |
OK. I misunderstood your suggestion the first time. These commits should move the locked file into the per-package directory and remove it when it is no longer needed. |
What do I need to do to keep this moving forward? Should I merge the recent additions to CPM into my fork? |
OK. I may have to pass this on to someone else. I think I have a decent start here but I cannot debug this. None of my environments behave like the one here does. The only time I am getting meaningful feedback is when the workflows are run here, and that is not conducive to debugging. Furthermore, I don't know anything about the error that is being raised here or why it did not appear before, or why it did appear before on my fork's workflow. |
@PercentBoat4164 it seems that the GitHub actions runner has been updated to use a new version of compiler tools that are no longer compatible with some of the example projects being built in the test suite. This should be soon resolved in #430 which should hopefully solve the issues you are seeing in the workflows. |
Oh! That is amazing to hear. Thank you. I am glad that I am not going insane. Is there anything that I need to do to prepare for this change? |
@PercentBoat4164 I like this but I cannot see any additional unit-test to check the added behavior? This may be tricky but as CPM is being used by many more developers we need to ensure features don't get broken inadvertently by the next person who comes along 👍 As an additional 'idea' all packages are currently satisfied sequentially and parallelism is inherently lost. A future enhancement could be to 'somehow' check the lock with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not an expert on how CMake handles file locking, but from what I've read this commit looks good.
Kudos for making an integration test.
I pushed some changes to how the requested integration test lib change is handled. I think its better to be explicit in such scenarios. Also the new arg name
can be useful in other scenarios
* keyword args * 'name' arg to allow multiple projects from the same test
- Fixed small grammatical errors.
Oops. Sorry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks again for implementing this feature and for your patience and changes!
This feature is now released in |
This should fix some issues pointed out by other users. Specifically #424, #371. and #293. Only CPMAddPackage() was changed. Changes should be made anywhere else where the cache is accessed.
Thanks to @Banderi for providing the idea for this solution.