-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update multi-file 'dotnet run file' documentation #48437
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
base: main
Are you sure you want to change the base?
Conversation
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.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
documentation/general/dotnet-run-file.md:48
- [nitpick] Consider verifying that the command-line flag naming (e.g., '--cs-from-stdin' and '--cs-code') is consistent and clearly communicates their purpose.
We can consider adding an option like `dotnet run --cs-from-stdin` which would read the C# file from the standard input
App/Program1/obj/ | ||
App/Program2/bin/ | ||
App/Program2/obj/ | ||
We could consider using `InternalsVisibleTo` attribute but that might result in slight differences between single- and multi-entry-point programs |
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.
Not sure if we want to use InternalsVisibleTo or source sharing. Went for source sharing in this doc but can change.
We could consider using `InternalsVisibleTo` attribute but that might result in slight differences between single- and multi-entry-point programs | ||
(if not now then perhaps in the future if [some "more internal" accessibility](https://github.com/dotnet/csharplang/issues/6794) is added to C# which doesn't respect `InternalsVisibleTo`) | ||
which would be undesirable when users start with a single entry point and later add another. | ||
Also, `InternalsVisibleTo` needs to be added into a C# file as an attribute, or via a complex-looking `AssemblyAttribute` item group into the `.csproj` like: |
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.
Roslyn itself uses items like the following: https://github.com/dotnet/roslyn/blob/4f8f5e6feb80a9c2d651232735ef69d32710927f/src/Compilers/Core/Portable/Microsoft.CodeAnalysis.csproj#L50
Is that not available to us 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.
No, I think that's defined by arcade in https://github.com/dotnet/arcade/blob/1741844bd26eb13fc4731b1e9aed218686717fbd/src/Microsoft.DotNet.Arcade.Sdk/tools/GenerateInternalsVisibleTo.targets
We could introduce this into SDK and that would simplify this option, but still I think there might be other differences with IVT vs source sharing.
The entry-point projects (`Program1` and `Program2` in our example) | ||
have the shared `.cs` files source-included via `<Content Include="../Shared/**/*.cs" />`. |
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 am feeling a little concerned about this still, because it doesn't match what a user would ordinarily do with a normal solution setup. But, I think it's the simplest approach, and I don't have any better ideas. It might turn out to be fine.
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.
We can also add multiple options to the convert command, e.g., one that generates IVT, or one that generates just project reference (internals won't be visible but maybe user can fix that up manually).
@@ -207,6 +218,9 @@ We do not limit these directives to appear only in entry point files. | |||
Indeed, it might be beneficial to let a non-entry-point file like `Util.cs` be self-contained and have all the `#:package`s it needs specified in it, | |||
which also makes it possible to share it independently or symlink it to multiple script folders. | |||
This is also similar to `global using`s which users usually put into a single file but don't have to. | |||
However, beware that all directives are always parsed by the CLI regardless of whether the file is included in the final compilation | |||
(in other words, any build customizations like `<Compile Exclude="Util.cs" />` are ignored), | |||
because the CLI must parse the directives before invoking MSBuild to avoid multiple build invocations. |
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.
Forgot about multi entry point scenarios. I think we need to exclude directives from other entry point files but include directives from the current entry point and all non-entry-point files. But that sems impossible if we use build to detect which files are entry points.
Perhaps we can limit this to only allow directives in the entry point files for now.
Aside: It would be a good idea to come up with and use a search-engine-friendly term for the I think "ignored directives" should basically be tucked away in the C# spec. That's just how we happen to allow them to pass nicely thru the compiler layer while having their "real" meaning be determined by an external layer. I don't think that's a term we should tell users when discussing this feature area. |
## Build outputs | ||
|
||
Build outputs are placed in a temp directory under a subdirectory whose name is hashed file path of the entry point | ||
and which is restricted to the current user per [runtime guidelines][temp-guidelines]. |
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.
Need to adjust this to allow for multiple users to run the same file.
No description provided.