Support variables in #:project directives
#51108
Conversation
jjonescz
force-pushed
the
sprint-project-vars
branch
from
9ad9ae2 to
bc17aea
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds support for MSBuild variable expansion inside #:project directives and updates tests and documentation accordingly.
- Introduces directive evaluation pass (EvaluateDirectives) to expand MSBuild variables and resolve project paths.
- Extends CSharpDirective.Project to preserve original and unresolved names for later path adjustments during project conversion.
- Adds tests covering variable usage and updates documentation about variable handling limitations.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| RunTelemetryTests.cs | Adapts tests to new CSharpDirective.Project constructor. |
| RunFileTests.cs | Adds tests for variable-based project references and malformed variable syntax. |
| DotnetProjectConvertTests.cs | Adds test cases for variable-containing paths; updates expectations for cross-platform separators. |
| VirtualProjectBuildingCommand.cs | Adds directive evaluation, caching of source file, and enhanced project directive handling. |
| ProjectConvertCommand.cs | Integrates directive evaluation and updates path rewrite logic to preserve variable intent. |
| dotnet-run-file.md | Documents variable support and caveats for #:project directives. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
The linked issue has milestone 10.0.2xx, was this PR meant to target release/10.0.2xx branch? |
|
Definitely, I was worried there might be changes missing in the 2xx branch, but looks like it's fairly up to date wrt file-based app PRs, so I can retarget now. |
This comment was marked as outdated.
This comment was marked as outdated.
|
@RikkiGibson @333fred @MiYanni for reviews, thanks |
src/Cli/dotnet/Commands/Project/Convert/ProjectConvertCommand.cs
Outdated
Show resolved
Hide resolved
| ? (project is null ? p : p.WithName(project.ExpandString(p.Name))) | ||
| .ResolveProjectPath(sourceFile, diagnostics) |
There was a problem hiding this comment.
Nit: The original formatting here makes what ResolveProjectPath was being invoked on hard to see. Consider something more like this.
| ? (project is null ? p : p.WithName(project.ExpandString(p.Name))) | |
| .ResolveProjectPath(sourceFile, diagnostics) | |
| ? (project is null | |
| ? p | |
| : p.WithName(project.ExpandString(p.Name))) | |
| .ResolveProjectPath(sourceFile, diagnostics) |
|
|
||
| public Project WithName(string name, bool preserveUnresolvedName = false) | ||
| { | ||
| return name == Name |
There was a problem hiding this comment.
It feels like we would also want to create a new instance if !preserveUnresolvedName && UnresolvedName != name. If we don't expect this to occur, it would probably be good to add an assertion to that effect.
There was a problem hiding this comment.
Good catch, forgot to update this when adding the parameter.
|
|
||
| // If the original path is to a directory, just append the resolved file name | ||
| // but preserve the variables from the original, e.g., `../$(..)/Directory/Project.csproj`. | ||
| if (Directory.Exists(project.UnresolvedName)) |
There was a problem hiding this comment.
If I am understanding right, project.UnresolvedName could be relative, creating a dependency on the process current directory. I think we should avoid that, e.g. by explicitly resolving relative to some known absolute path, like the Path of the containing SourceFile, before doing this check.
There was a problem hiding this comment.
Good catch, thanks, I will add a test for this too.
There was a problem hiding this comment.
LGTM, but, would like to wait to merge until #51373 goes in, if possible, so that I can punt the conflicts over to you :P
I guess there will also be a need to decide whether the analyzer will do the evaluation step. I lean slightly toward yes it should, but, don't feel very strongly at the moment.
| /// the <see cref="UnresolvedName"/> will be expanded, but not resolved | ||
| /// (i.e., can be pointing to project directory or file). | ||
| /// </remarks> | ||
| public string UnresolvedName { get; init; } |
There was a problem hiding this comment.
It feels like it's not straightforward to determine whether certain steps have been performed or not on a given Project instance. I'm concerned that if the Project instance reached a layer of the system which expects expansion and/or resolution to have already been done, when those steps weren't actually performed, it may pass by unnoticed.
Is there anything we can do about that? For example, maybe there should be ExpandedName, ResolvedName etc properties which are just null when that step hasn't been performed on the instance. And the base Name property might just hold "whatever the 'best name' is" for the instance, which appears to be used for deduplication, etc.
Curious if you think this situation is worth doing anything about.
It also looks like properties which refer to the same project, using different original paths, are supposed to be treated as duplicates. I think I filed a bug on that for when path/to/project/ and path/to/project/Project.csproj are both used. But do we also have tests for cases where a variable expansion causes paths $(Prop1)/Project.csproj and $(Prop2)/Project.csproj to be the same, for example?
4 participants
Closes #49286.