-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-39598: [C#] Fix verification script #39605
GH-39598: [C#] Fix verification script #39605
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good to me, only a minor comment to see if we could find the error earlier instead of when the RC is generated.
@@ -880,8 +880,8 @@ test_csharp() { | |||
if [ "${SOURCE_KIND}" = "local" ]; then | |||
echo "Skipping sourelink verification on local build" | |||
else | |||
dotnet tool run sourcelink test artifacts/Apache.Arrow/Release/netstandard1.3/Apache.Arrow.pdb | |||
dotnet tool run sourcelink test artifacts/Apache.Arrow/Release/netcoreapp3.1/Apache.Arrow.pdb | |||
dotnet tool run sourcelink test artifacts/Apache.Arrow/Release/netstandard2.0/Apache.Arrow.pdb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no experience with C# and I might be saying something that doesn't make sense but is there a reason why we don't test this when SOURCE_KIND
is local
?
We probably could find the issue on nightlies / maintenance branch if we also tested there instead of only when we have generated a Release candidate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the skip was added by #33799.
@westonpace Do you remember why you disabled the sourcelink
check when SOURCE_KIND
is local
?
@@ -880,8 +880,8 @@ test_csharp() { | |||
if [ "${SOURCE_KIND}" = "local" ]; then | |||
echo "Skipping sourelink verification on local build" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a minor typo:
echo "Skipping sourelink verification on local build" | |
echo "Skipping sourcelink verification on local build" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
### What changes are included in this PR? The verification script is modified to look for the versions of .NET now supported by the package. ### Are these changes tested? Manually tested the verification command. * Closes: #39598 Authored-by: Curt Hagenlocher <[email protected]> Signed-off-by: Curt Hagenlocher <[email protected]>
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit d6a8305. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
### What changes are included in this PR? The verification script is modified to look for the versions of .NET now supported by the package. ### Are these changes tested? Manually tested the verification command. * Closes: apache#39598 Authored-by: Curt Hagenlocher <[email protected]> Signed-off-by: Curt Hagenlocher <[email protected]>
### What changes are included in this PR? The verification script is modified to look for the versions of .NET now supported by the package. ### Are these changes tested? Manually tested the verification command. * Closes: apache#39598 Authored-by: Curt Hagenlocher <[email protected]> Signed-off-by: Curt Hagenlocher <[email protected]>
### What changes are included in this PR? The verification script is modified to look for the versions of .NET now supported by the package. ### Are these changes tested? Manually tested the verification command. * Closes: apache#39598 Authored-by: Curt Hagenlocher <[email protected]> Signed-off-by: Curt Hagenlocher <[email protected]>
### What changes are included in this PR? The verification script is modified to look for the versions of .NET now supported by the package. ### Are these changes tested? Manually tested the verification command. * Closes: apache#39598 Authored-by: Curt Hagenlocher <[email protected]> Signed-off-by: Curt Hagenlocher <[email protected]>
### What changes are included in this PR? The verification script is modified to look for the versions of .NET now supported by the package. ### Are these changes tested? Manually tested the verification command. * Closes: apache#39598 Authored-by: Curt Hagenlocher <[email protected]> Signed-off-by: Curt Hagenlocher <[email protected]>
What changes are included in this PR?
The verification script is modified to look for the versions of .NET now supported by the package.
Are these changes tested?
Manually tested the verification command.
file does not exist: artifacts/Apache.Arrow/Release/netstandard1.3/Apache.Arrow.pdb
#39598