CZEMacLeod / MSBuild.SDK.SystemWeb

This MSBuild SDK is designed to allow for the easy creation and use of SDK (shortform) projects targeting ASP.NET 4.x using System.Web.
MIT License
151 stars 8 forks source link

Only include web.$(Configuration).config where xxxx exists on disk. Fixes #67 #68

Open CZEMacLeod opened 11 months ago

CZEMacLeod commented 11 months ago

Ensures that web.*.config files automatically included in None for each configuration, actually exist on disk to prevent missing files showing in the solution tree and issues with TVFS.

This should resolve #67

CZEMacLeod commented 11 months ago

I'll be pulling in a few PRs together, including #67 at a minimum to make it easier to do releases.

leusbj commented 11 months ago

@CZEMacLeod I know that I had originally proposed that the "Debug/Release/BindingRedirects" as None objects... but the "DependentUpon" property will also prevent these items from being included in the published/packaged site even if they are Content. As a note of interest, I noticed that Visual Studio MVC/WebAPI/SPA app templates use Content for the Web.Debug.config and Web.Release.config.

Is it worth just simplifying the globbing of .Config files to something like this that contains a wildcard... so it will only ever pull in files that do exist on disk... and then update ones that we believe should not be placed in the puiblished/packages web application

<ItemGroup Condition="'$(EnableWebFormsDefaultItems)'=='true'">
   <_WebConfigConfiguration Include="$(Configurations)" />
   <_WebConfigConfiguration Include="BindingRedirects" />

   <Content Include="Web*.config" Exclude="$(DefaultItemExcludes);$(DefaultExcludesInProjectFolder)" />
   <Content Update="@(_WebConfigConfiguration->'Web.%(Identity).config')" >
      <DependentUpon Condition="EXISTS('Web.config')">Web.config</DependentUpon>
   </Content>    
</ItemGroup>
CZEMacLeod commented 11 months ago

@leusbj Interesting idea. I did consider adding "BindingRedirects" to the configurations list, but I felt that it should be handled separately (even though the logic is basically the same). I'm not sure if a WAP will ever not have a web.config file, but for the sake of completeness, I guess the DependentUpon Condition is a good idea.

I think I will use your idea of just using the DefaultItemExcludes and DefaultExcludesInProjectFolderproperties instead of introducing a new one withExcludeWebConfigConfiguration`.

Unfortunately, while I like the idea of using DefaultItemExcludes, we actually add web.*.config to this property and therefore cannot use it in the itemgroup.

I did feel that there may be other uses for the _WebConfiguration item group around applying transforms; I have some stuff in another project I use to replace slowcheetah, which applies multiple transforms based on the selected configuration and the local computer name. e.g. if the configuration was Debug.SQL2019, and the PC was MyPC, it would look for (and apply in order), web.Debug.config, web.Debug.SQL2019.config, web.MyPC.config, and web.Debug.SQL2019.MyPC.config. allowing you to have customized settings for specific developer machines, and not repeat settings in each configuration that is 'derived' from another. On a build server you can pass in a custom pc name such as BuildServer and apply say web.Release.config and web.Release.BuildServer.config.

All of that is out of scope for this PR though, but I wanted to explain my thought process. Is there a performance, or other reason you feel that keeping these files as None is a problem?

leusbj commented 11 months ago

@CZEMacLeod No performance reason. Just wasn't sure if treating them differently was actually important anymore... and if it wasn't there might be a straight forward approach to glob them all

CZEMacLeod commented 11 months ago

@leusbj The thing is, you want the web.*.config files which aren't related to the configuration to be content and included in the packaged outputs so must not have the DependentUpon set. This means that when you deploy them, they can be used by web deploy. So, you do need to handle the configuration and binding files differently.

leusbj commented 11 months ago

@CZEMacLeod Sorry for confusion. My phrase "treating them differently", was geared towards they don't need to be in different ItemGroups. That they could all be Content and as long as the DependentUpon was set (or not set) correctly for the desired outcome, then everything would work.

In my mind the primary reason I had initially proposed putting the package/publish transformations as None was a "convenience" factor. That if for some reason, a project had a build configuration that exactly matched a release name... the developer could use the GUI Item Properties pane to switch the ItemGroup from None to Content. Then our logic, which automatically applies the DependentUpon property ONLY affects None Items, would ignore that item... and so that item would start to appear in the package for release pipelines to use later.

What you have looks to work (plus you have insight into the slow-cheetah ideas that I definitely didn't consider).