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

Remove GRPC and move to http using kestrel #1137

Merged
merged 6 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions Directory.Packages.props
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@
<PackageVersion Include="DotNet.Glob" Version="3.1.3" />
<PackageVersion Include="FluentAssertions" Version="6.12.0" />
<PackageVersion Include="GitHubActionsTestLogger" Version="2.3.3" />
<PackageVersion Include="Google.Protobuf" Version="3.24.2" />
<PackageVersion Include="Grpc.Core" Version="2.46.6" />
<PackageVersion Include="Grpc.Tools" Version="2.57.0" />
<PackageVersion Include="Ignore" Version="0.1.50" />
<PackageVersion Include="ini-parser-netstandard" Version="2.5.2" />
<PackageVersion Include="Microsoft.AspNetCore.SpaServices.Extensions" Version="5.0.1" />
Expand Down
56 changes: 0 additions & 56 deletions Src/CSharpier.Cli.Tests/GrpcTests.cs

This file was deleted.

68 changes: 68 additions & 0 deletions Src/CSharpier.Cli.Tests/IpcTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
namespace CSharpier.Cli.Tests;

using System.Diagnostics;
using System.Net.Http.Json;
using FluentAssertions;
using NUnit.Framework;

[TestFixture]
public class IpcTests
{
private static readonly HttpClient httpClient = new HttpClient();

// TODO server add other tests
// starting on port
// ignore file
// option file
[Test]
[Ignore("not working on GH/linux")]
public async Task Stuff()
{
var path = Path.Combine(Directory.GetCurrentDirectory(), "dotnet-csharpier.dll");

var processStartInfo = new ProcessStartInfo("dotnet", $"{path} --ipc")
{
UseShellExecute = false,
ErrorDialog = false,
CreateNoWindow = true,
WindowStyle = ProcessWindowStyle.Hidden,
RedirectStandardOutput = true,
RedirectStandardError = true,
EnvironmentVariables = { ["DOTNET_NOLOGO"] = "1" },
};

var process = new Process { StartInfo = processStartInfo };
try
{
process.Start();

var portString = (await process.StandardOutput.ReadLineAsync() ?? string.Empty).Replace(
"Started on ",
string.Empty
);
var port = int.Parse(portString);
var data = new FormatFileDto
{
FileName = "/Temp/test.cs",
FileContents = "public class TestClass { }"
};

var response = await httpClient.PostAsJsonAsync(
$"http://localhost:{port}/format",
data
);
response.EnsureSuccessStatusCode();
var result = await response.Content.ReadFromJsonAsync<FormatFileResult>();
if (result == null)
{
Assert.Fail("Result is null");
}

result!.FormattedFile!.TrimEnd().Should().Be("public class TestClass { }");
}
finally
{
process.Kill();
}
}
}
7 changes: 1 addition & 6 deletions Src/CSharpier.Cli/CSharpier.Cli.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,6 @@
</PropertyGroup>
<ItemGroup>
<PackageReference Include="DotNet.Glob" />
<PackageReference Include="Google.Protobuf" />
<PackageReference Include="Grpc.Core" />
<PackageReference Include="Grpc.Tools">
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
<PrivateAssets>all</PrivateAssets>
</PackageReference>
<PackageReference Include="Ignore" />
<PackageReference Include="ini-parser-netstandard" />
<PackageReference Include="Microsoft.Extensions.Logging" />
Expand All @@ -27,6 +21,7 @@
<PackageReference Include="System.IO.Hashing" />
<PackageReference Include="System.Text.Encoding.CodePages" />
<PackageReference Include="YamlDotNet" />
<FrameworkReference Include="Microsoft.AspNetCore.App" />
</ItemGroup>
<ItemGroup>
<AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleToAttribute">
Expand Down
12 changes: 6 additions & 6 deletions Src/CSharpier.Cli/CommandLineOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ internal delegate Task<int> Handler(
bool skipWrite,
bool writeStdout,
bool pipeMultipleFiles,
bool grpc,
int? grpcPort,
bool ipc,
int? ipcPort,
bool noCache,
bool noMSBuildCheck,
bool includeGenerated,
Expand Down Expand Up @@ -83,11 +83,11 @@ public static RootCommand Create()
"Keep csharpier running so that multiples files can be piped to it via stdin."
),
new Option(
new[] { "--grpc" },
"Run CSharpier as a service using GRPC so that multiple files may be formatted."
new[] { "--server" },
"Run CSharpier as a server so that multiple files may be formatted."
),
new Option<int?>(
new[] { "--grpc-port" },
new[] { "--server-port" },
"Specify the port that CSharpier should start on. Defaults to a random unused port."
),
new Option<string>(
Expand All @@ -105,7 +105,7 @@ public static RootCommand Create()
if (
!Console.IsInputRedirected
&& !cmd.Children.Contains("directoryOrFile")
&& !cmd.Children.Contains("--grpc")
&& !cmd.Children.Contains("--server")
)
{
return "directoryOrFile is required when not piping stdin to CSharpier";
Expand Down
3 changes: 3 additions & 0 deletions Src/CSharpier.Cli/ConsoleLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ public bool IsEnabled(LogLevel logLevel)
}

public IDisposable BeginScope<TState>(TState state)
#if NET7_0_OR_GREATER
where TState : notnull
#endif
{
throw new NotImplementedException();
}
Expand Down
10 changes: 5 additions & 5 deletions Src/CSharpier.Cli/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ public static async Task<int> Run(
bool skipWrite,
bool writeStdout,
bool pipeMultipleFiles,
bool grpc,
int? grpcPort,
bool ipc,
int? ipcPort,
bool noCache,
bool noMSBuildCheck,
bool includeGenerated,
Expand All @@ -52,10 +52,10 @@ CancellationToken cancellationToken
);
}

if (grpc)
if (ipc)
{
return await GrpcFormatter.StartServer(
grpcPort,
return await ServerFormatter.StartServer(
ipcPort,
logger,
actualConfigPath,
cancellationToken
Expand Down
Original file line number Diff line number Diff line change
@@ -1,39 +1,46 @@
using System.Net.NetworkInformation;
using CSharpier.Proto;
using Grpc.Core;
using Microsoft.Extensions.Logging;

namespace CSharpier.Cli;

using System.IO.Abstractions;
using System.Net;
using System.Net.NetworkInformation;
using CSharpier.Cli.Options;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Logging;

public static class GrpcFormatter
public static class ServerFormatter
{
public static Task<int> StartServer(
public static async Task<int> StartServer(
int? port,
ConsoleLogger logger,
string? actualConfigPath,
CancellationToken cancellationToken
)
{
var thePort = port ?? FindFreePort();
var server = new Server
{
Services =
{
CSharpierService.BindService(
new CSharpierServiceImplementation(actualConfigPath, logger)
)
},
Ports = { new ServerPort("localhost", thePort, ServerCredentials.Insecure) }
};
server.Start();

var builder = WebApplication.CreateBuilder();
builder
.WebHost
.ConfigureKestrel(
(_, serverOptions) =>
{
serverOptions.Listen(IPAddress.Any, thePort);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, you can actually use unix sockets (Linux, possibly Mac) and named pipes (Windows) to avoid having to chose a port.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That will make it much easier if you ever plan on having multiple editors share an instance of csharpier.

Copy link
Owner Author

Choose a reason for hiding this comment

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

The proto models are no longer needed, I probably forgot to delete them.

I took a look at both a bit. One concern was windows/linux/mac + java/js/c# is a lot of combinations of things to figure out.

Also with named pipes/unix sockets, I assume I still need to come up with a unique name for each instance of CSharpier that runs. If you are running rider with 0.27 and vscode with 0.27 right now that is two instances of csharpier. I assume they would be unhappy all trying to share the same socket/pipe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. With UNIX sockets, you'd be using (probably):

"${XDG_RUNTIME_DIR}/csharpier/${version}.sock"

Which (in my machine) expanded to:

/run/user/1000/csharpier/0.27.2.sock

I'm not sure what the conventions are for windows named pipes, but I assume a similar thing is possible.

);
var app = builder.Build();
var service = new CSharpierServiceImplementation(actualConfigPath, logger);
app.MapPost(
"/format",
async (FormatFileDto formatFileDto, CancellationToken cancellationToken) =>
await service.FormatFile(formatFileDto, cancellationToken)
);
logger.LogInformation("Started on " + thePort);

await app.RunAsync();

Console.ReadKey();

return Task.FromResult(0);
return 0;
}

public static int FindFreePort()
Expand Down Expand Up @@ -63,7 +70,18 @@ public static int FindFreePort()
}
}

public class CSharpierServiceImplementation : CSharpierService.CSharpierServiceBase
public class FormatFileDto
{
public required string FileContents { get; set; }
public required string FileName { get; set; }
}

public class FormatFileResult
{
public string? FormattedFile { get; set; }
}

public class CSharpierServiceImplementation
{
private readonly string? configPath;
private readonly IFileSystem fileSystem;
Expand All @@ -76,17 +94,18 @@ public CSharpierServiceImplementation(string? configPath, ConsoleLogger logger)
this.fileSystem = new FileSystem();
}

public override async Task<FormatFileResult> FormatFile(
public async Task<FormatFileResult> FormatFile(
FormatFileDto formatFileDto,
ServerCallContext context
CancellationToken cancellationToken
)
{
try
{
var directoryName = this.fileSystem.Path.GetDirectoryName(formatFileDto.FileName);
DebugLogger.Log(directoryName ?? string.Empty);
if (directoryName == null)
{
// TODO proto we can probably still make this work, and just use default options
// TODO server we can probably still make this work, and just use default options
throw new Exception(
$"There was no directory found for file {formatFileDto.FileName}"
);
Expand All @@ -97,31 +116,31 @@ ServerCallContext context
this.configPath,
this.fileSystem,
this.logger,
context.CancellationToken
cancellationToken
);

if (
GeneratedCodeUtilities.IsGeneratedCodeFile(formatFileDto.FileName)
|| optionsProvider.IsIgnored(formatFileDto.FileName)
)
{
// TODO proto should we send back that this is ignored?
// TODO server should we send back that this is ignored?
return new FormatFileResult();
}

var result = await CSharpFormatter.FormatAsync(
formatFileDto.FileContents,
optionsProvider.GetPrinterOptionsFor(formatFileDto.FileName),
context.CancellationToken
cancellationToken
);

// TODO proto what about checking if this actually formatted?
// TODO server what about checking if this actually formatted?
// could send back any error messages now
return new FormatFileResult { FormattedFile = result.Code };
}
catch (Exception ex)
{
// TODO proto should this return this as an error?
// TODO server should this return this as an error?
DebugLogger.Log(ex.ToString());
return new FormatFileResult();
}
Expand Down
Loading