There should only be one constant. Either NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR or USE_SYSTEM_EXTRACTOR, not both.

Speaking as a downstream packager, I'm fine with these kinda changes being done in a breaking way so long as the docs get updated and the change is mentioned in release notes.

Packagers shouldn't be surprised when experimental software has breaking changes 😀

There should've only been one compile constant in the beginning, which is NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR. I still don't understand what caused #1836.

There should've only been one compile constant in the beginning, which is NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR.

Ah yes, that's what I was using. I got confused as the diff made it look like NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR was a new constant replacing USE_SYSTEM_EXTRACTOR. Should've double checked before commenting!

I don't fully understand the solution, but this seems relevant #1919 (comment):

So we can't do an alias by appending DefineConstants in the .csproj or .targets file. Instead I alias by checking both define names

@erri120 themselves originally added both USE_SYSTEM_EXTRACTOR and NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR.

Due to how they were used in the .targets file, I assumed it was meant to be an alias.
I kept this alias in the PR; should I not have done that?

AFAIK downstreams only use NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR and not USE_SYSTEM_EXTRACTOR, so the latter is safe to remove.

I still don't understand what caused #1836.

@erri120 Setting DefineConstants via CLI acts as an 'absolute override', i.e. any changes you make to them, in .csproj, .targets or otherwise get discarded. So USE_SYSTEM_EXTRACTOR was not being applied if conditionally set.

And aside from that _UseSystemExtractor wasn't even being determined properly, so binaries were always copied to output even when they should not have been.

Setting DefineConstants via CLI acts as an 'absolute override', i.e. any changes you make to them, in .csproj, .targets or otherwise get discarded.

Possibly out of scope for this PR, but is that a possible justification for moving to the pattern described in NixOS/nixpkgs#331150 (comment)?

I.e:

  <PropertyGroup Condition="$(UseSystemExtractor) == 'true'">
    <DefineConstants>$(DefineConstants);NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR</DefineConstants>
  </PropertyGroup>

Paired with dotnet build -p:UseSystemExtractor=true.

Personally I would prefer it that way, after (re)discovering more of the nature in how DefineConstants interacts with the rest of the build system.