Skip to content

[dotnet] [build] Format solution instead of projects#17457

Closed
nvborisenko wants to merge 13 commits into
SeleniumHQ:trunkfrom
nvborisenko:dotnet-format-solution
Closed

[dotnet] [build] Format solution instead of projects#17457
nvborisenko wants to merge 13 commits into
SeleniumHQ:trunkfrom
nvborisenko:dotnet-format-solution

Conversation

@nvborisenko
Copy link
Copy Markdown
Member

2x faster

💥 What does this PR do?

Use SDK with setting up workspace once, instead calling it one by one per project.

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s):
    • What was generated:
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Cleanup (formatting, renaming)

@selenium-ci selenium-ci added C-dotnet .NET Bindings B-build Includes scripting, bazel and CI integrations labels May 14, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Format solution file instead of individual projects for faster builds

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Format entire solution file instead of individual projects
• Significantly improves build performance by 2x
• Simplifies formatting logic in both Unix and Windows scripts
• Eliminates per-project iteration overhead
Diagram
flowchart LR
  A["Per-project formatting<br/>Unix + Windows loops"] -- "Refactor to" --> B["Solution-level formatting<br/>Selenium.slnx"]
  B -- "Result" --> C["2x faster build<br/>Simplified scripts"]
Loading

Grey Divider

File Changes

1. dotnet/private/dotnet_format.bzl ✨ Enhancement +4/-14

Replace per-project formatting with solution-level formatting

• Replaced Unix shell loop that iterates over individual .csproj files with single dotnet format
 call on Selenium.slnx
• Replaced Windows batch loops for src and test directories with single format command on solution
 file
• Updated echo messages to reflect solution-level formatting instead of per-project formatting
• Removed redundant project discovery and iteration logic

dotnet/private/dotnet_format.bzl


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 14, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (3)

Grey Divider


Action required

1. Windows %* passed unsafely 📘 Rule violation ⛨ Security ⭐ New
Description
The Windows script forwards %* directly into the dotnet format command, which can be interpreted
by cmd.exe if arguments contain metacharacters like & or |. This violates the safe argument
handling requirement for build/CI scripts.
Code

dotnet/private/dotnet_format.bzl[91]

+"%DOTNET%" format %* Selenium.slnx || exit /b 1
Evidence
PR Compliance ID 14 requires safe argument handling in scripts. The Windows batch invocation uses
%* directly, which is not safely escaped/validated before being parsed by cmd.exe.

dotnet/private/dotnet_format.bzl[91-91]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Windows wrapper uses `%*` directly in a command invocation, which is unsafe in `cmd.exe` because metacharacters in forwarded arguments can alter command parsing.

## Issue Context
This script is an executable entrypoint; forwarded args should be either disallowed, strictly validated/whitelisted, or passed via a safer mechanism (e.g., only known flags, response file, or controlled argument construction).

## Fix Focus Areas
- dotnet/private/dotnet_format.bzl[91-91]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Workspace arg after options 🐞 Bug ≡ Correctness ⭐ New
Description
dotnet_format appends Selenium.slnx after all forwarded args ($@ / %*), so the solution path
can be consumed as the value for an option that expects a parameter (e.g., --diagnostics) instead
of being treated as the workspace. This can make dotnet format run against an unintended workspace
or fail due to mis-parsed arguments.
Code

dotnet/private/dotnet_format.bzl[56]

+"$DOTNET" format "$@" Selenium.slnx || exit 1
Evidence
The wrapper currently appends Selenium.slnx after the forwarded args in both the Unix and Windows
scripts. The repo also demonstrates passing options that require values (e.g., `--diagnostics
<ID>`), making this argument placement ambiguous and prone to being parsed as an option value rather
than the workspace.

dotnet/private/dotnet_format.bzl[53-56]
dotnet/private/dotnet_format.bzl[88-91]
rake_tasks/dotnet.rake[119-132]
dotnet/.editorconfig[55-59]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The wrapper currently runs `dotnet format` with forwarded args first and the workspace (`Selenium.slnx`) last. If the last forwarded argument is an option that requires a value, `Selenium.slnx` can be treated as that value instead of the workspace.

## Issue Context
This target is invoked by other tooling (e.g., rake tasks / format scripts) and forwards arbitrary args to `dotnet format`.

## Fix Focus Areas
- dotnet/private/dotnet_format.bzl[55-56]
- dotnet/private/dotnet_format.bzl[90-91]

## Suggested change
Update both scripts to place the workspace immediately after `format`:
- Unix: `"$DOTNET" format Selenium.slnx "$@" || exit 1`
- Windows: `"%DOTNET%" format Selenium.slnx %* || exit /b 1`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Selenium.slnx path unquoted 📘 Rule violation ☼ Reliability ⭐ New
Description
The formatter scripts pass Selenium.slnx without quoting it as a path argument. This violates the
script safety guidance to quote paths and can cause brittle parsing if the path ever changes (e.g.,
spaces).
Code

dotnet/private/dotnet_format.bzl[56]

+"$DOTNET" format "$@" Selenium.slnx || exit 1
Evidence
PR Compliance ID 14 requires safe argument handling, including quoting paths in scripts. The Unix
and Windows dotnet format invocations include Selenium.slnx as a bare, unquoted path argument.

dotnet/private/dotnet_format.bzl[56-56]
dotnet/private/dotnet_format.bzl[91-91]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The scripts invoke `dotnet format` with an unquoted solution path argument (`Selenium.slnx`). Compliance requires quoting paths to avoid parsing issues.

## Issue Context
This file generates both Unix and Windows wrapper scripts. Even if `Selenium.slnx` currently has no spaces, quoting path arguments is a required safety practice for build/CI scripts.

## Fix Focus Areas
- dotnet/private/dotnet_format.bzl[56-56]
- dotnet/private/dotnet_format.bzl[91-91]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Selenium.slnx not validated 📘 Rule violation ☼ Reliability
Description
The new formatter script always runs dotnet format against a fixed Selenium.slnx path without
first validating the file exists, so failures may be non-deterministic and harder to diagnose
depending on workspace layout. Add an explicit existence check with a clear error message before
invoking dotnet format.
Code

dotnet/private/dotnet_format.bzl[R55-56]

+echo "Running dotnet format $@ on Selenium.slnx..."
+"$DOTNET" format "$@" "$DOTNET_DIR/Selenium.slnx" || exit 1
Evidence
The checklist requires early validation of external/config-derived inputs with deterministic
exceptions. The Unix and Windows scripts now reference a fixed Selenium.slnx path directly in the
dotnet format invocation without an explicit existence check or tailored error message.

dotnet/private/dotnet_format.bzl[55-56]
dotnet/private/dotnet_format.bzl[90-91]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The formatter scripts unconditionally run `dotnet format` on `Selenium.slnx` without validating that the solution file exists, which can lead to unclear failures when the workspace root is unexpected or the file is missing.

## Issue Context
This PR changed formatting from per-project `.csproj` iteration to a single solution file (`Selenium.slnx`). With this change, the solution file becomes a required input derived from environment/workspace state and should be validated early with a deterministic, actionable error.

## Fix Focus Areas
- dotnet/private/dotnet_format.bzl[55-56]
- dotnet/private/dotnet_format.bzl[90-91]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Formatting scope can drift 🐞 Bug ⚙ Maintainability
Description
//dotnet:format now formats only the projects listed in dotnet/Selenium.slnx, so any new
.csproj added under dotnet/src or dotnet/test but not added to the solution will silently stop
being formatted/linted. This can let style/whitespace regressions slip through because
scripts/format.* and rake dotnet:format rely on this Bazel target as the formatter entrypoint.
Code

dotnet/private/dotnet_format.bzl[R55-56]

+echo "Running dotnet format $@ on Selenium.slnx..."
+"$DOTNET" format "$@" "$DOTNET_DIR/Selenium.slnx" || exit 1
Evidence
The formatter entrypoint now targets a single solution file, and that solution file hard-codes which
projects exist; therefore formatting coverage depends on keeping that list in sync with the actual
set of .csproj files.

dotnet/private/dotnet_format.bzl[53-56]
dotnet/Selenium.slnx[1-11]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`dotnet/private/dotnet_format.bzl` switched from discovering all `*.csproj` under `dotnet/src` and `dotnet/test` to formatting a static solution file (`dotnet/Selenium.slnx`). If a new project is added but the solution is not updated, `//dotnet:format` will no longer format that project and the failure mode is silent.

## Issue Context
- The new scripts call `dotnet format ... dotnet/Selenium.slnx`.
- `Selenium.slnx` is an explicit allowlist of project paths.

## Fix approach
Add a lightweight guard before invoking `dotnet format`:
- Enumerate `src/**/*.csproj` and `test/**/*.csproj`.
- Verify each path is present in `Selenium.slnx` (exact match on the `<Project Path="..." />` entries).
- If any are missing, print a clear error listing the missing projects and exit non-zero.
- Implement the same guard in both the Unix and Windows script bodies.

(Alternative: auto-generate a temporary solution file from the discovered csproj list and run `dotnet format` on that generated solution, to preserve the performance win while avoiding manual sync.)

## Fix Focus Areas
- dotnet/private/dotnet_format.bzl[30-69]
- dotnet/private/dotnet_format.bzl[71-104]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 62eaf58

Results up to commit 6b3669a


🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)


Remediation recommended
1. Selenium.slnx not validated 📘 Rule violation ☼ Reliability
Description
The new formatter script always runs dotnet format against a fixed Selenium.slnx path without
first validating the file exists, so failures may be non-deterministic and harder to diagnose
depending on workspace layout. Add an explicit existence check with a clear error message before
invoking dotnet format.
Code

dotnet/private/dotnet_format.bzl[R55-56]

+echo "Running dotnet format $@ on Selenium.slnx..."
+"$DOTNET" format "$@" "$DOTNET_DIR/Selenium.slnx" || exit 1
Evidence
The checklist requires early validation of external/config-derived inputs with deterministic
exceptions. The Unix and Windows scripts now reference a fixed Selenium.slnx path directly in the
dotnet format invocation without an explicit existence check or tailored error message.

dotnet/private/dotnet_format.bzl[55-56]
dotnet/private/dotnet_format.bzl[90-91]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The formatter scripts unconditionally run `dotnet format` on `Selenium.slnx` without validating that the solution file exists, which can lead to unclear failures when the workspace root is unexpected or the file is missing.

## Issue Context
This PR changed formatting from per-project `.csproj` iteration to a single solution file (`Selenium.slnx`). With this change, the solution file becomes a required input derived from environment/workspace state and should be validated early with a deterministic, actionable error.

## Fix Focus Areas
- dotnet/private/dotnet_format.bzl[55-56]
- dotnet/private/dotnet_format.bzl[90-91]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Formatting scope can drift 🐞 Bug ⚙ Maintainability
Description
//dotnet:format now formats only the projects listed in dotnet/Selenium.slnx, so any new
.csproj added under dotnet/src or dotnet/test but not added to the solution will silently stop
being formatted/linted. This can let style/whitespace regressions slip through because
scripts/format.* and rake dotnet:format rely on this Bazel target as the formatter entrypoint.
Code

dotnet/private/dotnet_format.bzl[R55-56]

+echo "Running dotnet format $@ on Selenium.slnx..."
+"$DOTNET" format "$@" "$DOTNET_DIR/Selenium.slnx" || exit 1
Evidence
The formatter entrypoint now targets a single solution file, and that solution file hard-codes which
projects exist; therefore formatting coverage depends on keeping that list in sync with the actual
set of .csproj files.

dotnet/private/dotnet_format.bzl[53-56]
dotnet/Selenium.slnx[1-11]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`dotnet/private/dotnet_format.bzl` switched from discovering all `*.csproj` under `dotnet/src` and `dotnet/test` to formatting a static solution file (`dotnet/Selenium.slnx`). If a new project is added but the solution is not updated, `//dotnet:format` will no longer format that project and the failure mode is silent.

## Issue Context
- The new scripts call `dotnet format ... dotnet/Selenium.slnx`.
- `Selenium.slnx` is an explicit allowlist of project paths.

## Fix approach
Add a lightweight guard before invoking `dotnet format`:
- Enumerate `src/**/*.csproj` and `test/**/*.csproj`.
- Verify each path is present in `Selenium.slnx` (exact match on the `<Project Path="..." />` entries).
- If any are missing, print a clear error listing the missing projects and exit non-zero.
- Implement the same guard in both the Unix and Windows script bodies.

(Alternative: auto-generate a temporary solution file from the discovered csproj list and run `dotnet format` on that generated solution, to preserve the performance win while avoiding manual sync.)

## Fix Focus Areas
- dotnet/private/dotnet_format.bzl[30-69]
- dotnet/private/dotnet_format.bzl[71-104]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 14, 2026

Persistent review updated to latest commit 62eaf58

Comment thread dotnet/private/dotnet_format.bzl Outdated
"%DOTNET%" format %* "%%p" || exit /b 1
)
echo Running dotnet format %* on Selenium.slnx...
"%DOTNET%" format %* Selenium.slnx || exit /b 1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

1. Windows %* passed unsafely 📘 Rule violation ⛨ Security

The Windows script forwards %* directly into the dotnet format command, which can be interpreted
by cmd.exe if arguments contain metacharacters like & or |. This violates the safe argument
handling requirement for build/CI scripts.
Agent Prompt
## Issue description
The Windows wrapper uses `%*` directly in a command invocation, which is unsafe in `cmd.exe` because metacharacters in forwarded arguments can alter command parsing.

## Issue Context
This script is an executable entrypoint; forwarded args should be either disallowed, strictly validated/whitelisted, or passed via a safer mechanism (e.g., only known flags, response file, or controlled argument construction).

## Fix Focus Areas
- dotnet/private/dotnet_format.bzl[91-91]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

"$DOTNET" format "$@" "$proj" || exit 1
done || exit 1
echo "Running dotnet format $@ on Selenium.slnx..."
"$DOTNET" format "$@" Selenium.slnx || exit 1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Action required

2. Workspace arg after options 🐞 Bug ≡ Correctness

dotnet_format appends Selenium.slnx after all forwarded args ($@ / %*), so the solution path
can be consumed as the value for an option that expects a parameter (e.g., --diagnostics) instead
of being treated as the workspace. This can make dotnet format run against an unintended workspace
or fail due to mis-parsed arguments.
Agent Prompt
## Issue description
The wrapper currently runs `dotnet format` with forwarded args first and the workspace (`Selenium.slnx`) last. If the last forwarded argument is an option that requires a value, `Selenium.slnx` can be treated as that value instead of the workspace.

## Issue Context
This target is invoked by other tooling (e.g., rake tasks / format scripts) and forwards arbitrary args to `dotnet format`.

## Fix Focus Areas
- dotnet/private/dotnet_format.bzl[55-56]
- dotnet/private/dotnet_format.bzl[90-91]

## Suggested change
Update both scripts to place the workspace immediately after `format`:
- Unix: `"$DOTNET" format Selenium.slnx "$@" || exit 1`
- Windows: `"%DOTNET%" format Selenium.slnx %* || exit /b 1`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@nvborisenko nvborisenko marked this pull request as draft May 14, 2026 13:15
@nvborisenko
Copy link
Copy Markdown
Member Author

I love bazel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations C-dotnet .NET Bindings Compliance violation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants