Repository navigation
WIN-24578 :: Add Rider plugin support for .net projects - #19
agorielikov wants to merge 8 commits into
Conversation
udif2025
left a comment
There was a problem hiding this comment.
Solid, well-documented PR - the NativeBuild refactor is clean and the -graph / -nodeReuse:false / UseSharedCompilation=false reasoning is great. No blockers; the one I'd like addressed before merge is how Build Selected Projects handles referenced projects' configuration/platform (comment on projectsBuild). The rest are smaller points and nits.
| # RR = RustRover, IU = IntelliJ IDEA Ultimate, RD = Rider (the .NET actions - | ||
| # see rider-support.xml). Community editions (IC) don't carry | ||
| # com.intellij.modules.ultimate, which the Rust plugin requires there. | ||
| $targetProductCodes = @("RR", "IU", "RD") |
There was a problem hiding this comment.
Nit: is there a Linux installer script that needs Rider added too, or is this .ps1 the only one?
| dependencies { | ||
| intellijPlatform { | ||
| // Same 2026.2 (262) line as the root module's other compile targets. | ||
| rider("2026.2") { |
There was a problem hiding this comment.
The rider module is compiled against 2026.2 only, and the README says Rider 2026.2. If the plugin's since-build still allows 261, Rider 2026.1 will load rider-support.xml and may fail on Rider-internal APIs (isExistingSolution, solutionFilterPath, ...). Either add Rider 2026.1 to the Verifier IDEs or make sure the compatibility range / installer keeps it out.
| } | ||
| val exitCode = processHandler.exitCode | ||
| val (content, type) = when { | ||
| stopRequested -> "${presentation.contentName} was stopped." to NotificationType.INFORMATION |
There was a problem hiding this comment.
stopRequested is only set when the build is cancelled from this status-bar progress. Stopping it via the Stop Build action or the tool window ends up here as "failed (exit code -1)" with an ERROR balloon. Consider checking whether the handler was destroyed / sharing a flag with Stop Build.
| } | ||
| cmdLines.append("\r\n") | ||
| if (index < build.commands.lastIndex) { | ||
| cmdLines.append("if errorlevel 1 exit /b %ERRORLEVEL%\r\n") |
There was a problem hiding this comment.
if errorlevel 1 only catches exit codes >= 1, so a negative one (e.g. a crashed dotnet host) lets the chain carry on. if not "%ERRORLEVEL%"=="0" exit /b %ERRORLEVEL% (or || exit /b) is safer.
| .append(" --caption ").append(shellSingleQuoted(project.name)) | ||
| script.append("exec ").append(shellSingleQuoted(consolePath)) | ||
| .append(" --caption ").append(shellSingleQuoted(caption)) | ||
| .append(" --ib-quiet --build-cache-local-shared --build-cache-basedir=\"\$PWD\" -- ") |
There was a problem hiding this comment.
Nit: this still passes --build-cache-local-shared --build-cache-basedir for MSBuild builds, though caching isn't supported for .NET yet. Harmless, or should they be dropped for MSBuild?
| return "\"" + escaped + "\\".repeat(trailingBackslashes) + "\"" | ||
| } | ||
|
|
||
| private const val CMD_SPECIAL_CHARACTERS = "&|<>^()" |
There was a problem hiding this comment.
Nit: cmdQuoted now also quotes &|<>^() and doubles trailing backslashes. That changes the CMake/CLion command line too - looks correct, but worth a quick CLion sanity run.
|
|
||
| internal object DotNetBuildRunner { | ||
|
|
||
| fun build(project: Project, request: DotNetBuildRequest, actionType: String) { |
There was a problem hiding this comment.
Nit: nothing stops a second Build from starting while one is still running on the same solution - two MSBuilds in one tree will hit file-lock failures. Fine if Rust/CMake behave the same today.
| * One MSBuild invocation per project, run one after the other: MSBuild only accepts a single | ||
| * project per invocation. Each one is still parallel within itself (see [COMMON_MSBUILD_ARGUMENTS]). | ||
| */ | ||
| internal fun projectsBuild( |
There was a problem hiding this comment.
Building each project directly with -p:Configuration=... -p:Platform=... means those global properties also flow to its ProjectReferences (outside a solution build MSBuild doesn't unset them). So every referenced project is built with the selected project's mapped config|platform rather than its own solution mapping - e.g. App -> Release|x64, Lib -> Release|AnyCPU: Lib gets built as x64 into a different bin\ than Rider's build, and can bring back the plugin-vs-Rider rebuild churn e02c494 fixed.
Suggestion: build through the solution with project targets instead, e.g. App.sln -t:"Folder\App;Lib" (solution-folder path is available from Rider's entity tree or the .sln). The solution then maps every project correctly, and multiple selected projects build in one parallel invocation instead of sequentially. If you prefer the current approach, at least worth testing on a mixed-platform solution.
| */ | ||
| internal object MsBuildLocator { | ||
|
|
||
| fun locate(): MsBuildLocation { |
There was a problem hiding this comment.
Nit: locate() runs vswhere on every build (up to a 15s timeout). Could cache the result per session and reset it when the setting changes.
udif2025
left a comment
There was a problem hiding this comment.
Overall the design is solid: the Rider adapter only exposes platform types, the MSBuild flag choices are well reasoned, vswhere requires the .NET SDK component, cmdQuoted handles operator characters and trailing backslashes, and stopped builds are reported as stopped. CI is green.
The main issue is the first inline comment: -graph most likely breaks Build Selected Projects, and nothing here tests it against a real MSBuild. The version/embedded-zip mismatch should also be fixed before merge. The rest are smaller suggestions.
Review drafted with Claude Code.
| * only affect this one command-line invocation. | ||
| */ | ||
| internal val COMMON_MSBUILD_ARGUMENTS = listOf( | ||
| "-graph", |
There was a problem hiding this comment.
Likely bug: -graph breaks Build Selected Projects. projectsBuild passes the solution's per-project targets (-t:src\App;My_Lib), but in static-graph mode MSBuild doesn't build the solution metaproject. It expands the .sln into entry points for every project and applies the requested targets to each project directly. So App.csproj would be asked for a target named src\App and fail with MSB4057, and it wouldn't be limited to the selection either.
The PR description only mentions whole-solution testing. Could you run this action on a real solution with a project inside a solution folder? If it fails, I'd drop -graph for projectsBuild, since the graph-mode rationale above is about whole-solution builds anyway.
(I couldn't run MSBuild to confirm, so this is from how graph builds handle .sln entry points.)
There was a problem hiding this comment.
Ran it on a real solution (App → My.Lib in a src solution folder, plus an unrelated root Other project) with both dotnet msbuild 17.14.43 and VS 2022 MSBuild 17.14.40, using the exact projectsBuild arguments including -graph. -t:src\App, -t:src\My_Lib and -t:src\My_Lib;Other all exit 0 and build only the selected projects plus their references (Other is untouched unless selected). The static graph does load every project in the .sln, but graph mode then maps the solution's per-project targets to those projects rather than passing them down, so no MSB4057. Keeping -graph for projectsBuild.
| @@ -129,7 +131,7 @@ if ($anyRunning) { | |||
| } | |||
|
|
|||
| $pluginZipBase64 = @' | |||
There was a problem hiding this comment.
The version isn't bumped but the embedded build changed. gradle.properties is still version = 1.0.3, while this blob now contains a different intellij-ib-plugin-1.0.3.jar (I decoded it and it includes the Rider/dotnet classes). That leaves two different builds labelled 1.0.3 in the field. Please bump the version and move the CHANGELOG [Unreleased] entries under it.
Also, a ~200 KB locally built blob can't be verified in review or reproduced from CI. Consider generating it in the release workflow instead of committing it.
There was a problem hiding this comment.
Bumped the version to 1.1.0.
- gradle.properties is now 1.1.0. I chose a minor bump because this release adds CLion and Rider support; switch it to 1.0.4 if you'd rather.
- In CHANGELOG.md, the former Unreleased entries now sit under ## [1.1.0], with an empty ## [Unreleased] above.
- A fresh build produces intellij-ib-plugin-1.1.0.jar, so two different builds no longer share the 1.0.3 label.
| // org.rust only loads through rust-support.xml. Confirmed by the Verifier's own "Missing | ||
| // dependencies: com.jetbrains.rust (optional): Unavailable" against Rider. (The CMake lines above | ||
| // don't apply to Rider: it bundles com.intellij.cmake itself.) | ||
| com.incredibuild.ibplugin:Package 'org\.rust' is not found |
There was a problem hiding this comment.
This ignore is global, so it also hides real org.rust resolution problems when verifying against RustRover/IDEA, not just Rider. Can it be scoped to the Rider target? If not, a RustRover-only verification run without this line would keep that coverage.
There was a problem hiding this comment.
Scoped to Rider now.
| request.kind, | ||
| request.configuration, | ||
| request.platform, | ||
| restore = needsRestore(readSolutionProjectFiles(request.solutionFile)) |
There was a problem hiding this comment.
When a .slnf is open, entryPoint is the filter but this reads the full .sln. One never-restored project that the filter excludes then adds -restore to every filtered build, which is the restore churn COMMON_MSBUILD_ARGUMENTS says it avoids. Suggest reading the filter's project list when entryPoint is a .slnf.
There was a problem hiding this comment.
The restore check now reads only the filter's own projects when a .slnf is open, so a project the filter leaves out can no longer add -restore to every filtered build.
| .append(" --caption ").append(shellSingleQuoted(project.name)) | ||
| script.append("exec ").append(shellSingleQuoted(consolePath)) | ||
| .append(" --caption ").append(shellSingleQuoted(caption)) | ||
| .append(" --ib-quiet --build-cache-local-shared --build-cache-basedir=\"\$PWD\" -- ") |
There was a problem hiding this comment.
Now that this path is shared with MSBuild, the build-cache flags go to .NET builds on Linux too. The PR says caching doesn't support .NET yet. Should these be left off for MSBuild builds, or have you confirmed they're harmless?
There was a problem hiding this comment.
Good catch, and it applied on Windows too: the shared native path was also sending /BUILDCACHELOCAL=ON to MSBuild builds. I hadn't verified the cache was harmless for .NET, so rather than rely on that, NativeBuild now has a buildCache flag. It stays on for CMake and is off for both MSBuild builders, so .NET builds get no --build-cache-* options on Linux and no /BUILDCACHELOCAL on Windows. Tests cover both wrappers and both builders. We can turn it back on once Build Cache supports .NET.
| * two quick clicks can't both get past it. A Rebuild's chained build is unaffected: its clean | ||
| * has already ended by the time the build starts. | ||
| */ | ||
| private fun confirmNoBuildRunning(project: Project): Boolean { |
There was a problem hiding this comment.
This guard now applies to the Rust and CMake builds too, which is a behaviour change worth a CHANGELOG line. Two smaller points:
- After "Stop Running Build" the new build still isn't started, so the user has to click again. Consider starting it once the stop completes.
- Builds accelerated through
IncredibuildProjectTaskRunner(the IDE build pipeline) aren't tracked byisBuildRunning(), so they can still overlap with a tool-window build.
There was a problem hiding this comment.
Thanks, all three addressed:
CHANGELOG: Added under 1.1.0 → Changed. Only one Incredibuild build runs at a time, in every IDE. Starting a second one offers to stop the running build, and before this change two builds could run side by side with the second replacing the first's tool-window output.
Start after stop: "Stop Running Build" now starts the new build automatically once the old process has actually exited, never while it's still winding down. run() now reports through onStarted / onNotStarted callbacks instead of returning the handler, since the start can now happen later. When the old process ends, the guard runs again before starting, in case something else started in between. Cancel behaves as before: the build doesn't start, and an IDE-pipeline build reports ABORTED. The dialog now reads "Stop it and start this one once it has stopped?". The menu builds, the CLion/Rider IDE builds and Rebuild's chained cargo clean all go through this unchanged.
Pipeline overlap: Half right. Pipeline builds were already tracked, because IncredibuildProjectTaskRunner creates its process through createProcessHandler, which registers it as the active handler. So isBuildRunning() and Stop Build saw it, and a menu build started during a pipeline build was already caught. The gap was the other direction: the pipeline never checked before starting, so it could start on top of a running menu build. It now calls a new blocking awaitNoBuildRunning() before each build, and before Rebuild's cargo clean. That shows the same stop prompt, waits for the stop to complete, and reports ABORTED if the user cancels.
| } | ||
| val target = when (request.kind) { | ||
| DotNetBuildKind.BUILD_PROJECTS -> | ||
| request.projectFiles.joinToString(", ") { File(it).nameWithoutExtension } |
There was a problem hiding this comment.
Nit: with many projects selected, this makes the banner and progress title very long. Consider capping it, e.g. App, Lib +12 more.
There was a problem hiding this comment.
Done. The banner now shows at most two names plus a count, e.g. App, Lib +12 more. Up to three are still listed in full, since "+1 more" wouldn't save anything. The progress title wasn't affected: it shows the solution name, not the selection. Covered by a unit test.
| return | ||
| } | ||
|
|
||
| val selection = RiderSolutionModel.selection(e.dataContext) |
There was a problem hiding this comment.
Nit: the selection is computed for Build/Rebuild Solution outside the context menu too, where it isn't used. You could compute it only for BUILD_PROJECTS or isFromContextMenu.
There was a problem hiding this comment.
Done. update() now reads the selection only in the context-menu branch and for Build Selected Projects. On the menu path it sits after the kind != BUILD_PROJECTS || check, so Build/Rebuild Solution skip it entirely. No behaviour change.
|
|
||
| ### Added | ||
|
|
||
| - CLion support for CMake projects: **Build**, **Rebuild** and **Build All** in the **Incredibuild** menu run |
There was a problem hiding this comment.
plugin.xml already described CLion support before this PR. Was this entry already released? If so, it shouldn't be under [Unreleased].
There was a problem hiding this comment.
With new version all of it was moved out of unreleased and under 1.1.0
Add Rider plugin support, which still installs from same zip file as CLion and RustRover.
Distribution works as expected, tested on a build with 100 sub-project libraries.
Caching doesn't work as .net is not supported yet.