Allow void entry point when calling BuildRunner.Execute. #69
Conversation
src/Faithlife.Build/BuildRunner.cs
Outdated
| /// <returns>The exit code for the build.</returns> | ||
| public static int Execute(string[] args, Action<BuildApp> initialize) => | ||
| ExecuteAsync(args, initialize).GetAwaiter().GetResult(); | ||
| public static int Execute(string[] args, Action<BuildApp> initialize, BuildRunnerSettings? settings = null) => |
There was a problem hiding this comment.
This is technically a binary breaking change. Would we prefer to add method overloads, or perhaps a method with a different name to allow void entry point execution?
There was a problem hiding this comment.
If the only use case here is unit testing, consider an internal overload.
There was a problem hiding this comment.
We should avoid the binary breaking change, yes.
I don't mind BuildRunnerSettings if we want to maintain both behaviors.
src/Faithlife.Build/BuildRunner.cs
Outdated
| ArgumentNullException.ThrowIfNull(args); | ||
| ArgumentNullException.ThrowIfNull(initialize); | ||
|
|
||
| if (Assembly.GetEntryAssembly()?.EntryPoint?.ReturnType == typeof(void)) | ||
| if (!settings.AllowVoidEntrypoint && Assembly.GetEntryAssembly()?.EntryPoint?.ReturnType == typeof(void)) |
There was a problem hiding this comment.
Maybe we should just delete this surprising behavior. I forgot I added it. Long term, an analyzer would be a better solution to the problem of forgetting to return the exit code.
There was a problem hiding this comment.
Deleting the behavior would certainly remove any API changes and allow more flexibility in other repositories. If we're comfortable with that, that would likely be my preferred direction.
There was a problem hiding this comment.
In that case, I'm going to drop the API changes from this proposal.
src/Faithlife.Build/BuildRunner.cs
Outdated
| /// <returns>The exit code for the build.</returns> | ||
| public static int Execute(string[] args, Action<BuildApp> initialize) => | ||
| ExecuteAsync(args, initialize).GetAwaiter().GetResult(); | ||
| public static int Execute(string[] args, Action<BuildApp> initialize, BuildRunnerSettings? settings = null) => |
There was a problem hiding this comment.
We should avoid the binary breaking change, yes.
I don't mind BuildRunnerSettings if we want to maintain both behaviors.
| public void FailsOnMissingTarget() | ||
| { | ||
| using var error = new StringWriter(); | ||
| Console.SetError(error); |
There was a problem hiding this comment.
I don't love the non-parallelizable tests, but I guess we don't have a choice.
I'm surprised they aren't being run in parallel, but maybe that's not the default.
There was a problem hiding this comment.
Another option would be to pass stdout/stderr streams via BuildRunnerSettings and have it prefer those over Console if they're present.
There was a problem hiding this comment.
According to this, there is no parallel execution in NUnit by default: https://docs.nunit.org/articles/nunit/writing-tests/attributes/parallelizable.html
There was a problem hiding this comment.
That could work for direct Faithlife.Build output, but not for the apps that targets run, which could be confusing.
There was a problem hiding this comment.
Seeing as parallel execution is not the default in NUnit, I am going to say the Console redirection is fine as is.
Remove API changes to configure it, since it was surprising behavior.
There are two primary motivations for this change:
BuildRunnerSettings, instead of making it internal only.I'm definitely open to feedback on whether this is the correct approach and API design.