Skip to content

[dotnet] only embed the git revision in SourceLink metadata when stamping - #18004

Merged
titusfortner merged 1 commit into
SeleniumHQ:trunkfrom
titusfortner:dotnet-sourcelink-stamp
Sep 8, 2026
Merged

[dotnet] only embed the git revision in SourceLink metadata when stamping#18004
titusfortner merged 1 commit into
SeleniumHQ:trunkfrom
titusfortner:dotnet-sourcelink-stamp

Conversation

@titusfortner

Copy link
Copy Markdown
Member

🔗 Related Issues

Regressed from #17467, which has SourceLink metadata read the git revision from the workspace status file on every build; this prevented RBE from being able to cache test results, and results in every single RBE run executing all .NET tests regardless of changes (~120 RBE executor minutes)

💥 What does this PR do?

Prevents dotnet assemblies and tests from being rebuilt and rerun on every CI job. The SourceLink metadata now embeds the git revision only when stamping, so unstamped builds are cacheable again.

🔧 Implementation Notes

  • csharp_sourcelink_library gains a stamp attribute that defaults to the --stamp flag through a config_setting, the same pattern rules_pkg uses. Release builds already pass --stamp, so published packages keep the real commit; everything else gets the existing HEAD fallback.

🤖 AI assistance

  • AI assisted (complete below)
    • Tool(s): Claude Code (Fable 5.1)
    • What was generated: the CI log analysis that identified the cache miss, the fix, and this description
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Bug fix (backwards compatible)

@selenium-ci selenium-ci added C-dotnet .NET Bindings B-build Includes scripting, bazel and CI integrations labels Sep 8, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Gate .NET SourceLink revisions on Bazel stamping

🐞 Bug fix ⚙️ Configuration changes 🕐 10-20 Minutes

Grey Divider

AI Description

• Gates SourceLink git revision embedding behind Bazel stamping.
• Uses HEAD metadata for unstamped builds, restoring remote cache reuse.
• Preserves exact commit links for stamped release artifacts.
Diagram

graph TD
  A["Bazel Build"] --> B["Stamp Config"] --> C["SourceLink Rule"] --> D{"Stamped?"}
  D -->|Yes| E["Git Revision"] --> G["SourceLink JSON"] --> H["C# Assemblies"]
  D -->|No| F["HEAD Fallback"] --> G
Loading
High-Level Assessment

The selected approach is appropriate: it preserves revision-accurate SourceLink data for stamped releases while removing workspace-status dependencies from ordinary builds. A stable HEAD fallback retains SourceLink functionality without forcing assembly and test cache invalidation, and callers can still override the stamp attribute explicitly.

Files changed (2) +35 / -1

Bug fix (1) +29 / -1
sourcelink.bzlAvoid workspace revision inputs for unstamped SourceLink builds +29/-1

Avoid workspace revision inputs for unstamped SourceLink builds

• Adds a stamp attribute and macro that defaults to the global Bazel stamping flag. Unstamped builds write stable HEAD-based metadata directly, while stamped builds continue reading the exact revision from workspace status.

dotnet/private/sourcelink.bzl

Other (1) +6 / -0
BUILD.bazelExpose Bazel stamping as a private .NET configuration target +6/-0

Expose Bazel stamping as a private .NET configuration target

• Adds a package-visible config setting that detects when Bazel stamping is enabled. The SourceLink macro uses this setting to derive its default stamp behavior.

dotnet/private/BUILD.bazel

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Grey Divider


Action required

1. Build caching can silently regress 📘 Rule violation ☼ Reliability
Description
csharp_sourcelink_library now selects between HEAD metadata and workspace-status revision
metadata without a focused automated test. A later change to either stamping branch could make every
.NET assembly and test depend on the current revision again or omit the revision from published
metadata without detection.
Code

dotnet/private/sourcelink.bzl[R15-18]

+    if not ctx.attr.stamp:
+        ctx.actions.write(
+            output = output,
+            content = '{{"documents":{{"*":"{repo}/raw/HEAD/*"}}}}\n'.format(
Evidence
Compliance rule 5 requires focused regression coverage for changed behavior. The cited
implementation introduces distinct stamped and unstamped metadata paths, but the PR adds no test
exercising either path.

AGENTS.md: Add Focused Tests for Behavioral Changes and Fixes
dotnet/private/sourcelink.bzl[13-30]

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

## Issue description
Add focused regression coverage for SourceLink metadata generation in stamped and unstamped builds.

## Issue Context
The unstamped path must generate a `HEAD` URL without reading workspace status, while the stamped path must embed the stable Git revision and retain its existing fallback behavior.

## Fix Focus Areas
- dotnet/private/sourcelink.bzl[13-30]
- dotnet/private/sourcelink.bzl[102-125]

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


Grey Divider

Context sources
✅ Web pages:
  +2 more
Review mode: ⚖️ Balanced: This changes Bazel rule behavior and stamping/configuration semantics across .NET builds, so a careful review is warranted despite the localized diff.

Grey Divider

Tip of the day
💡 Did you know, you can tweak Display preferences with a live preview to see your comment before it ships

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread dotnet/private/sourcelink.bzl
@titusfortner titusfortner added this to the 4.49 milestone Sep 8, 2026
@titusfortner
titusfortner merged commit c756287 into SeleniumHQ:trunk Sep 8, 2026
26 checks passed
This was referenced Sep 9, 2026
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants