Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add posix dmp copy command #45704

Open
wants to merge 2 commits into
base: release/9.0.2xx
Choose a base branch
from

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Jan 3, 2025

Evidence that this seems to work:

image

This adds a line to a script to move dmps even on posix systems.

@v-wuzhai
Copy link
Member

v-wuzhai commented Jan 6, 2025

This change is made to ensure compatibility with different operating systems, making sure that the hangdump.dmp file can be found and copied to the specified upload directory in various environments. Additionally, I noticed that there doesn't seem to be a command for clearing the NuGet cache in the POSIX Shell environment. Should we consider making some changes here?

@@ -121,7 +121,8 @@
<HelixPreCommands Condition="!$(IsPosixShell)">call %HELIX_CORRELATION_PAYLOAD%\t\RunTestsOnHelix.cmd $(TestFullMSBuild);$(HelixPreCommands)</HelixPreCommands>
<HelixPreCommands Condition="$(IsPosixShell)">. $HELIX_CORRELATION_PAYLOAD/t/RunTestsOnHelix.sh;$(HelixPreCommands)</HelixPreCommands>
<HelixPostCommands Condition="!$(IsPosixShell)">PowerShell -ExecutionPolicy ByPass "dotnet nuget locals all -l | ForEach-Object { $_.Split(' ')[1]} | Where-Object{$_ -like '*cache'} | Get-ChildItem -Recurse -File -Filter '*.dat' | Measure";$(HelixPostCommands)</HelixPostCommands>
<HelixPostCommands>PowerShell -ExecutionPolicy ByPass "Get-ChildItem -Recurse -File -Filter '*hangdump.dmp' | Copy-Item -Destination $env:HELIX_WORKITEM_UPLOAD_ROOT";$(HelixPostCommands)</HelixPostCommands>
<HelixPostCommands Condition="!$(IsPosixShell)">PowerShell -ExecutionPolicy ByPass "Get-ChildItem -Recurse -File -Filter '*hangdump.dmp' | Copy-Item -Destination $env:HELIX_WORKITEM_UPLOAD_ROOT";$(HelixPostCommands)</HelixPostCommands>
<HelixPostCommands Condition="$(IsPosixShell)">find . -name '*hangdump.dmp' -exec cp {} $HELIX_WORKITEM_UPLOAD_ROOT \%3B;$(HelixPostCommands)</HelixPostCommands>
Copy link
Contributor

Choose a reason for hiding this comment

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

Quote "$HELIX_WORKITEM_UPLOAD_ROOT" to prevent word splitting in the POSIX shell in case the value contains white space.

(Windows PowerShell doesn't word-split the unquoted $env:HELIX_WORKITEM_UPLOAD_ROOT, so this will be consistent with that.)

@Forgind
Copy link
Member Author

Forgind commented Jan 6, 2025

This change is made to ensure compatibility with different operating systems, making sure that the hangdump.dmp file can be found and copied to the specified upload directory in various environments. Additionally, I noticed that there doesn't seem to be a command for clearing the NuGet cache in the POSIX Shell environment. Should we consider making some changes here?

Are you referring to the line just above this with dotnet nuget locals all -l? If so, I think that's a performance measure, not clearing the cache. For our tests, I think we actually don't want to clear the cache because then we'd have to re-download a lot of nuget packages, which would be expensive and may lead to rate-limiting from nuget. I think the actual nuget package downloading happens regardless of os. That said, perhaps it doesn't work properly on macos, and that's why the arm64 leg keeps failing? I don't have high hope for that hypothesis, but I'll mention it in the teams thread anyway.

Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

I don't have context as to why we need a different dmp copy command when we didnt before (assume that it was just never utilized on posix machines) or why we want these dmp files. But the overall components of the change look good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants