[cdac] Create transport package and rename the cdac reader - #114812

Merged
steveisok merged 17 commits into
dotnet:mainfrom
steveisok:cdac-packaging
Apr 25, 2025
Merged

[cdac] Create transport package and rename the cdac reader#114812
steveisok merged 17 commits into
dotnet:mainfrom
steveisok:cdac-packaging

Conversation

@steveisok

@steveisoksteveisok commented Apr 18, 2025

Copy link
Copy Markdown
Member

This change adds new cdac transport packages that are built for Windows, MacOS, and Linux. Each package contains a host specific native cdac library that is meant to ride along with various diagnostics tools.

The cdacreader was also renamed to cdac as part of this change. I picked what I thought was the best suggestion in #108720 (comment). Feel free to make alternate suggestions.

Contributes to #108720

CopilotAI review requested due to automatic review settings April 18, 2025 02:49
@ghostghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 18, 2025
CopilotAI reviewed Apr 18, 2025
View reviewed changes

CopilotAI left a comment

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.

Pull Request Overview

This PR introduces new cdac transport packages for Windows, MacOS, and Linux and renames the cdacreader to cdac.

  • Updated comments and macro definitions in native library code to reflect the new cdac naming
  • Adjusted pipeline build arguments and subset names across YAML configuration files to align with the renaming

Reviewed Changes

Copilot reviewed 231 out of 237 changed files in this pull request and generated no comments.

Show a summary per file
FileDescription
src/coreclr/debug/daccess/dacimpl.hUpdated comments to refer to cdac instead of cdacreader
src/coreclr/debug/daccess/cdac.cppRenamed macro definition and updated associated comments
eng/pipelines/runtime.ymlRevised build arguments to use tools.cdac and tools.cdactests
eng/pipelines/runtime-diagnostics.ymlModified build arguments to reference the updated cdac nomenclature
eng/pipelines/common/evaluate-default-paths.ymlChanged subset and include paths to reference cdac instead of cdacreader
Files not reviewed (6)
  • Directory.Build.props: Language not supported
  • eng/Subsets.props: Language not supported
  • eng/liveBuilds.targets: Language not supported
  • src/coreclr/debug/daccess/CMakeLists.txt: Language not supported
  • src/installer/pkg/projects/Microsoft.DotNet.CDac.Transport/Microsoft.DotNet.CDac.Transport.pkgproj: Language not supported
  • src/installer/pkg/projects/cdac-packages.proj: Language not supported

@steveisok
steveisok requested review from a teamApril 18, 2025 02:49
@jkotas

jkotas commented Apr 18, 2025

Copy link
Copy Markdown
Member

The cdacreader was also renamed to cdac as part of this change

The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.

@steveisok

steveisok commented Apr 18, 2025

Copy link
Copy Markdown
MemberAuthor

The cdacreader was also renamed to cdac as part of this change

The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.

I considered that initially and figured I would wait to see how others felt before going there. Thanks for the feedback.

@mikem8361

mikem8361 commented Apr 19, 2025

Copy link
Copy Markdown
Contributor

I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.

@steveisok

steveisok commented Apr 20, 2025

Copy link
Copy Markdown
MemberAuthor

I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.

Yeah, it'll be runtimes/<rid>/native, but looking at that again it probably should be tools/<rid>/<library>

@jkotas

jkotas commented Apr 20, 2025

Copy link
Copy Markdown
Member

tools directory in nuget packages is meant for executables. cdac is a native library. runtimes looks right to me.

@am11am11 added area-Infrastructure and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 21, 2025
@dotnet-policy-service

dotnet-policy-serviceBot commented Apr 21, 2025

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

@jkotas

jkotas commented Apr 21, 2025•
edited
Loading

Copy link
Copy Markdown
Member

CDac name does not follow the .NET naming conventions:

  • It is not an acronym in common usage. (I do not even know what this acronym stands for.) From .NET Framework design guidelines: "In general, it is important to avoid using acronyms in identifier names unless they are in common usage and are immediately understandable to anyone who might use the framework."
  • If we went with the acronym, it should be capitalized as Cdac. From .NET Framework design guidelines: "Acronyms of three or more characters follow the guidelines of any other word. Only the first letter is capitalized".

I think a longer self-descriptive name would be better.

@max-charlamb

max-charlamb commented Apr 21, 2025

Copy link
Copy Markdown
Member

@jkotas, would you be okay with contractdac?

@steveisok

steveisok commented Apr 21, 2025

Copy link
Copy Markdown
MemberAuthor

I think a longer self-descriptive name would be better.
@jkotas, would you be okay with contractdac?

I have no strong opinion. I'll be glad to name it whatever we all think is best.

This was referenced Apr 21, 2025
Closed
Open
Open
Closed
Closed
Open
Closed
@am11

am11 commented Apr 23, 2025

Copy link
Copy Markdown
Member

The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.

The convention used by src/native/libs is <managed namespace>.Native, which produce libraries with same pattern, e.g. libSystem.IO.Ports.Native. This could be applied here as well?

@jkotas

jkotas commented Apr 23, 2025

Copy link
Copy Markdown
Member

System.IO.Ports.Native is a helper library for implementation of System.IO.Ports namespace. The cdacreader is not a helper library like that. cdacreader is a managed library built from https://github.com/dotnet/runtime/tree/main/src/native/managed/cdacreader/src/ that we build with PublishAot. The implementation namespace for this library is Microsoft.Diagnostics.DataContractReader.Legacy;, but that is very long name.

We had more offline conversation about the naming. Stay tuned.

@jkotas

jkotas commented Apr 23, 2025

Copy link
Copy Markdown
Member

Add something like this to https://github.com/dotnet/runtime/blob/main/docs/project/glossary.md:

CDAC Codename for (Data Contracts)[../design/datacontracts/datacontracts_design.md] project

jkotas
jkotas reviewed Apr 23, 2025
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Target.cs
Comment threadsrc/coreclr/debug/daccess/cdac.cpp Outdated
Comment threadeng/Subsets.props Outdated
Comment threadsrc/native/managed/cdac/src/mscordaccore_universal.csproj
Comment threadeng/Subsets.props Outdated
@build-analysisbuild-analysisBot mentioned this pull request Apr 23, 2025
Closed
jkotas
jkotas reviewed Apr 23, 2025
View reviewed changes
Comment threaddocs/project/glossary.md Outdated
@jkotas

jkotas commented Apr 24, 2025

Copy link
Copy Markdown
Member

Microsoft.DotNet.CDAC.Transport.pkgproj

Nit: The casing that follows .NET naming conventions would be Cdac: #114812 (comment)

jkotas
jkotas approved these changes Apr 24, 2025
View reviewed changes

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks

Comment threadDirectory.Build.props

<PropertyGroup>
<DotNetHostBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', '$(OutputRID).$(HostConfiguration)', 'corehost'))</DotNetHostBinDir>
<DotNetCDacBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'mscordaccore_universal', '$(Configuration)', '$(NetCoreAppCurrent)', '$(OutputRID)', 'publish'))</DotNetCDacBinDir>

@jkotasjkotasApr 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: DotNetCDacBinDir -> DotNetCdacBinDir, _BuildCDacPack -> _BuildCdacPack

@steveisoksteveisokApr 25, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Thanks - I'll do that in a quick follow up.

This was referenced Apr 25, 2025
Open
Closed
@steveisok
steveisok merged commit 957aaac into dotnet:mainApr 25, 2025
@steveisok
steveisok deleted the cdac-packaging branch April 25, 2025 11:17
steveisok added a commit to steveisok/runtime that referenced this pull request Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR
f2665d9
Follow up from dotnet#114812 (comment)
@steveisoksteveisok mentioned this pull request Apr 25, 2025
Merged
steveisok added a commit that referenced this pull request Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR (#115047)
3eef331
Follow up from #114812 (comment)
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 27, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jkotasjkotasjkotas approved these changes

Assignees

@steveisoksteveisok

Labels

area-Infrastructure

Projects

Runtime Infra
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@steveisok@jkotas@mikem8361@max-charlamb@am11
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
[cdac] Create transport package and rename the cdac reader - #114812
Merged
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Apr 25, 2025
Merged
[cdac] Create transport package and rename the cdac reader#114812
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Conversation
@steveisok
@steveisoksteveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
This change adds new cdac transport packages that are built for Windows, MacOS, and Linux. Each package contains a host specific native cdac library that is meant to ride along with various diagnostics tools.
The cdacreader was also renamed to cdac as part of this change. I picked what I thought was the best suggestion in #108720 (comment). Feel free to make alternate suggestions.


Contributes to #108720
CopilotAI
review requested due to automatic review settings
April 18, 2025 02:49
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Apr 18, 2025
CopilotAI
reviewed
Apr 18, 2025
View reviewed changes
CopilotAI
left a comment


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.

Pull Request Overview
This PR introduces new cdac transport packages for Windows, MacOS, and Linux and renames the cdacreader to cdac.
  • Updated comments and macro definitions in native library code to reflect the new cdac naming
  • Adjusted pipeline build arguments and subset names across YAML configuration files to align with the renaming
Reviewed Changes
Copilot reviewed 231 out of 237 changed files in this pull request and generated no comments.
Show a summary per file
FileDescription
src/coreclr/debug/daccess/dacimpl.hUpdated comments to refer to cdac instead of cdacreader
src/coreclr/debug/daccess/cdac.cppRenamed macro definition and updated associated comments
eng/pipelines/runtime.ymlRevised build arguments to use tools.cdac and tools.cdactests
eng/pipelines/runtime-diagnostics.ymlModified build arguments to reference the updated cdac nomenclature
eng/pipelines/common/evaluate-default-paths.ymlChanged subset and include paths to reference cdac instead of cdacreader
Files not reviewed (6)
  • Directory.Build.props: Language not supported
  • eng/Subsets.props: Language not supported
  • eng/liveBuilds.targets: Language not supported
  • src/coreclr/debug/daccess/CMakeLists.txt: Language not supported
  • src/installer/pkg/projects/Microsoft.DotNet.CDac.Transport/Microsoft.DotNet.CDac.Transport.pkgproj: Language not supported
  • src/installer/pkg/projects/cdac-packages.proj: Language not supported
@steveisok
steveisok
requested review from
a teamApril 18, 2025 02:49
@jkotas
jkotas
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
@steveisok
steveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

MemberAuthor
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
I considered that initially and figured I would wait to see how others felt before going there. Thanks for the feedback.
@mikem8361
mikem8361
commented
Apr 19, 2025

Copy link


Copy Markdown

Contributor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
@steveisok
steveisok
commented
Apr 20, 2025

Copy link


Copy Markdown

MemberAuthor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
Yeah, it'll be runtimes/<rid>/native, but looking at that again it probably should be tools/<rid>/<library>
@jkotas
jkotas
commented
Apr 20, 2025

Copy link


Copy Markdown

Member
tools directory in nuget packages is meant for executables. cdac is a native library. runtimes looks right to me.
@am11am11
added

area-Infrastructure

and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Apr 21, 2025
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 21, 2025

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/runtime-infrastructure

See info in area-owners.md if you want to be subscribed.
@jkotas
jkotas
commented
Apr 21, 2025•

edited

Loading

Copy link


Copy Markdown

Member
CDac name does not follow the .NET naming conventions:
  • It is not an acronym in common usage. (I do not even know what this acronym stands for.) From .NET Framework design guidelines: "In general, it is important to avoid using acronyms in identifier names unless they are in common usage and are immediately understandable to anyone who might use the framework."
  • If we went with the acronym, it should be capitalized as Cdac. From .NET Framework design guidelines: "Acronyms of three or more characters follow the guidelines of any other word. Only the first letter is capitalized".
I think a longer self-descriptive name would be better.
@max-charlamb
max-charlamb
commented
Apr 21, 2025

Copy link


Copy Markdown

Member
@jkotas, would you be okay with contractdac?
@steveisok
steveisok
commented
Apr 21, 2025

Copy link


Copy Markdown

MemberAuthor
I think a longer self-descriptive name would be better.
@jkotas, would you be okay with contractdac?
I have no strong opinion. I'll be glad to name it whatever we all think is best.

This was referenced Apr 21, 2025

Closed


Open


Open


Closed


Closed


Open


Closed

@am11
am11
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
The convention used by src/native/libs is <managed namespace>.Native, which produce libraries with same pattern, e.g. libSystem.IO.Ports.Native. This could be applied here as well?
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
System.IO.Ports.Native is a helper library for implementation of System.IO.Ports namespace. The cdacreader is not a helper library like that. cdacreader is a managed library built from https://github.com/dotnet/runtime/tree/main/src/native/managed/cdacreader/src/ that we build with PublishAot. The implementation namespace for this library is Microsoft.Diagnostics.DataContractReader.Legacy;, but that is very long name.
We had more offline conversation about the naming. Stay tuned.
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
Add something like this to https://github.com/dotnet/runtime/blob/main/docs/project/glossary.md:
CDAC Codename for (Data Contracts)[../design/datacontracts/datacontracts_design.md] project
jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Target.cs
Comment threadsrc/coreclr/debug/daccess/cdac.cpp
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadsrc/native/managed/cdac/src/mscordaccore_universal.csproj
Comment threadeng/Subsets.props
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Apr 23, 2025

Closed

jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threaddocs/project/glossary.md
Outdated

@jkotas
jkotas
commented
Apr 24, 2025

Copy link


Copy Markdown

Member
Microsoft.DotNet.CDAC.Transport.pkgproj
Nit: The casing that follows .NET naming conventions would be Cdac: #114812 (comment)
jkotas
jkotas
approved these changes
Apr 24, 2025
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM! Thanks
Comment threadDirectory.Build.props

 <PropertyGroup>
 <DotNetHostBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', '$(OutputRID).$(HostConfiguration)', 'corehost'))</DotNetHostBinDir>
 <DotNetCDacBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'mscordaccore_universal', '$(Configuration)', '$(NetCoreAppCurrent)', '$(OutputRID)', 'publish'))</DotNetCDacBinDir>
@jkotasjkotasApr 24, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nit: DotNetCDacBinDir -> DotNetCdacBinDir, _BuildCDacPack -> _BuildCdacPack
@steveisoksteveisokApr 25, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks - I'll do that in a quick follow up.

This was referenced Apr 25, 2025

Open


Closed

@steveisok
steveisok
merged commit 957aaac
into
dotnet:mainApr 25, 2025
@steveisok
steveisok
deleted the

cdac-packaging

branch
April 25, 2025 11:17
steveisok added a commit
to steveisok/runtime
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR
f2665d9
Follow up from dotnet#114812 (comment)
@steveisoksteveisok
mentioned this pull request
Apr 25, 2025
 Merged

steveisok added a commit
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR (#115047)
3eef331
Follow up from #114812 (comment)
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jkotasjkotasjkotas approved these changes

Assignees

@steveisoksteveisok

Labels

area-Infrastructure

Projects

Runtime Infra

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@steveisok@jkotas@mikem8361@max-charlamb@am11

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[cdac] Create transport package and rename the cdac reader - #114812
Merged
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Apr 25, 2025
Merged
[cdac] Create transport package and rename the cdac reader#114812
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Conversation
@steveisok
@steveisoksteveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
This change adds new cdac transport packages that are built for Windows, MacOS, and Linux. Each package contains a host specific native cdac library that is meant to ride along with various diagnostics tools.
The cdacreader was also renamed to cdac as part of this change. I picked what I thought was the best suggestion in #108720 (comment). Feel free to make alternate suggestions.


Contributes to #108720
CopilotAI
review requested due to automatic review settings
April 18, 2025 02:49
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Apr 18, 2025
CopilotAI
reviewed
Apr 18, 2025
View reviewed changes
CopilotAI
left a comment


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.

Pull Request Overview
This PR introduces new cdac transport packages for Windows, MacOS, and Linux and renames the cdacreader to cdac.
  • Updated comments and macro definitions in native library code to reflect the new cdac naming
  • Adjusted pipeline build arguments and subset names across YAML configuration files to align with the renaming
Reviewed Changes
Copilot reviewed 231 out of 237 changed files in this pull request and generated no comments.
Show a summary per file
FileDescription
src/coreclr/debug/daccess/dacimpl.hUpdated comments to refer to cdac instead of cdacreader
src/coreclr/debug/daccess/cdac.cppRenamed macro definition and updated associated comments
eng/pipelines/runtime.ymlRevised build arguments to use tools.cdac and tools.cdactests
eng/pipelines/runtime-diagnostics.ymlModified build arguments to reference the updated cdac nomenclature
eng/pipelines/common/evaluate-default-paths.ymlChanged subset and include paths to reference cdac instead of cdacreader
Files not reviewed (6)
  • Directory.Build.props: Language not supported
  • eng/Subsets.props: Language not supported
  • eng/liveBuilds.targets: Language not supported
  • src/coreclr/debug/daccess/CMakeLists.txt: Language not supported
  • src/installer/pkg/projects/Microsoft.DotNet.CDac.Transport/Microsoft.DotNet.CDac.Transport.pkgproj: Language not supported
  • src/installer/pkg/projects/cdac-packages.proj: Language not supported
@steveisok
steveisok
requested review from
a teamApril 18, 2025 02:49
@jkotas
jkotas
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
@steveisok
steveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

MemberAuthor
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
I considered that initially and figured I would wait to see how others felt before going there. Thanks for the feedback.
@mikem8361
mikem8361
commented
Apr 19, 2025

Copy link


Copy Markdown

Contributor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
@steveisok
steveisok
commented
Apr 20, 2025

Copy link


Copy Markdown

MemberAuthor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
Yeah, it'll be runtimes/<rid>/native, but looking at that again it probably should be tools/<rid>/<library>
@jkotas
jkotas
commented
Apr 20, 2025

Copy link


Copy Markdown

Member
tools directory in nuget packages is meant for executables. cdac is a native library. runtimes looks right to me.
@am11am11
added

area-Infrastructure

and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Apr 21, 2025
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 21, 2025

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/runtime-infrastructure

See info in area-owners.md if you want to be subscribed.
@jkotas
jkotas
commented
Apr 21, 2025•

edited

Loading

Copy link


Copy Markdown

Member
CDac name does not follow the .NET naming conventions:
  • It is not an acronym in common usage. (I do not even know what this acronym stands for.) From .NET Framework design guidelines: "In general, it is important to avoid using acronyms in identifier names unless they are in common usage and are immediately understandable to anyone who might use the framework."
  • If we went with the acronym, it should be capitalized as Cdac. From .NET Framework design guidelines: "Acronyms of three or more characters follow the guidelines of any other word. Only the first letter is capitalized".
I think a longer self-descriptive name would be better.
@max-charlamb
max-charlamb
commented
Apr 21, 2025

Copy link


Copy Markdown

Member
@jkotas, would you be okay with contractdac?
@steveisok
steveisok
commented
Apr 21, 2025

Copy link


Copy Markdown

MemberAuthor
I think a longer self-descriptive name would be better.
@jkotas, would you be okay with contractdac?
I have no strong opinion. I'll be glad to name it whatever we all think is best.

This was referenced Apr 21, 2025

Closed


Open


Open


Closed


Closed


Open


Closed

@am11
am11
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
The convention used by src/native/libs is <managed namespace>.Native, which produce libraries with same pattern, e.g. libSystem.IO.Ports.Native. This could be applied here as well?
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
System.IO.Ports.Native is a helper library for implementation of System.IO.Ports namespace. The cdacreader is not a helper library like that. cdacreader is a managed library built from https://github.com/dotnet/runtime/tree/main/src/native/managed/cdacreader/src/ that we build with PublishAot. The implementation namespace for this library is Microsoft.Diagnostics.DataContractReader.Legacy;, but that is very long name.
We had more offline conversation about the naming. Stay tuned.
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
Add something like this to https://github.com/dotnet/runtime/blob/main/docs/project/glossary.md:
CDAC Codename for (Data Contracts)[../design/datacontracts/datacontracts_design.md] project
jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Target.cs
Comment threadsrc/coreclr/debug/daccess/cdac.cpp
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadsrc/native/managed/cdac/src/mscordaccore_universal.csproj
Comment threadeng/Subsets.props
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Apr 23, 2025

Closed

jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threaddocs/project/glossary.md
Outdated

@jkotas
jkotas
commented
Apr 24, 2025

Copy link


Copy Markdown

Member
Microsoft.DotNet.CDAC.Transport.pkgproj
Nit: The casing that follows .NET naming conventions would be Cdac: #114812 (comment)
jkotas
jkotas
approved these changes
Apr 24, 2025
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM! Thanks
Comment threadDirectory.Build.props

 <PropertyGroup>
 <DotNetHostBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', '$(OutputRID).$(HostConfiguration)', 'corehost'))</DotNetHostBinDir>
 <DotNetCDacBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'mscordaccore_universal', '$(Configuration)', '$(NetCoreAppCurrent)', '$(OutputRID)', 'publish'))</DotNetCDacBinDir>
@jkotasjkotasApr 24, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nit: DotNetCDacBinDir -> DotNetCdacBinDir, _BuildCDacPack -> _BuildCdacPack
@steveisoksteveisokApr 25, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks - I'll do that in a quick follow up.

This was referenced Apr 25, 2025

Open


Closed

@steveisok
steveisok
merged commit 957aaac
into
dotnet:mainApr 25, 2025
@steveisok
steveisok
deleted the

cdac-packaging

branch
April 25, 2025 11:17
steveisok added a commit
to steveisok/runtime
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR
f2665d9
Follow up from dotnet#114812 (comment)
@steveisoksteveisok
mentioned this pull request
Apr 25, 2025
 Merged

steveisok added a commit
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR (#115047)
3eef331
Follow up from #114812 (comment)
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jkotasjkotasjkotas approved these changes

Assignees

@steveisoksteveisok

Labels

area-Infrastructure

Projects

Runtime Infra

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@steveisok@jkotas@mikem8361@max-charlamb@am11

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[cdac] Create transport package and rename the cdac reader - #114812
Merged
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Apr 25, 2025
Merged
[cdac] Create transport package and rename the cdac reader#114812
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Conversation
@steveisok
@steveisoksteveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
This change adds new cdac transport packages that are built for Windows, MacOS, and Linux. Each package contains a host specific native cdac library that is meant to ride along with various diagnostics tools.
The cdacreader was also renamed to cdac as part of this change. I picked what I thought was the best suggestion in #108720 (comment). Feel free to make alternate suggestions.


Contributes to #108720
CopilotAI
review requested due to automatic review settings
April 18, 2025 02:49
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Apr 18, 2025
CopilotAI
reviewed
Apr 18, 2025
View reviewed changes
CopilotAI
left a comment


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.

Pull Request Overview
This PR introduces new cdac transport packages for Windows, MacOS, and Linux and renames the cdacreader to cdac.
  • Updated comments and macro definitions in native library code to reflect the new cdac naming
  • Adjusted pipeline build arguments and subset names across YAML configuration files to align with the renaming
Reviewed Changes
Copilot reviewed 231 out of 237 changed files in this pull request and generated no comments.
Show a summary per file
FileDescription
src/coreclr/debug/daccess/dacimpl.hUpdated comments to refer to cdac instead of cdacreader
src/coreclr/debug/daccess/cdac.cppRenamed macro definition and updated associated comments
eng/pipelines/runtime.ymlRevised build arguments to use tools.cdac and tools.cdactests
eng/pipelines/runtime-diagnostics.ymlModified build arguments to reference the updated cdac nomenclature
eng/pipelines/common/evaluate-default-paths.ymlChanged subset and include paths to reference cdac instead of cdacreader
Files not reviewed (6)
  • Directory.Build.props: Language not supported
  • eng/Subsets.props: Language not supported
  • eng/liveBuilds.targets: Language not supported
  • src/coreclr/debug/daccess/CMakeLists.txt: Language not supported
  • src/installer/pkg/projects/Microsoft.DotNet.CDac.Transport/Microsoft.DotNet.CDac.Transport.pkgproj: Language not supported
  • src/installer/pkg/projects/cdac-packages.proj: Language not supported
@steveisok
steveisok
requested review from
a teamApril 18, 2025 02:49
@jkotas
jkotas
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
@steveisok
steveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

MemberAuthor
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
I considered that initially and figured I would wait to see how others felt before going there. Thanks for the feedback.
@mikem8361
mikem8361
commented
Apr 19, 2025

Copy link


Copy Markdown

Contributor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
@steveisok
steveisok
commented
Apr 20, 2025

Copy link


Copy Markdown

MemberAuthor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
Yeah, it'll be runtimes/<rid>/native, but looking at that again it probably should be tools/<rid>/<library>
@jkotas
jkotas
commented
Apr 20, 2025

Copy link


Copy Markdown

Member
tools directory in nuget packages is meant for executables. cdac is a native library. runtimes looks right to me.
@am11am11
added

area-Infrastructure

and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Apr 21, 2025
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 21, 2025

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/runtime-infrastructure

See info in area-owners.md if you want to be subscribed.
@jkotas
jkotas
commented
Apr 21, 2025•

edited

Loading

Copy link


Copy Markdown

Member
CDac name does not follow the .NET naming conventions:
  • It is not an acronym in common usage. (I do not even know what this acronym stands for.) From .NET Framework design guidelines: "In general, it is important to avoid using acronyms in identifier names unless they are in common usage and are immediately understandable to anyone who might use the framework."
  • If we went with the acronym, it should be capitalized as Cdac. From .NET Framework design guidelines: "Acronyms of three or more characters follow the guidelines of any other word. Only the first letter is capitalized".
I think a longer self-descriptive name would be better.
@max-charlamb
max-charlamb
commented
Apr 21, 2025

Copy link


Copy Markdown

Member
@jkotas, would you be okay with contractdac?
@steveisok
steveisok
commented
Apr 21, 2025

Copy link


Copy Markdown

MemberAuthor
I think a longer self-descriptive name would be better.
@jkotas, would you be okay with contractdac?
I have no strong opinion. I'll be glad to name it whatever we all think is best.

This was referenced Apr 21, 2025

Closed


Open


Open


Closed


Closed


Open


Closed

@am11
am11
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
The convention used by src/native/libs is <managed namespace>.Native, which produce libraries with same pattern, e.g. libSystem.IO.Ports.Native. This could be applied here as well?
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
System.IO.Ports.Native is a helper library for implementation of System.IO.Ports namespace. The cdacreader is not a helper library like that. cdacreader is a managed library built from https://github.com/dotnet/runtime/tree/main/src/native/managed/cdacreader/src/ that we build with PublishAot. The implementation namespace for this library is Microsoft.Diagnostics.DataContractReader.Legacy;, but that is very long name.
We had more offline conversation about the naming. Stay tuned.
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
Add something like this to https://github.com/dotnet/runtime/blob/main/docs/project/glossary.md:
CDAC Codename for (Data Contracts)[../design/datacontracts/datacontracts_design.md] project
jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Target.cs
Comment threadsrc/coreclr/debug/daccess/cdac.cpp
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadsrc/native/managed/cdac/src/mscordaccore_universal.csproj
Comment threadeng/Subsets.props
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Apr 23, 2025

Closed

jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threaddocs/project/glossary.md
Outdated

@jkotas
jkotas
commented
Apr 24, 2025

Copy link


Copy Markdown

Member
Microsoft.DotNet.CDAC.Transport.pkgproj
Nit: The casing that follows .NET naming conventions would be Cdac: #114812 (comment)
jkotas
jkotas
approved these changes
Apr 24, 2025
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM! Thanks
Comment threadDirectory.Build.props

 <PropertyGroup>
 <DotNetHostBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', '$(OutputRID).$(HostConfiguration)', 'corehost'))</DotNetHostBinDir>
 <DotNetCDacBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'mscordaccore_universal', '$(Configuration)', '$(NetCoreAppCurrent)', '$(OutputRID)', 'publish'))</DotNetCDacBinDir>
@jkotasjkotasApr 24, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nit: DotNetCDacBinDir -> DotNetCdacBinDir, _BuildCDacPack -> _BuildCdacPack
@steveisoksteveisokApr 25, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks - I'll do that in a quick follow up.

This was referenced Apr 25, 2025

Open


Closed

@steveisok
steveisok
merged commit 957aaac
into
dotnet:mainApr 25, 2025
@steveisok
steveisok
deleted the

cdac-packaging

branch
April 25, 2025 11:17
steveisok added a commit
to steveisok/runtime
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR
f2665d9
Follow up from dotnet#114812 (comment)
@steveisoksteveisok
mentioned this pull request
Apr 25, 2025
 Merged

steveisok added a commit
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR (#115047)
3eef331
Follow up from #114812 (comment)
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jkotasjkotasjkotas approved these changes

Assignees

@steveisoksteveisok

Labels

area-Infrastructure

Projects

Runtime Infra

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@steveisok@jkotas@mikem8361@max-charlamb@am11

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
[cdac] Create transport package and rename the cdac reader - #114812
Merged
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Apr 25, 2025
Merged
[cdac] Create transport package and rename the cdac reader#114812
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Conversation
@steveisok
@steveisoksteveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
This change adds new cdac transport packages that are built for Windows, MacOS, and Linux. Each package contains a host specific native cdac library that is meant to ride along with various diagnostics tools.
The cdacreader was also renamed to cdac as part of this change. I picked what I thought was the best suggestion in #108720 (comment). Feel free to make alternate suggestions.


Contributes to #108720
CopilotAI
review requested due to automatic review settings
April 18, 2025 02:49
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Apr 18, 2025
CopilotAI
reviewed
Apr 18, 2025
View reviewed changes
CopilotAI
left a comment


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.

Pull Request Overview
This PR introduces new cdac transport packages for Windows, MacOS, and Linux and renames the cdacreader to cdac.
  • Updated comments and macro definitions in native library code to reflect the new cdac naming
  • Adjusted pipeline build arguments and subset names across YAML configuration files to align with the renaming
Reviewed Changes
Copilot reviewed 231 out of 237 changed files in this pull request and generated no comments.
Show a summary per file
FileDescription
src/coreclr/debug/daccess/dacimpl.hUpdated comments to refer to cdac instead of cdacreader
src/coreclr/debug/daccess/cdac.cppRenamed macro definition and updated associated comments
eng/pipelines/runtime.ymlRevised build arguments to use tools.cdac and tools.cdactests
eng/pipelines/runtime-diagnostics.ymlModified build arguments to reference the updated cdac nomenclature
eng/pipelines/common/evaluate-default-paths.ymlChanged subset and include paths to reference cdac instead of cdacreader
Files not reviewed (6)
  • Directory.Build.props: Language not supported
  • eng/Subsets.props: Language not supported
  • eng/liveBuilds.targets: Language not supported
  • src/coreclr/debug/daccess/CMakeLists.txt: Language not supported
  • src/installer/pkg/projects/Microsoft.DotNet.CDac.Transport/Microsoft.DotNet.CDac.Transport.pkgproj: Language not supported
  • src/installer/pkg/projects/cdac-packages.proj: Language not supported
@steveisok
steveisok
requested review from
a teamApril 18, 2025 02:49
@jkotas
jkotas
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
@steveisok
steveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

MemberAuthor
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
I considered that initially and figured I would wait to see how others felt before going there. Thanks for the feedback.
@mikem8361
mikem8361
commented
Apr 19, 2025

Copy link


Copy Markdown

Contributor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
@steveisok
steveisok
commented
Apr 20, 2025

Copy link


Copy Markdown

MemberAuthor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
Yeah, it'll be runtimes/<rid>/native, but looking at that again it probably should be tools/<rid>/<library>
@jkotas
jkotas
commented
Apr 20, 2025

Copy link


Copy Markdown

Member
tools directory in nuget packages is meant for executables. cdac is a native library. runtimes looks right to me.
@am11am11
added

area-Infrastructure

and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Apr 21, 2025
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 21, 2025

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/runtime-infrastructure

See info in area-owners.md if you want to be subscribed.
@jkotas
jkotas
commented
Apr 21, 2025•

edited

Loading

Copy link


Copy Markdown

Member
CDac name does not follow the .NET naming conventions:
  • It is not an acronym in common usage. (I do not even know what this acronym stands for.) From .NET Framework design guidelines: "In general, it is important to avoid using acronyms in identifier names unless they are in common usage and are immediately understandable to anyone who might use the framework."
  • If we went with the acronym, it should be capitalized as Cdac. From .NET Framework design guidelines: "Acronyms of three or more characters follow the guidelines of any other word. Only the first letter is capitalized".
I think a longer self-descriptive name would be better.
@max-charlamb
max-charlamb
commented
Apr 21, 2025

Copy link


Copy Markdown

Member
@jkotas, would you be okay with contractdac?
@steveisok
steveisok
commented
Apr 21, 2025

Copy link


Copy Markdown

MemberAuthor
I think a longer self-descriptive name would be better.
@jkotas, would you be okay with contractdac?
I have no strong opinion. I'll be glad to name it whatever we all think is best.

This was referenced Apr 21, 2025

Closed


Open


Open


Closed


Closed


Open


Closed

@am11
am11
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
The convention used by src/native/libs is <managed namespace>.Native, which produce libraries with same pattern, e.g. libSystem.IO.Ports.Native. This could be applied here as well?
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
System.IO.Ports.Native is a helper library for implementation of System.IO.Ports namespace. The cdacreader is not a helper library like that. cdacreader is a managed library built from https://github.com/dotnet/runtime/tree/main/src/native/managed/cdacreader/src/ that we build with PublishAot. The implementation namespace for this library is Microsoft.Diagnostics.DataContractReader.Legacy;, but that is very long name.
We had more offline conversation about the naming. Stay tuned.
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
Add something like this to https://github.com/dotnet/runtime/blob/main/docs/project/glossary.md:
CDAC Codename for (Data Contracts)[../design/datacontracts/datacontracts_design.md] project
jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Target.cs
Comment threadsrc/coreclr/debug/daccess/cdac.cpp
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadsrc/native/managed/cdac/src/mscordaccore_universal.csproj
Comment threadeng/Subsets.props
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Apr 23, 2025

Closed

jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threaddocs/project/glossary.md
Outdated

@jkotas
jkotas
commented
Apr 24, 2025

Copy link


Copy Markdown

Member
Microsoft.DotNet.CDAC.Transport.pkgproj
Nit: The casing that follows .NET naming conventions would be Cdac: #114812 (comment)
jkotas
jkotas
approved these changes
Apr 24, 2025
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM! Thanks
Comment threadDirectory.Build.props

 <PropertyGroup>
 <DotNetHostBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', '$(OutputRID).$(HostConfiguration)', 'corehost'))</DotNetHostBinDir>
 <DotNetCDacBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'mscordaccore_universal', '$(Configuration)', '$(NetCoreAppCurrent)', '$(OutputRID)', 'publish'))</DotNetCDacBinDir>
@jkotasjkotasApr 24, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nit: DotNetCDacBinDir -> DotNetCdacBinDir, _BuildCDacPack -> _BuildCdacPack
@steveisoksteveisokApr 25, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks - I'll do that in a quick follow up.

This was referenced Apr 25, 2025

Open


Closed

@steveisok
steveisok
merged commit 957aaac
into
dotnet:mainApr 25, 2025
@steveisok
steveisok
deleted the

cdac-packaging

branch
April 25, 2025 11:17
steveisok added a commit
to steveisok/runtime
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR
f2665d9
Follow up from dotnet#114812 (comment)
@steveisoksteveisok
mentioned this pull request
Apr 25, 2025
 Merged

steveisok added a commit
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR (#115047)
3eef331
Follow up from #114812 (comment)
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jkotasjkotasjkotas approved these changes

Assignees

@steveisoksteveisok

Labels

area-Infrastructure

Projects

Runtime Infra

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@steveisok@jkotas@mikem8361@max-charlamb@am11

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[cdac] Create transport package and rename the cdac reader - #114812
Merged
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Apr 25, 2025
Merged
[cdac] Create transport package and rename the cdac reader#114812
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Conversation
@steveisok
@steveisoksteveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
This change adds new cdac transport packages that are built for Windows, MacOS, and Linux. Each package contains a host specific native cdac library that is meant to ride along with various diagnostics tools.
The cdacreader was also renamed to cdac as part of this change. I picked what I thought was the best suggestion in #108720 (comment). Feel free to make alternate suggestions.


Contributes to #108720
CopilotAI
review requested due to automatic review settings
April 18, 2025 02:49
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Apr 18, 2025
CopilotAI
reviewed
Apr 18, 2025
View reviewed changes
CopilotAI
left a comment


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.

Pull Request Overview
This PR introduces new cdac transport packages for Windows, MacOS, and Linux and renames the cdacreader to cdac.
  • Updated comments and macro definitions in native library code to reflect the new cdac naming
  • Adjusted pipeline build arguments and subset names across YAML configuration files to align with the renaming
Reviewed Changes
Copilot reviewed 231 out of 237 changed files in this pull request and generated no comments.
Show a summary per file
FileDescription
src/coreclr/debug/daccess/dacimpl.hUpdated comments to refer to cdac instead of cdacreader
src/coreclr/debug/daccess/cdac.cppRenamed macro definition and updated associated comments
eng/pipelines/runtime.ymlRevised build arguments to use tools.cdac and tools.cdactests
eng/pipelines/runtime-diagnostics.ymlModified build arguments to reference the updated cdac nomenclature
eng/pipelines/common/evaluate-default-paths.ymlChanged subset and include paths to reference cdac instead of cdacreader
Files not reviewed (6)
  • Directory.Build.props: Language not supported
  • eng/Subsets.props: Language not supported
  • eng/liveBuilds.targets: Language not supported
  • src/coreclr/debug/daccess/CMakeLists.txt: Language not supported
  • src/installer/pkg/projects/Microsoft.DotNet.CDac.Transport/Microsoft.DotNet.CDac.Transport.pkgproj: Language not supported
  • src/installer/pkg/projects/cdac-packages.proj: Language not supported
@steveisok
steveisok
requested review from
a teamApril 18, 2025 02:49
@jkotas
jkotas
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
@steveisok
steveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

MemberAuthor
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
I considered that initially and figured I would wait to see how others felt before going there. Thanks for the feedback.
@mikem8361
mikem8361
commented
Apr 19, 2025

Copy link


Copy Markdown

Contributor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
@steveisok
steveisok
commented
Apr 20, 2025

Copy link


Copy Markdown

MemberAuthor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
Yeah, it'll be runtimes/<rid>/native, but looking at that again it probably should be tools/<rid>/<library>
@jkotas
jkotas
commented
Apr 20, 2025

Copy link


Copy Markdown

Member
tools directory in nuget packages is meant for executables. cdac is a native library. runtimes looks right to me.
@am11am11
added

area-Infrastructure

and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Apr 21, 2025
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 21, 2025

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/runtime-infrastructure

See info in area-owners.md if you want to be subscribed.
@jkotas
jkotas
commented
Apr 21, 2025•

edited

Loading

Copy link


Copy Markdown

Member
CDac name does not follow the .NET naming conventions:
  • It is not an acronym in common usage. (I do not even know what this acronym stands for.) From .NET Framework design guidelines: "In general, it is important to avoid using acronyms in identifier names unless they are in common usage and are immediately understandable to anyone who might use the framework."
  • If we went with the acronym, it should be capitalized as Cdac. From .NET Framework design guidelines: "Acronyms of three or more characters follow the guidelines of any other word. Only the first letter is capitalized".
I think a longer self-descriptive name would be better.
@max-charlamb
max-charlamb
commented
Apr 21, 2025

Copy link


Copy Markdown

Member
@jkotas, would you be okay with contractdac?
@steveisok
steveisok
commented
Apr 21, 2025

Copy link


Copy Markdown

MemberAuthor
I think a longer self-descriptive name would be better.
@jkotas, would you be okay with contractdac?
I have no strong opinion. I'll be glad to name it whatever we all think is best.

This was referenced Apr 21, 2025

Closed


Open


Open


Closed


Closed


Open


Closed

@am11
am11
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
The convention used by src/native/libs is <managed namespace>.Native, which produce libraries with same pattern, e.g. libSystem.IO.Ports.Native. This could be applied here as well?
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
System.IO.Ports.Native is a helper library for implementation of System.IO.Ports namespace. The cdacreader is not a helper library like that. cdacreader is a managed library built from https://github.com/dotnet/runtime/tree/main/src/native/managed/cdacreader/src/ that we build with PublishAot. The implementation namespace for this library is Microsoft.Diagnostics.DataContractReader.Legacy;, but that is very long name.
We had more offline conversation about the naming. Stay tuned.
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
Add something like this to https://github.com/dotnet/runtime/blob/main/docs/project/glossary.md:
CDAC Codename for (Data Contracts)[../design/datacontracts/datacontracts_design.md] project
jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Target.cs
Comment threadsrc/coreclr/debug/daccess/cdac.cpp
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadsrc/native/managed/cdac/src/mscordaccore_universal.csproj
Comment threadeng/Subsets.props
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Apr 23, 2025

Closed

jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threaddocs/project/glossary.md
Outdated

@jkotas
jkotas
commented
Apr 24, 2025

Copy link


Copy Markdown

Member
Microsoft.DotNet.CDAC.Transport.pkgproj
Nit: The casing that follows .NET naming conventions would be Cdac: #114812 (comment)
jkotas
jkotas
approved these changes
Apr 24, 2025
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM! Thanks
Comment threadDirectory.Build.props

 <PropertyGroup>
 <DotNetHostBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', '$(OutputRID).$(HostConfiguration)', 'corehost'))</DotNetHostBinDir>
 <DotNetCDacBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'mscordaccore_universal', '$(Configuration)', '$(NetCoreAppCurrent)', '$(OutputRID)', 'publish'))</DotNetCDacBinDir>
@jkotasjkotasApr 24, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nit: DotNetCDacBinDir -> DotNetCdacBinDir, _BuildCDacPack -> _BuildCdacPack
@steveisoksteveisokApr 25, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks - I'll do that in a quick follow up.

This was referenced Apr 25, 2025

Open


Closed

@steveisok
steveisok
merged commit 957aaac
into
dotnet:mainApr 25, 2025
@steveisok
steveisok
deleted the

cdac-packaging

branch
April 25, 2025 11:17
steveisok added a commit
to steveisok/runtime
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR
f2665d9
Follow up from dotnet#114812 (comment)
@steveisoksteveisok
mentioned this pull request
Apr 25, 2025
 Merged

steveisok added a commit
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR (#115047)
3eef331
Follow up from #114812 (comment)
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jkotasjkotasjkotas approved these changes

Assignees

@steveisoksteveisok

Labels

area-Infrastructure

Projects

Runtime Infra

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@steveisok@jkotas@mikem8361@max-charlamb@am11

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[cdac] Create transport package and rename the cdac reader - #114812
Merged
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Apr 25, 2025
Merged
[cdac] Create transport package and rename the cdac reader#114812
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Conversation
@steveisok
@steveisoksteveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
This change adds new cdac transport packages that are built for Windows, MacOS, and Linux. Each package contains a host specific native cdac library that is meant to ride along with various diagnostics tools.
The cdacreader was also renamed to cdac as part of this change. I picked what I thought was the best suggestion in #108720 (comment). Feel free to make alternate suggestions.


Contributes to #108720
CopilotAI
review requested due to automatic review settings
April 18, 2025 02:49
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Apr 18, 2025
CopilotAI
reviewed
Apr 18, 2025
View reviewed changes
CopilotAI
left a comment


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.

Pull Request Overview
This PR introduces new cdac transport packages for Windows, MacOS, and Linux and renames the cdacreader to cdac.
  • Updated comments and macro definitions in native library code to reflect the new cdac naming
  • Adjusted pipeline build arguments and subset names across YAML configuration files to align with the renaming
Reviewed Changes
Copilot reviewed 231 out of 237 changed files in this pull request and generated no comments.
Show a summary per file
FileDescription
src/coreclr/debug/daccess/dacimpl.hUpdated comments to refer to cdac instead of cdacreader
src/coreclr/debug/daccess/cdac.cppRenamed macro definition and updated associated comments
eng/pipelines/runtime.ymlRevised build arguments to use tools.cdac and tools.cdactests
eng/pipelines/runtime-diagnostics.ymlModified build arguments to reference the updated cdac nomenclature
eng/pipelines/common/evaluate-default-paths.ymlChanged subset and include paths to reference cdac instead of cdacreader
Files not reviewed (6)
  • Directory.Build.props: Language not supported
  • eng/Subsets.props: Language not supported
  • eng/liveBuilds.targets: Language not supported
  • src/coreclr/debug/daccess/CMakeLists.txt: Language not supported
  • src/installer/pkg/projects/Microsoft.DotNet.CDac.Transport/Microsoft.DotNet.CDac.Transport.pkgproj: Language not supported
  • src/installer/pkg/projects/cdac-packages.proj: Language not supported
@steveisok
steveisok
requested review from
a teamApril 18, 2025 02:49
@jkotas
jkotas
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
@steveisok
steveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

MemberAuthor
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
I considered that initially and figured I would wait to see how others felt before going there. Thanks for the feedback.
@mikem8361
mikem8361
commented
Apr 19, 2025

Copy link


Copy Markdown

Contributor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
@steveisok
steveisok
commented
Apr 20, 2025

Copy link


Copy Markdown

MemberAuthor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
Yeah, it'll be runtimes/<rid>/native, but looking at that again it probably should be tools/<rid>/<library>
@jkotas
jkotas
commented
Apr 20, 2025

Copy link


Copy Markdown

Member
tools directory in nuget packages is meant for executables. cdac is a native library. runtimes looks right to me.
@am11am11
added

area-Infrastructure

and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Apr 21, 2025
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 21, 2025

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/runtime-infrastructure

See info in area-owners.md if you want to be subscribed.
@jkotas
jkotas
commented
Apr 21, 2025•

edited

Loading

Copy link


Copy Markdown

Member
CDac name does not follow the .NET naming conventions:
  • It is not an acronym in common usage. (I do not even know what this acronym stands for.) From .NET Framework design guidelines: "In general, it is important to avoid using acronyms in identifier names unless they are in common usage and are immediately understandable to anyone who might use the framework."
  • If we went with the acronym, it should be capitalized as Cdac. From .NET Framework design guidelines: "Acronyms of three or more characters follow the guidelines of any other word. Only the first letter is capitalized".
I think a longer self-descriptive name would be better.
@max-charlamb
max-charlamb
commented
Apr 21, 2025

Copy link


Copy Markdown

Member
@jkotas, would you be okay with contractdac?
@steveisok
steveisok
commented
Apr 21, 2025

Copy link


Copy Markdown

MemberAuthor
I think a longer self-descriptive name would be better.
@jkotas, would you be okay with contractdac?
I have no strong opinion. I'll be glad to name it whatever we all think is best.

This was referenced Apr 21, 2025

Closed


Open


Open


Closed


Closed


Open


Closed

@am11
am11
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
The convention used by src/native/libs is <managed namespace>.Native, which produce libraries with same pattern, e.g. libSystem.IO.Ports.Native. This could be applied here as well?
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
System.IO.Ports.Native is a helper library for implementation of System.IO.Ports namespace. The cdacreader is not a helper library like that. cdacreader is a managed library built from https://github.com/dotnet/runtime/tree/main/src/native/managed/cdacreader/src/ that we build with PublishAot. The implementation namespace for this library is Microsoft.Diagnostics.DataContractReader.Legacy;, but that is very long name.
We had more offline conversation about the naming. Stay tuned.
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
Add something like this to https://github.com/dotnet/runtime/blob/main/docs/project/glossary.md:
CDAC Codename for (Data Contracts)[../design/datacontracts/datacontracts_design.md] project
jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Target.cs
Comment threadsrc/coreclr/debug/daccess/cdac.cpp
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadsrc/native/managed/cdac/src/mscordaccore_universal.csproj
Comment threadeng/Subsets.props
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Apr 23, 2025

Closed

jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threaddocs/project/glossary.md
Outdated

@jkotas
jkotas
commented
Apr 24, 2025

Copy link


Copy Markdown

Member
Microsoft.DotNet.CDAC.Transport.pkgproj
Nit: The casing that follows .NET naming conventions would be Cdac: #114812 (comment)
jkotas
jkotas
approved these changes
Apr 24, 2025
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM! Thanks
Comment threadDirectory.Build.props

 <PropertyGroup>
 <DotNetHostBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', '$(OutputRID).$(HostConfiguration)', 'corehost'))</DotNetHostBinDir>
 <DotNetCDacBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'mscordaccore_universal', '$(Configuration)', '$(NetCoreAppCurrent)', '$(OutputRID)', 'publish'))</DotNetCDacBinDir>
@jkotasjkotasApr 24, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nit: DotNetCDacBinDir -> DotNetCdacBinDir, _BuildCDacPack -> _BuildCdacPack
@steveisoksteveisokApr 25, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks - I'll do that in a quick follow up.

This was referenced Apr 25, 2025

Open


Closed

@steveisok
steveisok
merged commit 957aaac
into
dotnet:mainApr 25, 2025
@steveisok
steveisok
deleted the

cdac-packaging

branch
April 25, 2025 11:17
steveisok added a commit
to steveisok/runtime
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR
f2665d9
Follow up from dotnet#114812 (comment)
@steveisoksteveisok
mentioned this pull request
Apr 25, 2025
 Merged

steveisok added a commit
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR (#115047)
3eef331
Follow up from #114812 (comment)
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jkotasjkotasjkotas approved these changes

Assignees

@steveisoksteveisok

Labels

area-Infrastructure

Projects

Runtime Infra

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@steveisok@jkotas@mikem8361@max-charlamb@am11

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
[cdac] Create transport package and rename the cdac reader - #114812
Merged
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Apr 25, 2025
Merged
[cdac] Create transport package and rename the cdac reader#114812
steveisok merged 17 commits into
dotnet:mainfrom 
steveisok:cdac-packaging
Conversation
@steveisok
@steveisoksteveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
This change adds new cdac transport packages that are built for Windows, MacOS, and Linux. Each package contains a host specific native cdac library that is meant to ride along with various diagnostics tools.
The cdacreader was also renamed to cdac as part of this change. I picked what I thought was the best suggestion in #108720 (comment). Feel free to make alternate suggestions.


Contributes to #108720
CopilotAI
review requested due to automatic review settings
April 18, 2025 02:49
@ghostghost
added
the 
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
label
Apr 18, 2025
CopilotAI
reviewed
Apr 18, 2025
View reviewed changes
CopilotAI
left a comment


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.

Pull Request Overview
This PR introduces new cdac transport packages for Windows, MacOS, and Linux and renames the cdacreader to cdac.
  • Updated comments and macro definitions in native library code to reflect the new cdac naming
  • Adjusted pipeline build arguments and subset names across YAML configuration files to align with the renaming
Reviewed Changes
Copilot reviewed 231 out of 237 changed files in this pull request and generated no comments.
Show a summary per file
FileDescription
src/coreclr/debug/daccess/dacimpl.hUpdated comments to refer to cdac instead of cdacreader
src/coreclr/debug/daccess/cdac.cppRenamed macro definition and updated associated comments
eng/pipelines/runtime.ymlRevised build arguments to use tools.cdac and tools.cdactests
eng/pipelines/runtime-diagnostics.ymlModified build arguments to reference the updated cdac nomenclature
eng/pipelines/common/evaluate-default-paths.ymlChanged subset and include paths to reference cdac instead of cdacreader
Files not reviewed (6)
  • Directory.Build.props: Language not supported
  • eng/Subsets.props: Language not supported
  • eng/liveBuilds.targets: Language not supported
  • src/coreclr/debug/daccess/CMakeLists.txt: Language not supported
  • src/installer/pkg/projects/Microsoft.DotNet.CDac.Transport/Microsoft.DotNet.CDac.Transport.pkgproj: Language not supported
  • src/installer/pkg/projects/cdac-packages.proj: Language not supported
@steveisok
steveisok
requested review from
a teamApril 18, 2025 02:49
@jkotas
jkotas
commented
Apr 18, 2025

Copy link


Copy Markdown

Member
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
@steveisok
steveisok
commented
Apr 18, 2025

Copy link


Copy Markdown

MemberAuthor
The cdacreader was also renamed to cdac as part of this change
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
I considered that initially and figured I would wait to see how others felt before going there. Thanks for the feedback.
@mikem8361
mikem8361
commented
Apr 19, 2025

Copy link


Copy Markdown

Contributor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
@steveisok
steveisok
commented
Apr 20, 2025

Copy link


Copy Markdown

MemberAuthor
I can't tell from the changes but does the resulting package contain all the platform/architecture builds of the cda modules under a rid subdirectory? Something like tools/win-x64/cdac.dll, tools/linux-x64/libcdac.so, tools/osx-arm64/libcdac.dylib, etc.
Yeah, it'll be runtimes/<rid>/native, but looking at that again it probably should be tools/<rid>/<library>
@jkotas
jkotas
commented
Apr 20, 2025

Copy link


Copy Markdown

Member
tools directory in nuget packages is meant for executables. cdac is a native library. runtimes looks right to me.
@am11am11
added

area-Infrastructure

and removed

needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
Apr 21, 2025
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 21, 2025

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/runtime-infrastructure

See info in area-owners.md if you want to be subscribed.
@jkotas
jkotas
commented
Apr 21, 2025•

edited

Loading

Copy link


Copy Markdown

Member
CDac name does not follow the .NET naming conventions:
  • It is not an acronym in common usage. (I do not even know what this acronym stands for.) From .NET Framework design guidelines: "In general, it is important to avoid using acronyms in identifier names unless they are in common usage and are immediately understandable to anyone who might use the framework."
  • If we went with the acronym, it should be capitalized as Cdac. From .NET Framework design guidelines: "Acronyms of three or more characters follow the guidelines of any other word. Only the first letter is capitalized".
I think a longer self-descriptive name would be better.
@max-charlamb
max-charlamb
commented
Apr 21, 2025

Copy link


Copy Markdown

Member
@jkotas, would you be okay with contractdac?
@steveisok
steveisok
commented
Apr 21, 2025

Copy link


Copy Markdown

MemberAuthor
I think a longer self-descriptive name would be better.
@jkotas, would you be okay with contractdac?
I have no strong opinion. I'll be glad to name it whatever we all think is best.

This was referenced Apr 21, 2025

Closed


Open


Open


Closed


Closed


Open


Closed

@am11
am11
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
The namespace used by cdac is Microsoft.Diagnostics.DataContractReader. If we are renaming it, it would be nice to make the native binary name to be aligned with the namespace and APIs names to make the connection clear.
The convention used by src/native/libs is <managed namespace>.Native, which produce libraries with same pattern, e.g. libSystem.IO.Ports.Native. This could be applied here as well?
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
System.IO.Ports.Native is a helper library for implementation of System.IO.Ports namespace. The cdacreader is not a helper library like that. cdacreader is a managed library built from https://github.com/dotnet/runtime/tree/main/src/native/managed/cdacreader/src/ that we build with PublishAot. The implementation namespace for this library is Microsoft.Diagnostics.DataContractReader.Legacy;, but that is very long name.
We had more offline conversation about the naming. Stay tuned.
@jkotas
jkotas
commented
Apr 23, 2025

Copy link


Copy Markdown

Member
Add something like this to https://github.com/dotnet/runtime/blob/main/docs/project/glossary.md:
CDAC Codename for (Data Contracts)[../design/datacontracts/datacontracts_design.md] project
jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threadsrc/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Target.cs
Comment threadsrc/coreclr/debug/daccess/cdac.cpp
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadsrc/native/managed/cdac/src/mscordaccore_universal.csproj
Comment threadeng/Subsets.props
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Apr 23, 2025

Closed

jkotas
jkotas
reviewed
Apr 23, 2025
View reviewed changes
Comment threaddocs/project/glossary.md
Outdated

@jkotas
jkotas
commented
Apr 24, 2025

Copy link


Copy Markdown

Member
Microsoft.DotNet.CDAC.Transport.pkgproj
Nit: The casing that follows .NET naming conventions would be Cdac: #114812 (comment)
jkotas
jkotas
approved these changes
Apr 24, 2025
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM! Thanks
Comment threadDirectory.Build.props

 <PropertyGroup>
 <DotNetHostBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', '$(OutputRID).$(HostConfiguration)', 'corehost'))</DotNetHostBinDir>
 <DotNetCDacBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'mscordaccore_universal', '$(Configuration)', '$(NetCoreAppCurrent)', '$(OutputRID)', 'publish'))</DotNetCDacBinDir>
@jkotasjkotasApr 24, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nit: DotNetCDacBinDir -> DotNetCdacBinDir, _BuildCDacPack -> _BuildCdacPack
@steveisoksteveisokApr 25, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks - I'll do that in a quick follow up.

This was referenced Apr 25, 2025

Open


Closed

@steveisok
steveisok
merged commit 957aaac
into
dotnet:mainApr 25, 2025
@steveisok
steveisok
deleted the

cdac-packaging

branch
April 25, 2025 11:17
steveisok added a commit
to steveisok/runtime
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR
f2665d9
Follow up from dotnet#114812 (comment)
@steveisoksteveisok
mentioned this pull request
Apr 25, 2025
 Merged

steveisok added a commit
that referenced
this pull request
Apr 25, 2025
@steveisok
[cdac] Nits follow up from transport package PR (#115047)
3eef331
Follow up from #114812 (comment)
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 27, 2025
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

Copilot code reviewCopilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jkotasjkotasjkotas approved these changes

Assignees

@steveisoksteveisok

Labels

area-Infrastructure

Projects

Runtime Infra

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@steveisok@jkotas@mikem8361@max-charlamb@am11