-
Notifications
You must be signed in to change notification settings - Fork 281
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
Change | Simplify Conditional File Inclusion #2425
Change | Simplify Conditional File Inclusion #2425
Conversation
@@ -2,9 +2,11 @@ | |||
// The .NET Foundation licenses this file to you under the MIT license. | |||
// See the LICENSE file in the project root for more information. | |||
|
|||
using Microsoft.Data; | |||
#if !NET8_0_OR_GREATER |
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 this part of the code System.Net.Security
is not being used on Unix when using Net7+. The driver is using the newly added API from #2063. Basically this part of the code could be deleted when support for net6 is over.
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.
Good to know. There is several files that have !NET8_0_OR_GREATER in this PR, so theoretically they can be removed once net6 support is over. As for now, they should stay, right?
You can also specify |
@cheenamalhotra This is true, but I don't believe modifying the project file should be part of the normal development cycle. In my opinion, all the files for a project should always be visible in the IDE. If a condition necessitates excluding or including a file, the file should still be visible in the project, but opening it will clearly show what conditions it is included in. As an example, when I was experimenting with ref projects, running BuildAllConfigurations, I was getting an error about a class not existing. Tracking down the issue was difficult because I didn't know which framework build was failing. I'm sure it's somewhere in the build log, but the log is very dense and difficult to read at a glance, so I resorted to enabling and disabling each framework from the build target until I isolated which one was throwing the error. Then I had to track down what conditions included the offending file using the csproj, and then track down what conditions included the missing class file. With these changes, I can easily open the file that throws the error, flip through the different target frameworks (idk if VS has this feature) and see what lights up. (There is the caveat that this is still an issue for building on different platforms, but I have plans to eventually remove that issue, too. This PR is just one step.) |
I agree with that and we will also be able to enable loading all files by default without the need to manage which files to import on what frameworks. But I wanted to provide an alternative solution with target framework while you're reviewing compilation errors.. Overall this change looks good to me, and should help us keep our project files lightweight. |
f366ec3
to
8bd6438
Compare
The history here is that project file exclusions were the preferred way of doing conditional compilation. I don't know the reasons why it was preferred. I'm guessing the preference may have just been inherited from .NET Core standards (when SDS lived in dotnet runtime). I never liked it for the same reasons @benrr101 mentioned. So I'm in support of this change. |
…nditionals-inclusion
Assuming that this next round of tests pass (except the enclave ones b/c they never pass apparently), this PR should be ready to go. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2425 +/- ##
==========================================
- Coverage 72.73% 72.69% -0.04%
==========================================
Files 313 313
Lines 61739 61737 -2
==========================================
- Hits 44905 44882 -23
- Misses 16834 16855 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Description: This PR is some prerequisite work for removing ref projects. Although it may not be strictly required for removing ref projects, it will increase my confidence when making the changes to remove ref packages. The netcore project has a bunch of conditional inclusion of files that in some cases overlap and in some cases are cancelled out or redundant due to changes to the supported version changes over the years. In addition, the newer version of .net support more precompiler constants for easy inclusion/exclusion of code based on .net version. (eg,
NET8_0_OR_NEWER
,NETCOREAPP
). As such, it is no longer necessary to have conditional inclusion of files based on .net version - this can be pushed to#if
wrappers in the files. This simplifies the different groups for including files to: !AnyOS, TargetsWindows, and TargetsUnix.IsUAPAssembly is currently unaddressed with this PR since I'm not 100% sure what that flag implies. There seems to be some discussion about removing support for UAP anyways (#2323)
Even though there is efforts underway to move to a single project that combines netfx and netcore, this change stands on its own. Using conditional inclusion of files in a project can lead to difficult to diagnose compile-time errors, often times manifesting as "did you forget a reference or using statement?" errors. These are difficult to track down because opening the file in the IDE will usually show no error - the file is right there! why would it not build!? Requiring digging deep to realize the file is part of the project, but not included in a certain build. By unconditionally including the files in the project, and moving the conditional to
#if
, the IDE will show whether or not the code within the#if
is included based on which version of .net is being built.Ref Project Removal Background: Ref assemblies can be generated with the implementation projects if using a certain attribute in the proj, which should remove the need for a separate ref project. The current ref projects are built for AnyOS, and stored in the nuget pkg without specifying an OS. However, the implementation projects generate empty assemblies when built for AnyOS. Thus, when generating a ref assembly, it generates a empty ref assembly. To alleviate that, I'm proposing including the non-OS-specific files in the AnyOS build and using the ProduceOnlyReferenceAssembly attribute to only generate the reference assembly. Although initial tests with simply removing the AnyOS check seemed successful, I'm less confident in them because the different conditions on including files is complex. That's why this PR exists.
Testing: