-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Include some default items in file-based apps #49584
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
Changes from 8 commits
e1cfa93
de3c9a5
7799856
2c3f5c1
020269d
3fe613a
528a61e
1ef0473
3e79deb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -140,7 +140,7 @@ public int Execute() | |
| if (EntryPointFileFullPath is not null) | ||
| { | ||
| Debug.Assert(!ReadCodeFromStdin); | ||
| projectFactory = CreateVirtualCommand().PrepareProjectInstance().CreateProjectInstance; | ||
| projectFactory = CreateVirtualCommand().CreateProjectInstance; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -587,7 +587,10 @@ public static RunCommand FromParseResult(ParseResult parseResult) | |
| } | ||
|
|
||
| // If '-' is specified as the input file, read all text from stdin into a temporary file and use that as the entry point. | ||
| entryPointFilePath = Path.GetTempFileName(); | ||
| // We create a new directory for each file so other files are not included in the compilation. | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The CI failed, revealing a bug here, so I'm fixing it in the last commit. Previously, the implementation for running code from stdin put the code into a temp file but with this PR other files in the temp directory could be potentially included in the compilation. I've fixed that by putting the stdin code files into isolated temp directories instead. I've reused the same machinery we have for creating the artifacts folders. I think both these kinds of temp dirs (for artifacts and for stdin code) can reuse other features when implemented (periodical cleanup and customizing the temp path).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a risk that I am curious if we could just think of each user having their own "stdin app" and reusing the artifacts path for it across runs. However, I don't think it's urgent/necessary (or even necessarily correct?) to do so.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We should probably create the file only if it doesn't exist and fail if it does. Thanks. If that happens often, we can improve on the random file name generation.
Hm, interesting, but that would disallow users running multiple stdin apps in parallel. |
||
| string directory = VirtualProjectBuildingCommand.GetTempSubdirectory(Path.GetRandomFileName()); | ||
| VirtualProjectBuildingCommand.CreateTempSubdirectory(directory); | ||
| entryPointFilePath = Path.Join(directory, "app.cs"); | ||
| using (var stdinStream = Console.OpenStandardInput()) | ||
| using (var fileStream = File.OpenWrite(entryPointFilePath)) | ||
| { | ||
|
|
||
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.
How did we arrive at disabling this particular set of items? Do we have a reason for believing that if the directory contains some embedded resource, that we don't want to include it in the file-based app?
Uh oh!
There was an error while loading. Please reload this page.
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.
Compile items are disabled since we are doing single file apps.
Embedded resources were discussed in the internal chat and I think the decision was to also exclude them, right, @baronfel @DamianEdwards? I'm not sure there is a strong reason for that. I think I would also prefer to include everything unless explicitly disabled (would make it easier to reason about this).
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.
Keeping embedded resources seems fine to me. @baronfel what was your reasoning for excluding them from single-file apps?