Repository navigation
Conversation
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
|
Good evening. CI failed downloading |
|
Retriggered. @betamaxbandit can you review this? |
|
@jonahgraham this one is quite a big code reorg, tried to replace three mutable |
|
I like the idea of having a mutable record here. Thanks for cleaning that up. @betamaxbandit and @DangMinhTam382 are the most knowledgeable and active people in this area, so I hope they can review. |
| return Status.OK_STATUS; | ||
| } | ||
| try { | ||
| setActiveBuildConfig(selection, monitor); |
There was a problem hiding this comment.
This looks fragile and unnecessary nested: we schedule a Job that runs synchronous workspace modification inside it. What about WorkspaceJob?
Also please have a look at this comment
There was a problem hiding this comment.
Thanks, good points. I reworked the PR:
- The tracker job is now a WorkspaceJob with the workspace root rule, like before this PR.
- I dropped the CoreModel lock entirely, which also answers your question on Fix deadlock for CBuildConfigurationManager and CoreBuildLaunchBarTracker #1534. It was only used by five copies of the same "read, change, set the project description" code. The root rule does the same, and setDescription needs it anyway. With one lock there is no lock order, so the deadlock can't happen.
- One helper in CBuildConfigurationManager now does this change. The tracker, CBuildConfiguration.setActive(), CoreBuildLaunchConfigDelegate.buildForLaunch() and the Docker launcher's buildForLaunch() all use it. The helper takes the root rule itself, because the other callers don't run inside a workspace operation. In the tracker job that's just a nested run under the same rule.
- The tracker job now uses only the mode and target of its own change, which fixes the mixed state.
About combining createBuildConfiguration with addBuildConfiguration: I agree, but it changes ICBuildConfigurationManager API, so I'd rather do it in a separate PR.
Drop the CoreModel lock around project description changes and rely on the workspace root rule only, so there is no lock order to get wrong. One helper in CBuildConfigurationManager now makes these changes for the launch bar tracker, CBuildConfiguration.setActive() and the Core Build and Docker buildForLaunch(). The tracker job now uses only the mode and target of its own launch bar change, and is a WorkspaceJob. Fixes eclipse-cdt#1542 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7ddbdf0 to
642faa2
Compare
Fixes #1542
Why
CoreBuildLaunchBarTrackerhas two problems:CoreModel.CBuildConfiguration.setActive()andCoreBuildLaunchConfigDelegate.buildForLaunch()(the launch bar Build button and "build before launch") lockCoreModelfirst and then wait for the rule insetDescription. This is the same deadlock as Deadlock between CoreBuildLaunchBarTracker job and CBuildConfigurationManager.createBuildConfiguration() #1533; Fix deadlock for CBuildConfigurationManager and CoreBuildLaunchBarTracker #1534 fixes it only forcreateBuildConfiguration.Fix
CoreModellock. It was only used to make read, change and set of the project description one step, in five copies of the same code. The workspace root rule does that too, andsetDescriptionneeds it anyway. With only one lock, there is no lock order to get wrong.CBuildConfigurationManagerdoes it under the root rule.createBuildConfiguration, the tracker,CBuildConfiguration.setActive(),CoreBuildLaunchConfigDelegate.buildForLaunch()and the DockerContainerLaunchConfigurationDelegate.buildForLaunch()use it.WorkspaceJob.org.eclipse.cdt.debug.coreandorg.eclipse.cdt.docker.launcherare added as friends oforg.eclipse.cdt.internal.core.build. There is no API change.Tests
New
CoreBuildLaunchBarTrackerTests, run locally on Linux:mainquickChangesDoNotCreateConfigsForUnselectedCombinationsdebug/trackerTestTargetconfiguration is created, but Debug was never selected for that targetsetActiveDoesNotDeadlockWithTrackersetActive()and the tracker job deadlockCBuildConfigurationManagerTests,DefaultBuildConfigurationNameTests,DefaultBuildConfigurationNameTestsNoToolchainsandICBuildConfigurationProviderTestsalso pass.This PR changes
createBuildConfigurationlike #1534 does, so whichever is merged second needs a small rebase.🤖 Generated with Claude Code