Remove feature flags for #:include and #:exclude#53775
Remove feature flags for #:include and #:exclude#53775jjonescz wants to merge 10 commits intodotnet:release/10.0.3xxfrom
#:include and #:exclude#53775Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the feature-flag gating for the #:include / #:exclude directives in file-based apps, following up on #52347 and making the directives available without additional MSBuild properties.
Changes:
- Removes the feature-flag enforcement logic for
#:include/#:exclude(and transitive directive processing) fromVirtualProjectBuilder. - Updates CLI tests to no longer set the removed feature flags and removes the now-obsolete feature-flag test.
- Removes the
ExperimentalFeatureDisabledlocalized resource string and updates thedotnet run filedocumentation accordingly.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests.cs | Removes flag setup and the flag-gating test; adds a new test around the remaining item-mapping feature flag. |
| test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs | Removes now-unnecessary feature-flag properties from conversion tests that use include/exclude directives. |
| src/Microsoft.DotNet.ProjectTools/VirtualProjectBuilder.cs | Removes directive feature-flag checks so include/exclude/transitive processing is no longer gated. |
| src/Microsoft.DotNet.ProjectTools/Resources.resx | Removes the ExperimentalFeatureDisabled resource string. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.zh-Hant.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.zh-Hans.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.tr.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.ru.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.pt-BR.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.pl.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.ko.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.ja.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.it.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.fr.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.es.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.de.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Microsoft.DotNet.ProjectTools/xlf/Resources.cs.xlf | Removes the ExperimentalFeatureDisabled localized entry. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/InternalAPI.Unshipped.txt | Drops internal API entries for the removed experimental-flag constants. |
| src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs | Removes the now-unused experimental-flag constant definitions for include/exclude/transitive. |
| documentation/general/dotnet-run-file.md | Removes documentation that stated #:include/#:exclude/transitive were gated by experimental feature flags. |
|
@333fred @RikkiGibson @MiYanni for reviews, thanks |
| For a given `dotnet run file.cs`, we include directives from the current entry point file (`file.cs`) and all other non-entry-point C# files, | ||
| specifically from all `Compile` items included in the project, no matter whether the `Compile` items are specified in some MSBuild code or inferred from `#:include`. | ||
| (Processing directives from other files is currently gated under a feature flag that can be enabled by setting the MSBuild property `ExperimentalFileBasedProgramEnableTransitiveDirectives=true`.) | ||
|
|
There was a problem hiding this comment.
Was turning this single paragraph into two paragraphs intended?
There was a problem hiding this comment.
No, will revert, thanks.
| { | ||
| public const string ExperimentalFileBasedProgramEnableIncludeDirective = nameof(ExperimentalFileBasedProgramEnableIncludeDirective); | ||
| public const string ExperimentalFileBasedProgramEnableExcludeDirective = nameof(ExperimentalFileBasedProgramEnableExcludeDirective); | ||
| public const string ExperimentalFileBasedProgramEnableTransitiveDirectives = nameof(ExperimentalFileBasedProgramEnableTransitiveDirectives); |
There was a problem hiding this comment.
do want to call out that I think the editor still needs changes to handle transitive directives properly. So today it will see #: and assume the file is an entry point (depending on the order things load in).
There was a problem hiding this comment.
I am comfortable with moving forward on that change and just making sure we expedite it properly. But may want to extract this bit to separate PR, if our goal was to only remove experimental flags for these, for scenarios where we expect editor tooling to work.
There was a problem hiding this comment.
@DamianEdwards what would you like to do about the transitive feature flag given IDE doesn't implement support for that yet?
There was a problem hiding this comment.
@RikkiGibson what's the plan for integrating your entry-point detection changes? Is that the change you're referring to when you said:
I am comfortable with moving forward on that change and just making sure we expedite it properly.
I am concerned about missing the boat here a bit as the next train is 10.0.400 (or 11 previews) and that's not until August+. It would be super unfortunate if we're able to get the editor experience changes done well before then but we can't enable it by default in the SDK.
That said, pulling the removal of flag on the transitive directive feature in the SDK into a separate PR seems like the prudent thing to do given the current state.
| } | ||
| } | ||
|
|
||
| private void CheckDirectives( |
There was a problem hiding this comment.
So some version of this check, is going to return, when #:ref support merges behind experimental flag? The churn doesn't specifically worry me, just wanted to understand what to expect.
There was a problem hiding this comment.
yes, and since I just merged the #:ref change, I'm going to merge base branch back here and this diff should become smaller
RikkiGibson
left a comment
There was a problem hiding this comment.
Done review pass. Let's just work out how to deal with transitive directives and then should be able to get this merge ready.
e37e4bf to
d1a16bc
Compare
|
@RikkiGibson for another look, thanks |
Follow up on #52347.