-
-
Notifications
You must be signed in to change notification settings - Fork 730
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
WithExpectedExitCode ignored when the tool exit code is zero #4013
Comments
Hmm not sure I'm following if I have this console public static class Program
{
public static async Task Main(string[] args)
{
await Task.CompletedTask;
}
} Executing that will return 0 in PowerShell
And in Cake
when executed
If I change co public static class Program
{
public static async Task Main(string[] args)
{
await Task.CompletedTask;
throw new Exception("WOW");
}
} it won't return zero in PowerShell
And in Cake
when executed
And with Context.Tools.RegisterFile(MakeAbsolute(File("./bin/Debug/net7.0/Test.exe")));
Command(
new [] { "Test.exe", "Test"},
expectedExitCode: -532462766
); it won't fail
same if I remove the throw in console public static class Program
{
public static async Task Main(string[] args)
{
await Task.CompletedTask;
}
} it won't fail, am I right to say that that's what unexpected? And you wanted an error like
? That would mean something like diff --git a/src/Cake.Common/Tools/Command/CommandAliases.cs b/src/Cake.Common/Tools/Command/CommandAliases.cs
index 7f1f3fb0..e32aa486 100644
--- a/src/Cake.Common/Tools/Command/CommandAliases.cs
+++ b/src/Cake.Common/Tools/Command/CommandAliases.cs
@@ -8,6 +8,7 @@ using System.Linq;
using Cake.Core;
using Cake.Core.Annotations;
using Cake.Core.IO;
+using Cake.Core.Tooling;
namespace Cake.Common.Tools.Command
{
@@ -425,8 +426,7 @@ namespace Cake.Common.Tools.Command
{
ToolName = toolExecutableNames.First(),
ToolExecutableNames = toolExecutableNames,
- HandleExitCode = exitCode => exitCode == expectedExitCode
- };
+ }.WithExpectedExitCode(expectedExitCode);
return settingsCustomization?.Invoke(settings) ?? settings;
}
diff --git a/src/Cake.Core/Tooling/ToolSettingsExtensions.cs b/src/Cake.Core/Tooling/ToolSettingsExtensions.cs
index 4011095f..72e1cef4 100644
--- a/src/Cake.Core/Tooling/ToolSettingsExtensions.cs
+++ b/src/Cake.Core/Tooling/ToolSettingsExtensions.cs
@@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.
using System;
+using System.Globalization;
using Cake.Core.IO;
namespace Cake.Core.Tooling
@@ -147,6 +148,8 @@ namespace Cake.Core.Tooling
/// <returns>The tools settings.</returns>
public static T WithExpectedExitCode<T>(this T toolSettings, int expectExitCode)
where T : ToolSettings
- => toolSettings.WithToolSettings(toolSettings => toolSettings.HandleExitCode = exitCode => exitCode == expectExitCode);
+ => toolSettings.WithHandleExitCode(exitCode => exitCode == expectExitCode
+ ? true
+ : throw new CakeException(exitCode, string.Format(CultureInfo.InvariantCulture, "Process returned an unexpected exit code {0}.", exitCode)));
}
} But regardless a process will always have an exit code. |
@devlead to reproduce the problem I outline you must meet two conditions:
In this scenario I expect an exception to be thrown by Cake because the exit code does not match the expected value. However, I observe that my Cake script completes successfully, and no exception is thrown. As I have stated, I believe the root of this issue is that zero is hard coded in |
The issue is that exit code is marked as handled if This is why to change this behavior something like above - => toolSettings.WithToolSettings(toolSettings => toolSettings.HandleExitCode = exitCode => exitCode == expectExitCode);
+ => toolSettings.WithHandleExitCode(exitCode => exitCode == expectExitCode
+ ? true
+ : throw new CakeException(exitCode, string.Format(CultureInfo.InvariantCulture, "Process returned an unexpected exit code {0}.", exitCode)));
} to work like you describe above. Unsure if this is the desired behavior, but above diff would result in that behavior change. |
@devlead Here's unit test to demonstrate the problem
|
This is exactly the problem. In the scenario I'm presenting, the exit code is zero and the expected exit code is non-zero. They are not equal and therefore I expect Cake to throw an exception. |
Yes, but the delegate is used for if exit code is essentially already handled or not, and that's what it does. |
Look at the |
If the executable returns a value like '999' for example and the expected exit code is '1234', Cake will throw an exception because they don't match. Why would Cake not throw an exception when the exit code is zero and the expected exit code is '1234'? Why would Cake behave differently in this scenario? |
Yes but it's only processed if HandleExitCode returns false. An expected exit code is something that was added with Command without changing the internals of the Cake process, so Zero is the only known good it has. To only allow it to return true or failed would mean changing the internals or changing the delegate as in the example above in the extension method. |
Correct. And currently HandleExitCode does indeed return false because zero is not equal to '1234'. But this is completely overridden in the In my opinion, an exception should ALWAYS be thrown when HandleExitCode returns false. Currently, that's not the case. I am highlighting the fact that there is one scenario where HandleExitCode returns false, but Cake does not throw the expected exception. |
My suggestion is to replace this code: if (!settings.HandleExitCode?.Invoke(exitCode) ?? true)
{
ProcessExitCode(process.GetExitCode());
} with something like this: if (!settings.HandleExitCode?.Invoke(exitCode) ?? true)
{
const string message = "{0}: Process returned an error (exit code {1}).";
throw new CakeException(exitCode, string.Format(CultureInfo.InvariantCulture, message, GetToolName(), exitCode));
}
else
{
ProcessExitCode(process.GetExitCode());
} The goal is to always throw an exception when |
Prerequisites
Cake runner
Cake .NET Tool
Cake version
2.3.0
Operating system
Windows
Operating system architecture
64-Bit
CI Server
No response
What are you seeing?
This bug is related to the new feature added in Cake 2.3.0.
Firstly, let's say I have an executable built with C# with the following signature
static async Task Main(string[] args)
(noticeTask
rather thanTask<int>
). In other words, my executable does not return a numerical exit code and therefore the exit code is assumed to be zero. I haven't tested it, but I am assuming you could also reproduce this exact same problem with an executable with the following signature:static async Task<int> Main(string[] args)
coupled with this line in the body of the "Main":return 0;
.What's important in order to reproduce the problem I am describing is that the exit code of the executable must be zero. I fully understand that zero typically signifies that the tool completed successfully but that's not always the case.
Secondly, let's say that my Cake script contains the following:
Notice that I specified "1234" as the expected exit code. In this scenario, my executable returns zero and I have specified that the expected exit code should be 1234. I expect an exception to be thrown to let me know that the exit code does not match the expected value but that's not what I observe. I observe that my Cake script completes successfully, and no exception is thrown.
I investigated this situation and I believe I discovered what's causing this problem in
Cake.Core/Tooling/Tool.cs
. Line 101 invokes the custom exit code handler and correctly identifies that zero is not the expected exit code and line 103 invokesProcessExitCode
. However, the logic in theProcessExitCode
method on line 115 throws an exception IF AND ONLY IF the exit code is different than zero which, in the scenario I presented is not the desired behavior.https://github.com/cake-build/cake/blob/2e23e6f647e00e8ab0e8dc5a9ec2382f99659b45/src/Cake.Core/Tooling/Tool.cs#L101..L120
Again, let me repeat that I completely understand that a "zero" exit code typically signifies success, but I don't think Cake should hard code this logic which has the side effect of overriding the custom exit code handler as I have demonstrated in my scenario.
What is expected?
I expect an exception to be thrown which would let me know that the exit code does not match the expected value.
Steps to Reproduce
As I stated previously, there are two conditions that must be met to reproduce this problem:
Task<int> Main
returns zero orTask Main
does not return a numerical exit code which consequently is assumed to be zero)Command
must define a non-zero expected exit codeWhen these two conditions are met, I expect an exception but instead my Cake script completes successfully.
Output log
No response
The text was updated successfully, but these errors were encountered: