Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public override int Execute()

// Find directives (this can fail, so do this before creating the target directory).
var sourceFile = VirtualProjectBuildingCommand.LoadSourceFile(file);
var directives = VirtualProjectBuildingCommand.FindDirectivesForConversion(sourceFile, force: _force);
var directives = VirtualProjectBuildingCommand.FindDirectives(sourceFile, reportAllErrors: !_force, errors: null);

Directory.CreateDirectory(targetDirectory);

Expand All @@ -50,7 +50,7 @@ public override int Execute()
string projectFile = Path.Join(targetDirectory, Path.GetFileNameWithoutExtension(file) + ".csproj");
using var stream = File.Open(projectFile, FileMode.Create, FileAccess.Write);
using var writer = new StreamWriter(stream, Encoding.UTF8);
VirtualProjectBuildingCommand.WriteProjectFile(writer, directives);
VirtualProjectBuildingCommand.WriteProjectFile(writer, directives, isVirtualProject: false);

return 0;
}
Expand Down
107 changes: 107 additions & 0 deletions src/Cli/dotnet/Commands/Run/Api/RunApiCommand.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Immutable;
using System.CommandLine;
using System.Text.Json;
using System.Text.Json.Serialization;

namespace Microsoft.DotNet.Cli.Commands.Run.Api;

/// <summary>
/// Takes JSON from stdin lines, produces JSON on stdout lines, doesn't perform any changes.
/// Can be used by IDEs to see the project file behind a file-based program.
/// </summary>
internal sealed class RunApiCommand(ParseResult parseResult) : CommandBase(parseResult)
{
public override int Execute()
{
for (string? line; (line = Console.ReadLine()) != null;)
Copy link
Member

Choose a reason for hiding this comment

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

If you haven't already created one, add a test for non-ASCII stuff being included in the stream. One "fun" discovery we had when making our own JSON protocol similar to this for the MSBuildWorkspace build host stuff was that the console code page might gets screwed up in funny ways and mangle stuff. Since we're (currently) using Newtonsoft.Json our hack was just:

https://github.com/dotnet/roslyn/blob/0d6bd9de68dc7e15165dffc8934fe2f473b2c171/src/Workspaces/MSBuild/BuildHost/Rpc/Contracts/JsonSettings.cs#L25-L27

Not sure if you have a similar option here.

{
if (string.IsNullOrWhiteSpace(line))
{
continue;
}

try
{
RunApiInput input = JsonSerializer.Deserialize(line, RunFileApiJsonSerializerContext.Default.RunApiInput)!;
RunApiOutput output = input.Execute();
Respond(output);
}
catch (Exception ex)
{
Respond(new RunApiOutput.Error { Message = ex.Message, Details = ex.ToString() });
Copy link
Member

Choose a reason for hiding this comment

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

Just establishing the contract -- should we also be listening to stderr as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

should we also be listening to stderr as well?

You mean from the IDE? I think you should, although the code inside this try/catch will report all its errors as stdout json message, other code above this (like command-line parsing) might not.

}
}

return 0;

static void Respond(RunApiOutput message)
{
string json = JsonSerializer.Serialize(message, RunFileApiJsonSerializerContext.Default.RunApiOutput);
Console.WriteLine(json);
}
}
}

[JsonDerivedType(typeof(GetProject), nameof(GetProject))]
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to specify a TypeDescriminator here so it's very clear we're not using the class name (which might get renamed) as the actual API contract?

internal abstract class RunApiInput
{
private RunApiInput() { }

public abstract RunApiOutput Execute();

public sealed class GetProject : RunApiInput
{
public string? ArtifactsPath { get; init; }
Copy link
Member

Choose a reason for hiding this comment

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

What it does mean for us to pass this in? I would have imagined you were returning it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is optional, the SDK normally computes it, but you can overwrite it which is useful for tests. I will add a clarifying comment.

public required string EntryPointFileFullPath { get; init; }
Copy link
Member

Choose a reason for hiding this comment

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

This represents the primary bit of information that IDE must provide, right? This means that IDE must come up with a solution for searching, from a loose file that was opened, to find a "nearest relevant entry point", cracking the files as we go, and pass that in to dotnet cli. I think that's something we can do, just making sure of the expectation.

Copy link
Member Author

Choose a reason for hiding this comment

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

This API is for single-file currently, so it should be simple - just pass the current file as the EntryPointFileFullPath.

For multi-file scenarios, I agree you would need to do the search you describe. The API will then also take the list of files to exclude (i.e., the other entry points). The implementation can automatically discover the other .cs files to include (which it needs to read for directives) based on the entry point or we can pass them to the API if that would be better for perf (since the IDE might have already discovered them via its more efficient file watchers).

Copy link
Member Author

@jjonescz jjonescz Apr 29, 2025

Choose a reason for hiding this comment

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

Btw, it might be useful if the IDE checks SDK's version to determine whether run-api is present and whether it supports single-file or multi-file scenarios (and perhaps more in the future) - so the IDE can give better error messages when the SDK being used is not in sync with the features implemented in the IDE.

We could also introduce some run-api call like "GetCapabilities" that the IDE could use (and for SDKs that don't contain run-api command at all, presumably the IDE could detect that from the error message SDK produces when trying to run dotnet run-api).

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a version stamp in requests/responses would give us enough expressiveness here? That would be a bit like SymbolKey.FormatVersion.

Copy link
Member

Choose a reason for hiding this comment

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

it might be useful if the IDE checks SDK's version to determine whether run-api is present

If we have to do things like that, then I'd expect we have some API call we do for "get entry point discovery algorithm" or something, rather than an explicit check on the SDK version directly, especially if we try something experimentally and then have to roll it back in the SDK for a few weeks or something.

Copy link
Member

Choose a reason for hiding this comment

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

(and consider if/when we ever have a situation where the user's repo might opt them into some other behavior)

Copy link
Member

Choose a reason for hiding this comment

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

Why not just "EntryPoint"?

Copy link
Member Author

@jjonescz jjonescz May 16, 2025

Choose a reason for hiding this comment

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

For clarity. It needs to be a full path. It also needs to be a file, and we could allow directories as well in multi-file future if needed (then we'd presumably rename the property).


public override RunApiOutput Execute()
{
var sourceFile = VirtualProjectBuildingCommand.LoadSourceFile(EntryPointFileFullPath);
var errors = ImmutableArray.CreateBuilder<SimpleDiagnostic>();
var directives = VirtualProjectBuildingCommand.FindDirectives(sourceFile, reportAllErrors: true, errors);
string artifactsPath = ArtifactsPath ?? VirtualProjectBuildingCommand.GetArtifactsPath(EntryPointFileFullPath);

var csprojWriter = new StringWriter();
VirtualProjectBuildingCommand.WriteProjectFile(csprojWriter, directives, isVirtualProject: true, targetFilePath: EntryPointFileFullPath, artifactsPath: artifactsPath);

return new RunApiOutput.Project
{
Content = csprojWriter.ToString(),
Diagnostics = errors.ToImmutableArray(),
};
}
}
}

[JsonDerivedType(typeof(Error), nameof(Error))]
[JsonDerivedType(typeof(Project), nameof(Project))]
Comment on lines +79 to +80
Copy link
Member

Choose a reason for hiding this comment

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

Same potential comment as earlier: let's have explicit type discriminators here rather than implicitly encoding a type name.

internal abstract class RunApiOutput
{
private RunApiOutput() { }

/// <summary>
/// When the API shape or behavior changes, this should be incremented so the callers (IDEs) can act accordingly
/// (e.g., show an error message when an incompatible SDK version is being used).
Comment on lines +86 to +87
Copy link
Member

Choose a reason for hiding this comment

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

I would expect that the version number isn't stated here -- we've already had to assume the shape of the output to even read this! Maybe we need to do a few more things:

  1. The version number could stated outside of the first JSON payload (maybe just on a line of it's own), simply because by the time we're reading this value we've already more or less had to assume the JSON output format to a certain extent. This would be critical if/when we have to break the communication format more generally (i.e. we need to switch to MessagePack because this payload is getting too big to use.)
  2. The invocation of the API command from the IDE includes as an input switch the max supported version. That way if we're in a situation where you need to ship a breaking change in the SDK to fix an issue, you can still potentially return an older version for a downlevel client until clients catch up. I recognize the SDK doesn't want to be supporting downlevel clients forever, but it could still at least be useful where if we have to ship a break in Preview n, you can drop the support in Preivew (n+1), but for that few week window while the preview is being built and folks have a mismatch of client versions, we can still negotiate a version that works.

/// </summary>
[JsonPropertyOrder(-1)]
public int Version { get; } = 1;
Copy link
Member

Choose a reason for hiding this comment

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

It might also make sense for caller to pass in the "version" of the API they are using, implying the set of behaviors they expect, and the run-api command can reject the request in a well-defined way if it's not "compatible" with that version.

Copy link
Member

@RikkiGibson RikkiGibson Apr 30, 2025

Choose a reason for hiding this comment

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

Actually, it looks like this property is on the base type? So the caller can always just read the version that comes out. It would be good to verify that this property is present in an error case. No need for caller to pass in the version.


public sealed class Error : RunApiOutput
{
public required string Message { get; init; }
public required string Details { get; init; }
}

public sealed class Project : RunApiOutput
{
public required string Content { get; init; }
public required ImmutableArray<SimpleDiagnostic> Diagnostics { get; init; }
}
}

[JsonSerializable(typeof(RunApiInput))]
[JsonSerializable(typeof(RunApiOutput))]
internal partial class RunFileApiJsonSerializerContext : JsonSerializerContext;
20 changes: 20 additions & 0 deletions src/Cli/dotnet/Commands/Run/Api/RunApiCommandParser.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.CommandLine;

namespace Microsoft.DotNet.Cli.Commands.Run.Api;

internal sealed class RunApiCommandParser
{
public static Command GetCommand()
{
Command command = new("run-api")
{
Hidden = true,
Copy link
Member

Choose a reason for hiding this comment

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

Is there precedent for other commands like this? Rather than having a new command can we/should we hide this behind just an --api flag on dotnet run?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not an expert on dotnet.exe CLI, but I think hiding this under dotnet run --api means all the other dotnet run flags can still be passed and we would need to manually check and error on them, e.g., dotnet run --no-restore --api and so on.

};

command.SetAction((parseResult) => new RunApiCommand(parseResult).Execute());
return command;
}
}
Loading
Loading