Fixed: Building NMA with System Extractor

[Sewer56]

CC @MattSturgeon

I got some time to kill end of sprint.

This fixes the behaviour of NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR to correctly function.
I can verify the following:

1. No 7zz in output directory
2. System extractor is being used (easy to verify by just uninstalling it).

In addition, I have altered the code to also account for system binaries possibly being called 7z and 7zzs.
(Note: On my Arch system, the AUR package renames it to 7z from 7zz in original download)

The code does a lookup of $PATH, and determines the appropriate binary based on whatever it can find first.

fixes #1836

[Sewer56]

With respects to building, you can't modify DefineConstants if it's specifically set in the CLI via -p:DefineConstants="...".
This seems to have been tripping me up too. So we can't do an alias by appending DefineConstants in the .csproj or .targets file. Instead I alias by checking both define names; it's shorter in terms of line count either way.

[erri120]

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

[MattSturgeon]

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 😀

[erri120]

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.

[MattSturgeon]

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

[Sewer56]

@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.

[Sewer56]

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.

[MattSturgeon]

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.

[Sewer56]

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.

[Sewer56]

Update: We'll be avoiding static bundling in the future; will check dependencies at runtime and emit a health check/diagnostic. For downstream's convenience, this is merged for now.