Support scripts without extension #49332
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for executing script files without an extension by detecting a shebang line and adjusting the execution logic accordingly.
- Updated test cases to validate behavior when a file does or does not include a shebang.
- Modified the entry point validation logic in VirtualProjectBuildingCommand to probe for a shebang if the file isn’t a .cs file.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests.cs | Added two new tests to verify script execution with and without a shebang. |
| src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs | Updated the file existence and shebang detection logic for non-.cs files. |
|
@dotnet/run-file for review please |
| // Check if the first two characters are #! | ||
| try | ||
| { | ||
| using var stream = File.OpenRead(entryPointFilePath); |
There was a problem hiding this comment.
In the case there is a BOM this is going to fail right? Think we need to use a reader API that is BOM aware and grab the first two char from it to compare to the prefix.
There was a problem hiding this comment.
So, it's fairly easy to specifically detect a BOM. It does open up a question about encoding in general though, so I went looking a bit.
It turns out that linux doesn't support BOM encoded scripts as having a #!: it must literally be the first two chars in a file, and AFAICT it also has to be ASCII.
This further leads me to the question: should we only be doing this check on non-windows?
There was a problem hiding this comment.
Linux source to back up my claims: https://github.com/torvalds/linux/blob/master/fs/binfmt_script.c#L41
There was a problem hiding this comment.
Interesting. Would not have thought that. Let's match that behavior then: aka keep as is.
|
For some reason I can't review this on my phone but I approve the change |
| /// </summary> | ||
| [Theory] | ||
| [InlineData("Program")] | ||
| [InlineData("Program.csx")] | ||
| [InlineData("Program.vb")] | ||
| public void NonCsFileExtension(string fileName) | ||
| public void NonCsFileExtensionWithShebang(string fileName) |
There was a problem hiding this comment.
Consider capturing the new behavior in the spec
This comment was marked as resolved.
This comment was marked as resolved.
| @@ -847,7 +847,28 @@ public static void RemoveDirectivesFromFile(ImmutableArray<CSharpDirective> dire | |||
|
|
|||
| public static bool IsValidEntryPointPath(string entryPointFilePath) | |||
| { | |||
| return entryPointFilePath.EndsWith(".cs", StringComparison.OrdinalIgnoreCase) && File.Exists(entryPointFilePath); | |||
| if (!File.Exists(entryPointFilePath)) | |||
There was a problem hiding this comment.
I am interested to know what happens when command dotnet build is run, when the working directory contains a file build which has a #!, etc.
I am not super sure I care what happens? It just feels like the behavior should be defined.
There was a problem hiding this comment.
This PR doesn't handle the dotnet <file[.cs]> case, you still have to do dotnet run <file[.cs]> right now, so I guess we'll define that behavior when we do that side of the feature. I believe it's tracked separately under: #49202
There was a problem hiding this comment.
nose goes, whoever merges second between this and #48387 gets to deal with it.
There was a problem hiding this comment.
The built-in build command should take precedence (in fact, everything should take precedence, invoking the file in dotnet file is the last fallback when there is nothing else that matches file), but I can definitely add a test in my PR (or in a follow up) once this one goes in.
That makes me think, perhaps we could disallow dotnet file and require dotnet ./file? Nor sure. Could be done in a follow up as well.
Co-authored-by: Rikki Gibson <[email protected]>
Closes #49219