DirectXTex icon indicating copy to clipboard operation
DirectXTex copied to clipboard

Make the NuGetConfiguration explicit for Release build

Open Rashmatash opened this issue 3 years ago • 6 comments

DirectXTex NuGet package doesn't point to the correct library location in projects with configuration names other than "Debug" and "Release". The developer can provide the correct location through the use of NuGetConfiguration property. However, it's overwritten by this line in the ".targets" file:

<PropertyGroup Label="Non_Debug" Condition="'$(Configuration.ToLower())' != 'debug'">

Please change this to explicitly compare to "release":

<PropertyGroup Label="Release" Condition="'$(Configuration.ToLower())' == 'release'">

P.S. this is also applicable to DirectX tool kit NuGet packages.

Rashmatash avatar Aug 02 '22 15:08 Rashmatash

Changing the comparison operator and string just changes the problem (what if I have a build called Release but in my weird opposites world I want debug libs). I would suggest it may be better to fix this by only setting NuGetConfiguration if it didn't already have a value set too (so it could be overriden in the project file before it gets there and that value remains untouched).

e.g. something like

sausag3 avatar Aug 02 '22 21:08 sausag3

The .targets assumes configurations like 'Debug', 'Release', and 'Profile'. This is a pattern I've seen in a number of other projects as well.

While the suggestion from @sausag3 is one option, the same property <NuGetConfiguration> is often used in other .targets files so might be set to something nonsensical.

walbourn avatar Aug 02 '22 22:08 walbourn

Makes sense. The way .targets are right now, they assume 'debug' and 'non-debug'. How about changing that to

<PropertyGroup Label="Debug" Condition="'$(Configuration.ToLower().Contains('debug')"> and <PropertyGroup Label="Release" Condition="'$(Configuration.ToLower().Contains('release')">

so at least it doesn't default to release folder for debug build configurations that aren't just called 'debug'?

Rashmatash avatar Aug 03 '22 07:08 Rashmatash

That's potentially now creating a different problem, what if someone has "NotRelease" as a config name? The safest solution is going to be the project file explicitly specifying either Debug or Release to this in a way that overrides the targets file making the decision.

@walbourn Instead of checking if NuGetConfiguration is not '' how about something named a little more package specific? I see the Agility SDK using variables like 'Microsoft_Direct3D_D3D12_SkipIncludeDir', how about either we can pass in a variable like 'Microsoft_DirectXTex_Configuration' which would have the value you want, or 'Microsoft_DirectXTex_SkipSettingConfiguration' which would ignore setting it and just use whatever the project had set to.

I should add I also have a project that suffers from this issue and I'd been meaning to file a bug on it (and have just realized that's on one of the DirectXTK12 projects and not this one :) ). I've currently hacked around it by removing the Import of the targets file and manually setting up the Include/Lib paths myself (only needs to be in the one project that actually links), but this is less than ideal and is a pain if/when I need to update the NuGet package. In my instance we have two different Debug builds (the other still has Debug in its name), so we also tripped over the "it's exactly debug or it must be release", so would like to find a good solution for everyone.

sausag3 avatar Aug 03 '22 08:08 sausag3

I'd suggest using the Contains() solution to cover the majority of projects, combined with package specific names to tackle less common config names as you mentioned.

Rashmatash avatar Aug 03 '22 11:08 Rashmatash

The DirectX Agility SDK and Windows SDK don't have any "configuration" logic. They are just "SDKs" delivered as a NuGet instead of a library.

I'll see if there's another pattern to follow here. The solution I'm using is one I've seen used in many projects.

walbourn avatar Aug 03 '22 20:08 walbourn

How about:

  <PropertyGroup Label="Debug" Condition="'$(Configuration.ToLower())' == 'debug'">
    <NuGetConfiguration>Debug</NuGetConfiguration>
  </PropertyGroup>
  <PropertyGroup Label="Non_Debug" Condition="'$(Configuration.ToLower())' == 'profile'">
    <NuGetConfiguration>Release</NuGetConfiguration>
  </PropertyGroup>
  <PropertyGroup Label="Non_Debug" Condition="'$(Configuration.ToLower())' == 'release'">
    <NuGetConfiguration>Release</NuGetConfiguration>
  </PropertyGroup>
  <PropertyGroup Condition="'$(NuGetConfiguration)' == ''">
    <NuGetConfiguration>Release</NuGetConfiguration>
  </PropertyGroup>

walbourn avatar Oct 17 '22 23:10 walbourn

https://github.com/microsoft/DirectXTex/commit/54af4abdf89f90b9d8f7cab5f227fb0f0cce5bba

https://github.com/microsoft/DirectXTK/commit/5396a9c4099177d49e739bbee05c50d656564e7e

https://github.com/microsoft/DirectXTK12/commit/e10e06cb985b91185edf2870b3dbe85e804e96c6

https://github.com/microsoft/DirectXMesh/commit/3dcfbab1d88fc737422e0e3464755eff40d8c23d

https://github.com/microsoft/UVAtlas/commit/13a0760265cbe4b3463d7011f00e50b3f824d162

walbourn avatar Oct 18 '22 00:10 walbourn

How about:

  <PropertyGroup Label="Debug" Condition="'$(Configuration.ToLower())' == 'debug'">
    <NuGetConfiguration>Debug</NuGetConfiguration>
  </PropertyGroup>
  <PropertyGroup Label="Non_Debug" Condition="'$(Configuration.ToLower())' == 'profile'">
    <NuGetConfiguration>Release</NuGetConfiguration>
  </PropertyGroup>
  <PropertyGroup Label="Non_Debug" Condition="'$(Configuration.ToLower())' == 'release'">
    <NuGetConfiguration>Release</NuGetConfiguration>
  </PropertyGroup>
  <PropertyGroup Condition="'$(NuGetConfiguration)' == ''">
    <NuGetConfiguration>Release</NuGetConfiguration>
  </PropertyGroup>

This still doesn't solve the problem if the debug config name is not exactly 'debug', which is why I created the bug repost in the first place ;)

Edit: I worked around it by adding <NuGetConfiguration>Debug</NuGetConfiguration> in my vcxproj file. I guess it's good enough. Thank you so much for changing the script! :)

Rashmatash avatar Oct 18 '22 08:10 Rashmatash