-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fixing intellisense in vscode for src and ref projects #34025
Conversation
src/libraries/intellisense.targets
Outdated
<PropertyGroup> | ||
<RefPath>$([MSBuild]::NormalizeDirectory('$(RefRootPath)', '$(TargetFramework)'))</RefPath> | ||
<BaseIntermediateOutputPath>$([MSBuild]::NormalizeDirectory('$(ArtifactsObjDir)', '$(MSBuildProjectName)'))</BaseIntermediateOutputPath> | ||
<IntermediateOutputPath>$(BaseIntermediateOutputPath)$(TargetFramework)-$(TargetFrameworkSuffix)-$(Configuration)\</IntermediateOutputPath> | ||
<IntermediateOutputPath Condition="'$(TargetFrameworkSuffix)' == ''">$(BaseIntermediateOutputPath)$(TargetFramework)-$(Configuration)\</IntermediateOutputPath> | ||
</PropertyGroup> |
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.
These are also duplicated with Directory.Build.props, why didn't you move to shared props 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.
BaseIntermediateOutputPath doesn't seem to need redefining.
Just a thought here: what if we chose a convention for IntermediateOutputPath such that it evaluated to the same thing regardless of if the TFM stripping happened or not?
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.
Currently tfm in runtime repo refers to just tfm without the rid. we can expose a property which tfm-rid if rid is present and tfm otherwise.
@ericstj @safern @ViktorHofer can you take a look here ? this one is ready for review |
@@ -348,9 +271,6 @@ | |||
<!-- setting the output paths --> | |||
<OutputPath>$(BaseOutputPath)$(TargetFramework)-$(TargetFrameworkSuffix)-$(Configuration)\</OutputPath> | |||
<OutputPath Condition="'$(TargetFrameworkSuffix)' == ''">$(BaseOutputPath)$(TargetFramework)-$(Configuration)\</OutputPath> | |||
<BaseIntermediateOutputPath>$([MSBuild]::NormalizeDirectory('$(ArtifactsObjDir)', '$(MSBuildProjectName)'))</BaseIntermediateOutputPath> |
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 should remain I think since it doesn’t need TargetFramework.
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.
The reason for move is same as #34025 (comment)
It will be nice to keep all IntermediateOutput path properties together.
Related to #33427
The intellisense in vscode was broken in vscode. This change fixes it for src and ref project.
The tests project require some more work to get it fixed.