Skip to content

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Aug 30, 2025

This reverts commit c8d6ef5, again: The same change was already attempted in commit e8519e4 ("[roottest] Remove sequential compilation with Ninja.") and reverted in bab6bef which also holds some explanations and pointers why Ninja cannot be launched concurrently.

This reverts commit c8d6ef5, again:
The same change was already attempted in commit e8519e4 ("[roottest]
Remove sequential compilation with Ninja.") and reverted in bab6bef
which also holds some explanations and pointers why Ninja cannot be
launched concurrently.
Copy link

Test Results

    20 files      20 suites   3d 20h 38m 55s ⏱️
 3 648 tests  3 641 ✅ 0 💤 7 ❌
71 278 runs  71 266 ✅ 5 💤 7 ❌

For more details on these failures, see this check.

Results for commit 60e42f1.

Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I was only checking the git history of the roottest repository, so I missed the reverts that already happened in this repository. Sorry!

If you want to revert this again why not. I can just stop using ninja But still, I don't see the advantage of this revert and wouldn't do it.

  • Having this RUN_SERIAL slows down things so much that ninja becomes unusable for development purposes when you like to validate your changes by running roottest. And you can't work around it without changing the source code. Or you see a way?

  • The problem that you describe with doing an incremental build when running ctest makes sense, but it can easily be avoided by always building before running ctest, like we do in the CI, so ctest in run on a "clean" state

If you really want that we support this pattern of doing ctest straight from a dirty source directory, I'm sure we can find another solution without compromising parallel testing when using Ninja.

Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not bring this back for the ROOTTEST_COMPILE_MACRO at least, because there it's easy to avoid the problem. For the other cases, we can see later how building CMake targets can be avoided during test time.

Also, please consider adding a short comment in the code explaining why this is is necessary, so that in the future nobody removed these code blocks anymore as long as we build CMake targets during testing.

@hahnjo
Copy link
Member Author

hahnjo commented Aug 31, 2025

Having this RUN_SERIAL slows down things so much that ninja becomes unusable for development purposes when you like to validate your changes by running roottest. And you can't work around it without changing the source code. Or you see a way?

As I already wrote in #19766 (comment), Stephan has been working on #19702 which would "[prevent] only concurrent Ninja invocations, but otherwise [increase] parallelism".

The problem that you describe with doing an incremental build when running ctest makes sense, but it can easily be avoided by always building before running ctest, like we do in the CI, so ctest in run on a "clean" state

Most of the times yes, and that's my standard procedure (ninja && ctest -j12). But it happens to me every now and then that something decides it needs to run cmake again, even though ninja went through.

If you really want that we support this pattern of doing ctest straight from a dirty source directory, I'm sure we can find another solution without compromising parallel testing when using Ninja.

I don't want to actively support it, but as written above it sometimes happens spuriously. In addition, it can happen by mistake and it is really frustrating to end up with a fully corrupt build directory. I have this right now in my main build directory, crashing with LLVM assertions due to corrupt files, and even I couldn't figure out which file I have to delete to "save" my directory. This is just a huge waste of time and hinders development.

@hahnjo hahnjo dismissed guitargeek’s stale review August 31, 2025 09:31

This PR is a pure revert of commit c8d6ef5 and not the place to discuss other changes.

@hahnjo hahnjo requested a review from guitargeek August 31, 2025 09:32
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I understand the underlying problem and the possible solutions now, and I can follow up another time to avoid these forced serial tests

@hahnjo hahnjo merged commit 6812cd7 into root-project:master Sep 1, 2025
25 of 29 checks passed
@hahnjo hahnjo deleted the revert-ninja branch September 1, 2025 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants