-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add source package for file-based programs support #51373
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
Add source package for file-based programs support #51373
Conversation
src/Cli/Microsoft.DotNet.FileBasedPrograms/AppDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
src/Cli/Microsoft.DotNet.FileBasedPrograms/AppDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
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'm not sure why copying this file is needed. Does adding <Compile> to the package project not work? I see you are saying "It seems like a .cs file which is "out of tree" doesn't make it into the package." but I've recently created a source package where all the files come from a different directory and it works fine:
Although I wasn't using shproj and projitems, perhaps that's the problem?
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.
In my case I tried adding a Compile item to the .projitems. I'll take a look at the build for the source project you linked and try to work out what the difference is.
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'll take another stab at this soon, my current guess is it is related to a LinkBase attribute on the item, or similar.
| } | ||
|
|
||
| #if FILE_BASED_PROGRAMS_SOURCE_PACKAGE_GRACEFUL_EXCEPTION | ||
| internal class GracefulException : Exception |
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 we remove all the exception throwing? We already report diagnostics, right?
The calling code can convert diagnostics to an exception if needed.
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 it will be best to remove usage of this type from the source package. But, it will involve a decent chunk of refactoring.
| // Also normalize blackslashes to forward slashes to ensure the directive works on all platforms. | ||
| var sourceDirectory = Path.GetDirectoryName(context.SourceFile.Path) ?? "."; | ||
| var resolvedProjectPath = Path.Combine(sourceDirectory, directiveText.Replace('\\', '/')); | ||
| if (Directory.Exists(resolvedProjectPath)) |
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.
Seems like this may depend on the process current directory. Should we abstract that? Depending on process state makes it hard to test and reuse the code in different environments
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.
Are you suggesting adding some way to pass in a base directory for resolution here? Perhaps using the compilation’s SourceReferenceResolver if it exists?
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.
Perhaps pass additional defaultDirectory parameter or something similar.
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.
edit: apologies. Churning on this comment a bit as I think through the scenarios.
I was considering what we should do when context.SourceFile.Path is not absolute.
In the CLI, it looks like a previous layer verifies that the path which is used is absolute. So I think we will not even get to the path that Tomas pointed out when the path is not absolute. So, the non-absolute path scenario probably can't be exercised from the CLI tests.
sdk/src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs
Lines 106 to 112 in 448facf
| public VirtualProjectBuildingCommand( | |
| string entryPointFileFullPath, | |
| MSBuildArgs msbuildArgs) | |
| { | |
| Debug.Assert(Path.IsPathFullyQualified(entryPointFileFullPath)); | |
| EntryPointFileFullPath = entryPointFileFullPath; |
I think the editor scenario where we could get such a path is for an "untitled file". If you just ctrl+N in VS Code, and start writing a file-based app, with directives and all, we have no idea what to resolve those directives relative to. I think in that scenario, we should probably report that we don't know what project the #: refers to, because the current file doesn't have a full path. The message could even hint that the file needs to be saved to disk.
Do also note that the Path which flows in here, would come from the SyntaxTree, and as long as it is saved to disk, we would expect to get a full path for it.
I think we should punt this to a new issue, as we might want additional tests in both the CLI and editor scenarios. I think we definitely don't want a dependency on the process current directory. So the resolution to this issue would involve deleting the ?? ".".
17b3342 to
28adee2
Compare
|
When attempting to use the package in Roslyn, I get the following exception in the analyzer. It looks like the
|
|
I am unsure whether it is better to include the outputs of compiling the I didn't see any examples of a source-only package which includes resource files. Do we have any examples of that in our org? Tagging @tmat @baronfel in case you happen to know (or know where to look.) |
Resources in source packages are PITA. Is it possible to avoid? |
|
The resources are basically the error messages for bad This is the content that would be copied (and possibly translations performed again, unless, perhaps with comments we give the translators a heads up that there is another copy elsewhere to use as a reference.) sdk/src/Cli/Microsoft.DotNet.FileBasedPrograms/FileBasedProgramsResources.resx Lines 120 to 171 in 839754f
|
I would do this and on the consuming side something like this could work? <PackageReference Include="microsoft.dotnet.filebasedprograms" GeneratePathProperty="true" />
<EmbeddedResource Include="$(PkgMicrosoft_DotNet_FileBasedPrograms)contentfiles\cs\netstandard2.0\FileBasedProgramResources.resx" /> |
|
I was able to get things working on the Roslyn side with @jjonescz's suggestion. Hopefully there is a simpler way to define |
|
|
||
| ## Usage in Consuming Projects | ||
|
|
||
| To use this package in your project, add the following to your `.csproj` file: |
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.
Could the package contain a build/something.targets file (like for example our microsoft.net.compilers.toolset package has) that would be automatically included and would contain the EmbeddedResource item so users don't need to specify it themselves?
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 wanted to do this, but, wasn't able to get it working. It was not clear to me what the conventions are with targets files in nuget packages. For example, are all files under build/*.targets imported automatically in the consuming 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.
Found docs for this: https://learn.microsoft.com/en-us/nuget/concepts/msbuild-props-and-targets
Looks like we'd want a buildMultiTargeting/Microsoft.DotNet.FileBasedPrograms.targets
| <!-- Exclude the auto-generated .cs file from the resx from the package --> | ||
| <Target Name="ExcludeGeneratedResxFromPackage" BeforeTargets="GenerateNuspec"> | ||
| <ItemGroup> | ||
| <_PackageFiles Remove="@(_PackageFiles)" Condition="$([System.String]::Copy('%(_PackageFiles.Identity)').EndsWith('FileBasedProgramsResources.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.
Where exactly is this generated file located originally? Somewhere in obj/bin folder?
Anyway, perhaps a simpler way to exclude it would be something like
<Compile Update="/path/to/the/file.cs" Pack="false" />(where the path could be perhaps even something like **/FileBasedProgramsResources.cs)
(still might need to live in a Target though if the original item we are updating was defined in a Target as well)
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 exactly is this generated file located originally? Somewhere in
obj/binfolder?
With net9.0, for example: artifacts\obj\Microsoft.CodeAnalysis.CSharp.Features\Debug\net9.0\Microsoft.CodeAnalysis.CSharp.CSharpAnalyzersResources.cs
Yes, exactly, it needs to be done in a target. I would like to try and simplify this, but, I didn't yet manage to do so successfully.
I believe a target exists related to GenerateNuspec, which looks for which items have Pack="false" and excludes them. So we would try running before that and excluding the file by name or by glob.
I still don't really get how to "debug" msbuild targets. For example, sometimes the target I define appears in grayed-out in the binlog viewer, and I don't know what it means. I imagine the target was skipped, but I don't know how to get info about why it was skipped.
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.
For example, sometimes the target I define appears in grayed-out in the binlog viewer, and I don't know what it means. I imagine the target was skipped, but I don't know how to get info about why it was skipped.
If the target is skipped, there should be an explanation (see screenshot), otherwise I guess the greyed-out target is just empty or something.
ddcbc65 to
0c909f4
Compare
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
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.
Overall LGTM, just small nits and questions before I think this can be merged
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs
Outdated
Show resolved
Hide resolved
| <resheader name="writer"> | ||
| <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value> | ||
| </resheader> | ||
| <data name="CouldNotFindAnyProjectInDirectory" xml:space="preserve"> |
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 we remove these strings from the original resx to avoid duplication?
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, sorry, this should have been included in the original pure move commit.
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.
Some of these are also used in non-file-based programs scenarios, such as CouldNotFindAnyProjectInDirectory, but perhaps changing those sites to use FileBasedProgramsResources isn't really a problem?
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 did 2 things to try and clean up this area:
- Introduced extensions for the CliStrings which were deleted, which redirect to FileBasedProgramsResources, to avoid some churn at use sites.
- Migrated the translated strings from the old CliStrings/CliCommandStrings xlf files, to the new FileBasedProgramsResources xlf files.
|
Opened #51487 to track follow ups to this PR. |
| /// <summary> | ||
| /// Span of the full line including the trailing line break. | ||
| /// </summary> | ||
| public required TextSpan Span { get; init; } |
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 don't understand moving off of Range/Index above, but adding required here. Both need polyfills, why are we saying that we don't expect the consumer to have polyfilled System.Range, but do expect them to have polyfilled the required attributes?
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 was a result of an intermediate state where essentially all this code was pasted directly into the MS.CA.CSharp.Features project and made to compile under netstandard2.0. Then, when that code was brought back into the source package "scaffolding", the contracts package came along with it, adding a bunch of polyfills.
I'm restoring one of the usages of Range as it seems a bit better ergonomically.
Nevermind, the Contracts package provides a declaration of RequiredMemberAttribute, but doesn't declare the string.AsSpan(Range) extensions and similar. I will plan on leaving the current state as-is regarding required and Range/Index unless you have a strong feeling there.
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 have a strong feeling here. The source package effectively requires consumers to have polyfilled stuff; either it should state what it requires being polyfilled, and should be consistent about the effective TFM that should be polyfilled, or it should not depend on any polyfills being present.
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 a similar issue exists with the MS.CA.CSharp dependency. As currently written, if consuming project doesn't actually reference MS.CA.CSharp, then the source just won't compile.
It's not clear to me yet how straightforward this is to solve. The stopgap would be to document "you need to also add PackageReference to X, Y, and Z for things to work", but, it feels like we should be able to simply have references to the things we need and have the usual dependency management system work it out.
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 pushed a change to remove PrivateAssets from the dependencies which are needed for this package to compile at the consumption site, and verified that things work internally in the SDK and when consuming from Roslyn. I believe this addresses the concern re: polyfills.
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 doesn't particularly, as we have still reverted the Range changes, but I'm not going to block the PR on it.
This package will be used to support dotnet/roslyn#80575.
Manual review of the first 2 commits may be helpful.