-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Process C# directives in file-based programs #47702
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
Conversation
| if (virtualProjectFile) | ||
| { | ||
| writer.WriteLine($""" | ||
| <Project> |
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've tried using XmlWriter for this, but couldn't make it write indented XML with blank lines between sections. MSBuild APIs also seem unable to do that. So I ended up building the project file as string.
src/Cli/dotnet/commands/dotnet-run/VirtualProjectBuildingCommand.cs
Outdated
Show resolved
Hide resolved
DamianEdwards
left a comment
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.
This is looking good from my point of view 👍
|
@RikkiGibson @chsienki @MiYanni for reviews, thanks |
|
@RikkiGibson @chsienki @MiYanni for reviews, thanks |
|
Taking a look |
| #:sdk Microsoft.NET.Sdk | ||
| #:sdk Aspire.Hosting.Sdk/9.1.0 | ||
| #:property TargetFramework=net11.0 | ||
| #:package System.CommandLine=2.0.0-beta4.22272.1 |
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.
design nit, not to be addressed in this PR: it feels strange that sdk versions and package versions are specified using a different syntax.
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.
Assuming you mean the separator (/ vs =), actually either is permitted currently for any directive; each has its own rationale as explained in the spec:
Any value can optionally have two parts separated by
=or/(the former is consistent with how properties are usually passed, e.g.,/p:Prop=Value, and the latter is what the<Project Sdk="Name/Version">attribute uses).
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.
Huh, I have never had a need to specify an sdk version in the project node, so I did not know.
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's really only used for NuGet-delivered MSBuild SDKs - common ones are Traversal, NoTargets, and now Cargo, all of which are here.
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 do resonate with the feedback though - if we have a first-class syntax for the concept, we don't need to adhere to what MSBuild requires necessarily - as long as the 'eject' handles the transformation if required.
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 prefer just changing to #:package System.CommandLine/2.0.0-beta4.22272.1 and keeping #:property TargetFramework=net11.0.
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 think I can do space as the separator, that sounds best to me.
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 prefer just changing to
#:package System.CommandLine/2.0.0-beta4.22272.1and keeping#:property TargetFramework=net11.0.
Can you explain why would you choose / as separator for packages and = for properties?
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.
Can you explain why would you choose / as separator for packages and = for properties?
Because that matches the prior-art for each case today. PackageID=PackageVersion is not something seen anywhere else, but PackageID/PackageVersion is, and for properties PropertyName=PropertyValue matches the syntax at the CLI (/p:Name=Value).
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.
Where is PackageId/PackageVersion used today?
|
random test idea. It would be good to demonstrate the behavior for |
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 --from-stdin idea here sounds interesting. We added stdin support in csc/vbc: dotnet/roslyn#41166 but it requires wiring up to csc.dll in shell profile on Unix today (https://stackoverflow.com/a/56133028/863980).
cat Program.cs | dotnet run --from-stdin akin to cat main.c | gcc -xc - would be a delight! :)
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.
Yes, I think that should be easy to implement, I can look into it soon as I also think it would be very nice to have.
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.
Tiny suggestion: it would be cool if --from-stdin gets - alias based on the established / defacto convention:
printf "int main() {}" | clang -xc -
echo "int main() {}" | gcc -xc - -o a.out
cat file.rs | rustc -o hello -
printf "console.log('Hello from Node')" | node -
echo "print('Hello from Python')" | python3 -
cat file.lua | lua5.4 -
printf "puts 'Hello from Ruby'" | ruby -
echo "print 'Hello from Perl\n'" | perl -
wget -qO- http://example.com/script.sh | bash -
curl -sSL http://example.com/archive.tar.gz | tar xzf -Note that hyphen is explicitly supported by those tools, and it works on windows as well with those tools (usually with UTF8 codepage). On Unix, they can also use /dev/stdin instead.
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.
Yep I added stdin support in my global tool shim already at https://github.com/DamianEdwards/csrun
| } | ||
|
|
||
| public static string GetNonVirtualProjectFileText() | ||
| private static void WriteProjectFile(TextWriter writer, ImmutableArray<CSharpDirective> directives, bool isVirtualProject, string? targetFilePath) |
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 know this is being built to get functionality into the CLI quickly, but it feels like this really should be a model that you populate with values and then output as XML, instead of just a bunch of string smashing.
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 feels like this really should be a model that you populate with values and then output as XML, instead of just a bunch of string smashing
Can you elaborate what you mean? There is a model - CSharpDirective - which is turned into XML. As I said in a comment earlier (#47702 (comment)), I've tried using XmlWriter but couldn't achieve the same result (btw, I'd argue that XmlWriter also doesn't really have a "model", it has some state but otherwise just writes the values as strings - which is similar to what I do here - but perhaps you meant something different?).
I know this is being built to get functionality into the CLI quickly
In general, that is not my priority over quality code, so please don't assume that and feel free to give any feedback (I can address it in follow up PRs, since you already signed off, I'm planning to merge this one when CI is unstuck).
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.
Okay. Some quick coding and research (since I haven't done this stuff in a while). Here's a working proof-of-concept that outputs exactly one of your project XML test scenarios from here:
sdk/test/dotnet-project-convert.Tests/DotnetProjectConvertTests.cs
Lines 332 to 355 in bee3c8d
| expectedProject: """ | |
| <Project Sdk="Microsoft.NET.Sdk"> | |
| <Sdk Name="Aspire.Hosting.Sdk" Version="9.1.0" /> | |
| <PropertyGroup> | |
| <OutputType>Exe</OutputType> | |
| <TargetFramework>net10.0</TargetFramework> | |
| <ImplicitUsings>enable</ImplicitUsings> | |
| <Nullable>enable</Nullable> | |
| </PropertyGroup> | |
| <PropertyGroup> | |
| <TargetFramework>net11.0</TargetFramework> | |
| <LangVersion>preview</LangVersion> | |
| </PropertyGroup> | |
| <ItemGroup> | |
| <PackageReference Include="System.CommandLine" Version="2.0.0-beta4.22272.1" /> | |
| </ItemGroup> | |
| </Project> | |
| """, |
The "blank line" and multiple property group concept is a bit hacky, but there might be a way to modify the serialization of the actual elements to include the blank lines. If that can be done, then you can do property groups as an array instead. I didn't want to spend too long looking into it, though.
Here's the code:
using System.Text.RegularExpressions;
using System.Xml;
using System.Xml.Serialization;
// https://stackoverflow.com/a/3732234/294804
// This removes xmlns:xsi and xmlns:xsd from the root node.
XmlSerializerNamespaces emptyXmlNamespace = new([XmlQualifiedName.Empty]);
// https://stackoverflow.com/a/3732234/294804
// Sets indentation and removes the top ?xml element.
XmlWriterSettings xmlSettings = new()
{
Indent = true,
OmitXmlDeclaration = true
};
using StringWriter stringWriter = new();
using XmlWriter xmlWriter = XmlWriter.Create(stringWriter, xmlSettings);
new XmlSerializer(typeof(Project)).Serialize(xmlWriter, new Project(), emptyXmlNamespace);
// Convert BlankLine elements to blank lines.
// Remove numbers from PropertyGroup elements.
var lines = stringWriter.ToString().Split(Environment.NewLine);
for (var i = 0; i < lines.Length; i++)
{
if (lines[i].Contains("BlankLine"))
{
lines[i] = string.Empty;
}
if (lines[i].Contains("PropertyGroup"))
{
lines[i] = Regex.Replace(lines[i], @"(PropertyGroup\d*)", "PropertyGroup");
}
}
Console.WriteLine(string.Join(Environment.NewLine, lines));
[XmlRoot()]
public class Project
{
[XmlAttribute("Sdk")]
public string SdkAttribute { get; set; } = "Microsoft.NET.Sdk";
// Blank line concept based on here: https://stackoverflow.com/a/13948941/294804
public string BlankLine1 { get; set; } = string.Empty;
public Sdk Sdk { get; set; } = new();
public string BlankLine2 { get; set; } = string.Empty;
public PropertyGroup1 PropertyGroup1 { get; set; } = new();
public string BlankLine3 { get; set; } = string.Empty;
public PropertyGroup2 PropertyGroup2 { get; set; } = new();
public string BlankLine4 { get; set; } = string.Empty;
public ItemGroup ItemGroup { get; set; } = new();
public string BlankLine5 { get; set; } = string.Empty;
}
public class Sdk
{
[XmlAttribute()]
public string Name { get; set; } = "Aspire.Hosting.Sdk";
[XmlAttribute()]
public string Version { get; set; } = "9.1.0";
}
public class PropertyGroup1
{
public string OutputType { get; set; } = "Exe";
public string TargetFramework { get; set; } = "net10.0";
public string ImplicitUsings { get; set; } = "enable";
public string Nullable { get; set; } = "enable";
}
public class PropertyGroup2
{
public string TargetFramework { get; set; } = "net11.0";
public string LangVersion { get; set; } = "preview";
}
public class ItemGroup
{
public PackageReference PackageReference { get; set; } = new();
}
public class PackageReference
{
[XmlAttribute()]
public string Include { get; set; } = "System.CommandLine";
[XmlAttribute()]
public string Version { get; set; } = "2.0.0-beta4.22272.1";
}|
Hey, was CSI (c# interactive, csi.exe) and using the same conventions considered in the planning for this? |
Yes, it was considered but CSX is a different C# dialect and doesn't have a simple grow up story to project-based programs. If by conventions you mean |
|
Are there any plans to add |
No plans for that. But feel free to file an issue, it's something that could be added if there is demand. Thanks. |
|
Thanks! #48746 ^^ |

https://github.com/dotnet/sdk/blob/main/documentation/general/dotnet-run-file.md#directives-for-project-metadata