dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.7k stars 1.06k forks source link

Regression: files missing from solution explorer with multi-targeting #2337

Closed davkean closed 6 years ago

davkean commented 6 years ago

From @onovotny on June 15, 2018 0:22

This is broken with 15.7.3 and 15.8.2 p2 with .NET Core 2.1 installed. It used to work. I think that setting

<EnableDefaultCompileItems>false</EnableDefaultCompileItems> changed the behavior where files used to be visible.

The solution explorer is missing files that are specified by the include globs.

image

ProjectSystemRegression.zip

Note that the project builds correctly, but files aren't visible in VS.

A workaround requires adding <None Include="**\*.cs" Exclude="$(DefaultItemExcludes);$(DefaultExcludesInProjectFolder)" /> to the project file, but I don't believe that used to be necessary, thus the regression.

Here's the csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFrameworks>netstandard2.0;Xamarin.iOS10;MonoAndroid81;uap10.0.16299</TargetFrameworks>

    <EnableDefaultCompileItems>false</EnableDefaultCompileItems>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="MSBuild.Sdk.Extras" Version="1.5.4" PrivateAssets="All" />

    <Compile Include="**\*.shared.cs" />
    <Compile Include="**\*.shared.*.cs" />
  </ItemGroup>
  <ItemGroup Condition=" $(TargetFramework.StartsWith('netstandard')) ">
    <Compile Include="**\*.netstandard.cs" />
    <Compile Include="**\*.netstandard.*.cs" />
  </ItemGroup>
  <ItemGroup Condition=" $(TargetFramework.StartsWith('uap10.0')) ">
    <PackageReference Include="Microsoft.NETCore.UniversalWindowsPlatform" Version="6.0.8" />
    <Compile Include="**\*.uwp.cs" />
    <Compile Include="**\*.uwp.*.cs" />
  </ItemGroup>
  <ItemGroup Condition=" $(TargetFramework.StartsWith('MonoAndroid')) ">
    <PackageReference Include="Xamarin.Android.Support.CustomTabs" Version="27.0.2" />
    <PackageReference Include="Xamarin.Android.Support.Core.Utils" Version="27.0.2" />
    <Compile Include="**\*.android.cs" />
    <Compile Include="**\*.android.*.cs" />
  </ItemGroup>
  <ItemGroup Condition=" $(TargetFramework.StartsWith('Xamarin.iOS')) ">
    <Compile Include="**\*.ios.cs" />
    <Compile Include="**\*.ios.*.cs" />
  </ItemGroup>
  <Import Project="$(MSBuildSDKExtrasTargets)" Condition="Exists('$(MSBuildSDKExtrasTargets)')" />

</Project>

What's interesting here is that there's some dynamic evaluation happening. It's not just *.shared.cs that's visible, but it matches on the first conditional item group as well and shows *.netstandard.cs. It does not show any of the subsequent ones though.

@jamesmontemagno

Copied from original issue: dotnet/project-system#3652

davkean commented 6 years ago

Spoke to @jamesmontemagno yesterday about this, but he mentioned the issue he was running into was https://developercommunity.visualstudio.com/content/problem/268967/multi-targeting-project-with-conditionally-include.html, so clearly lead him down the wrong path.

This is https://github.com/dotnet/sdk/issues/1157 & https://github.com/dotnet/project-system/issues/3181 and was a deliberate change in the SDK.

clairernovotny commented 6 years ago

More weirdness -- it shows and picks up conditional that's the first tfm on the list. If I put the UWP one first, the close/reopen the solution, I see the following.

Also, if you copy/paste the file in the solution explorer, then rename it to match another item,

image

It wrongly adds a Compile item in the csproj: image

Where that should already be covered. VS should not be touching the csproj file here.

davkean commented 6 years ago

The tree is generated based on the configuration and has always been the case in the new project system. You can try this by placing a condition to only include a file in DEBUG and then switch to RELEASE, and we'll update the tree to remove the file. TFMs are considered a dimension in a configuration, you just can't change the "active" one.

I'm not sure I understand the erroneous entries what is the exact steps?

clairernovotny commented 6 years ago

If I add the <None Include="**\*.cs" Exclude="$(DefaultItemExcludes);$(DefaultExcludesInProjectFolder)" /> in, and I repeat, I get other weird behavior:

I copy Class1.uwp.cs and rename it to .android.cs image

clairernovotny commented 6 years ago

Something bad changed in the behavior though, perhaps due to the #1157, but this makes working with other tfm's other than the first, active one, impossible now.

davkean commented 6 years ago

There has been zero changes in the way we handle this in VS, this is an SDK-only change. VS will go out of its way to avoid double-including an item, which has been the behavior since VS 2017 RTM.

File separate bugs for the VS behavior, with a clear "actual" and "expected" so I can understand your expectation.

clairernovotny commented 6 years ago

Exact repro steps from the repro zip I uploaded initially:

  1. Add <None Include="**\*.cs" Exclude="$(DefaultItemExcludes);$(DefaultExcludesInProjectFolder)" /> to the first ItemGroup and hit save.
  2. Select Class1.netstandard.cs and Hit Ctrl+C then Ctrl+V. This adds the following to the csproj, incorrectly image
  3. Rename the pasted file from Class1 - Copy.netstandard.cs to Class1 - Copy.ios.cs. The csproj now shows this: image

Neither the Compile Include nor None Remove should be present after doing this.

davkean commented 6 years ago

This behavior is exactly how it was designed and has been the same for ~2 years, under the following constraints:

The behavior above can be described by the last two constraints. This behavior however is separate to the SDK change and hence, if you'd like to see a different behavior please file a new bug over http://github.com/dotnet/project-system.

clairernovotny commented 6 years ago

@davkean Happy to file a new issue or add to an existing one. I assume that the single TFM issue is the main issue; is there an open issue for that?

davkean commented 6 years ago

The tree is completely based around a "configuration", we represent TFMs a dimension of a configuration; debug|anycpu|net45 and debug|anycpu|netstandard20, etc.

I think this bug you opened two years ago; https://github.com/dotnet/project-system/issues/935 is the bug that best represents this the tree manipulation. The (avoid) double-including of a file behavior is slightly different issue and we should treat separately. I'm completely open to better ideas how to handle this but it is a bloody hard problem to solve and gets very complex very quickly.

clairernovotny commented 6 years ago

The double-including behavior doesn't bother me as much for now (unless it causes other issues).

Is solving dotnet/project-system#935 still in scope for 16.0? Like, can we think about how to solve it?

jamesmontemagno commented 6 years ago

The original issue outlined is a change in functionality from 15.7.1 to 15.7.3. essentially the valuations for the tree stop. In 15.7.1 all files showed correct and 15.7.3 they stopped.

They other issue of it addijg Includes I think has always been there, we manually delete them, it is more annoying then anything, but different then the original issue, so we should focus on that if possible.

davkean commented 6 years ago

Just to clear things up a little;

While it "appears" that the files shown are correct, they are shown with the wrong item type. If you were to manipulate the item's properties, it would not behave as you would expect. It looks like we/you got lucky that we correctly recognize these files as "source files" and they displayed correctly in the editor. This most definitely was not a "designed thing" and kinda just fell out of our implementation. The entire "how do we display multiple TFMs source files" was being tracked by https://github.com/dotnet/project-system/issues/935.

I'm not sure what to do this with this at this point, and will need to sync up with the SDK folks to discuss. @dsplaisted Can we discuss in our Monday sync-up?

dsplaisted commented 6 years ago

@davkean What do you mean by the patch version of VS? If a version like 15.7.3 is major.minor.patch, then we wouldn't put a change like this in a patch update to VS. We would (and did) put an update like this in a minor update. For the SDK we don't really have a difference between major and minor updates, since we release both with Visual Studio and with .NET Core, which can release major versions on different schedules.

davkean commented 6 years ago

@dsplaisted @jamesmontemagno and @onovotny mentioned that this was regressed between 15.7.1 to 15.7.3.

dsplaisted commented 6 years ago

@jamesmontemagno Is it possible that when testing on 15.7.3, that the .NET Core SDK 2.1.300 was installed, while only 2.1.200 was installed together with 15.7.1?

clairernovotny commented 6 years ago

That is possible.

jamesmontemagno commented 6 years ago

I am on 15.7.3 + 2.1.300. I can try to go back down to 2.1.200

dsplaisted commented 6 years ago

@jamesmontemagno Yes, it would be helpful if you could try it on 2.1.200 just so we're sure about what causes this.

jamesmontemagno commented 6 years ago

So some results here on my work machine:

jamesmontemagno commented 6 years ago

So, now with 15.7.4 still same issue with 2.1.300 sdk, however once I delete 2.1.300 from the dotnet/sdk folder it continues to work again, so it looks like there was a change here that is making it happen.

jamesmontemagno commented 6 years ago

Also for reference the reason i had 2.1.300 was for blazor demos

dsplaisted commented 6 years ago

@jamesmontemagno Thanks, this confirms that the behavior you're seeing is the result of what we thought it was: namely, the fix to #1157 and dotnet/project-system#3181.

We will discuss what we can do here to make it easier to browse conditionally included source files in a multi-targeted project. As a workaround, you can try adding something like this to your project file (after any Compile items):

<None Include="**\*.cs" Exclude="$(DefaultItemExcludes);$(DefaultExcludesInProjectFolder);@(Compile)" />
dsplaisted commented 6 years ago

@onovotny If you add @(Compile) to the Exclude as in my previous comment, it may fix the weird behavior you saw with what was added to the project file.

dsplaisted commented 6 years ago

@onovotny @jamesmontemagno Can you try the following "workaround"?

<ItemGroup>
    <None Include="**/*.cs" Exclude="$(DefaultItemExcludes);$(DefaultExcludesInProjectFolder);$(Compile)" />
</ItemGroup>

This should go after any Compile items are declared.

This should get you back to the pre-2.1.300 SDK behavior, where all the source code will show in the solution explorer, but various things (adding new items, moving, renaming) in the IDE may not work or may work strangely.

So I don't recommend adding this arbitrarily to all projects, just projects where you have TargetFramework-specific source files that you want to show up in solution explorer.

dsplaisted commented 6 years ago

I'm going to close this issue, as we believe we understand the behavior change here, and the real fix is for solution explorer to show files from all the target frameworks, which is tracked by https://github.com/dotnet/project-system/issues/935.

jamesmontemagno commented 6 years ago

Can confirm the workaround works.

ericstj commented 4 years ago

Small nit on the workaround: shouldn't that be @(Compile) not $(Compile)?