Added: Ability to restrict which games we ship the Application with for Release Builds

[Sewer56]

This PR introduces the ability to selectively enable or disable games from our Release builds.

We do this by straight up not compiling them into the App with the use of compile-time constants.

Changes (Summary)

• Added a new Directory.Build.targets file to define compile-time constants across projects.

  • Currently only Stardew Valley support is enabled in Release builds.
  • In Debug builds, all games are enabled by default. (Requested by @halgari)

• Updated the NexusMods.App.csproj file to only include project references when the relevant constants are defined.

• Moved conditionally enabled games to AddSupportedGames in Services class to keep things cleaner.

• Removed some unused using directives in startup project.

• Resolved cases where NuGet package(s) of now conditionally enabled projects were being imported transitively in tests, rather than directly. (Fix Build Errors)

• Added missing DEBUG conditional in MainThreadData.cs which would cause compile errors if unused using(s) were removed.

Note

We couldn't use Directory.Build.props, it seems the constants we define there are overwritten further down the road, so I instead added a Directory.Build.targets file as that comes later down the build logic.

[Sewer56]

fixes #1367

[erri120]

I really dislike this design. The issue #1367, which this PR was supposed to resolve, is about not showing any other game than Stardew Valley in the UI. Why did you use compile constants to hack project references instead of just having a simple check in the UI to now show any other game besides Stardew Valley?

This could've been a one line PR, instead this is a hacky and unmaintainable solution which doesn't just exclude games from the UI, but from the entire build process.

I'm vehemently against this approach.

[Sewer56]

Edit: I need to note that I forgot to push a commit before PR-ing this, so I fixed that now.

I will agree that #if FLAG #endif can be a bit of an eyesore, but I disagree over the proposed solution.
I'll explain the reason why at the very end. For now, I want to address some points.

instead of just having a simple check in the UI to now show any other game besides Stardew Valley?

I've been hearing that we're going to be enabling games one by one.
So first we do SDV. Then we do SDV AND Cyberpunk. etc.
This will make it easier to configure what we ship with.

I know that has no bearing on the suggested approach, but it's part of the reason why I did things this way.

is a hacky and unmaintainable solution

Why is this solution hacky and unmaintainable?

The process of adding a game (compared to before) involves the following extra lines:

• Adding a name+description to Directory.Build.targets (2 lines)
• An #if and #endif to Services.cs (2 lines)

That's it.
And we rarely change this stuff.

In addition we get to keep the previously undocumented overrides such as NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR in a central location.

And no, <PropertyGroup Condition="'$(Configuration)' == 'Debug'"> is not redundant. We will want to include functionality such as creating multiple loadouts here that is enabled in our debug builds but not released ones.

Why did you use compile constants to hack project references

I chose to not compile the other projects, yes.
But it is not mandatory.
It has no effect on runtime behaviour or whether the code compiles or not.

I did it simply to avoid shipping dead code, so our precious end users trying the Alpha can get set up and running faster.

If you really want to be pedantic about it however, I could:

• Slap a single Conditional attribute on each game's Add<Game> function.
• Remove out the conditional project builds.

Then the 'eyesore' of a bunch of #if at the bottom of Services.cs would be gone.

This could've been a one line PR

No, because there are certain places in the App where we may use the likes of dependency injection to obtain collections of entities. Or, otherwise operate on DI injected entities.

Some examples:

• The CLI requests IEnumerable<IGame> for listing games. So you would have to patch that.
  • That is also a listing of 'Supported Games'.
• The Registry (IGameRegistry) requests IEnumerable<ILocatableGame> games which would also need filtering.
• Anything that has a converted IGame game would need patching, (e.g. ManageGame CLI command).
  • So the user can't create a loadout for an 'unsupported' game via the CLI.

And there may be more locations.
That is more work objectively. I do not want to be playing whack-a-mole adding small blocks of code to restrict the inputs all over the place as we add more features.

The easiest way to prevent that is simply not adding the games in the first place, which I chose to do here.

---

Yes, you may argue that the ticket text refers to the UI if you want to be really, really pedantic about it.

However, the intent is:

• We only want people to mod Stardew Valley (for now)
• We only want Stardew Valley Feedback (for now)

If I were to leave such gaping holes in the implementation such as allowing people to interact with 'unsupported' games via the CLI, then I'm not doing a good enough job.

[halgari]

instead this is a hacky and unmaintainable solution which doesn't just exclude games from the UI, but from the entire build process.

@erri120 Can you go into more detail on the problems this will cause? Compile time switches for disabling modules is fairly common in a lot of applications. In the future we may want more options than just "Debug" or "Release" but that also is rather maintainable. Looking at this code, adding a new game will require adding abour 4-5 lines of code. I guess I'm not understanding where the hacks or unmaintainability comes into play here.

[erri120]

Can you go into more detail on the problems this will cause? Compile time switches for disabling modules is fairly common in a lot of applications. In the future we may want more options than just "Debug" or "Release" but that also is rather maintainable. Looking at this code, adding a new game will require adding abour 4-5 lines of code. I guess I'm not understanding where the hacks or unmaintainability comes into play here.

The #if preprocessor directive makes the code conditional, while the C# if statement makes the behavior conditional. Using conditional compilation means we now have multiple compilation configurations that result it different binaries. When we develop new features, we're using the DEBUG configuration, which enables all #if DEBUG blocks. Our IDE won't be able to report issues inside #if RELEASE blocks, because those have been conditionally excluded from compilation. Not only will our IDE not report issues inside these conditionally excluded blocks of code, it will also issue new warnings and maybe errors because of unused imports, classes, or other statements. @Sewer56 encountered this issue in this PR, he had to add a missing #if DEBUG directive to the imports in MainThreadData, because the compiler would otherwise complain in RELEASE mode.

Our IDE isn't the only place where these issues will pop up. Our build pipelines all use different configurations. The CI that runs on each PR uses DEBUG, our Git Builds pipeline uses RELEASE while our release pipeline uses RELEASE as well as our custom compile constants for INSTALLATION_METHOD_*. The binaries we get out of the Git Builds pipeline don't even match the ones we get out of the release pipeline because of conditional compilation.

I'm calling conditional compilation unmaintainable because you have to test each and every compilation configuration to determine whether the code compiles correctly and does what it's supposed to do. In contrast, using C# if statement means we don't have to worry about compilation and can focus on the behavior of our code.

As for this being "hacky", preprocessor directives are rarely a good choice, and never the only one. I regret adding NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR in #1308, that should've been a setting or something else, similar to the INSTALLATION_METHOD_* compile constants from #488. I want to avoid going deeper with preprocessor directives due to the reasons I wrote above.

Let's talk about alternatives. As mentioned before, this PR is trying to resolve #1367:

The supported games section is currently showing games we don't support.
For the Alpha please make sure its only showing Stardew Valley.

For the alpha release, we want an "on rails"-experience, meaning users can only interact and use the things we want them to test. In this case, we want users to manage Stardew Valley from the UI, not Skyrim, or Cyberpunk, or Sifu.

To remove other games from the UI, it's a one-line change:

NexusMods.App/src/NexusMods.App.UI/Pages/MyGames/MyGamesViewModel.cs

Lines 59 to 60 in 0c8f1f9

	var foundGames = gameRegistry.InstalledGames
	.ToObservableChangeSet();

var foundGames = gameRegistry.InstalledGames
                    .ToObservableChangeSet()
                    .Filter(x => x.Game.Domain != "stardewvalley");

Of course there should be a comment here linking to #1367, and similar with #1286, this could also have a setting. Either way, we only want Stardew Valley to appear in the UI, @Sewer56 mentioned that this affect the CLI, and I honestly don't think we care. If a user uses the CLI, then they're off the rails anyways. No point in adding restrictions there.

If you really wanna keep conditional compilation, then I might be amenable to these changes if we remove all the custom NEXUSMODS_APP_ENABLE_* constants and just use DEBUG. In DEBUG you get all games, in RELEASE you only get Stardew Valley. This reduces the amount of possible compilation configurations, while having the same effect.

Lastly, I believe the correct approach should be either settings, or feature flags using tools the other teams are already using, namely Unleash. Although, feature flags don't really make a ton of sense in non-web applications, we should definitely consider them if we run into something like this again.

[Sewer56]

he had to add a missing #if DEBUG directive to the imports in MainThreadData, because the compiler would otherwise complain in RELEASE mode.

I have to note, that this was actually unrelated to the PR.
On the main branch, MainThreadData has

public static readonly bool IsDebugMode = Debugger.IsAttached;

But at the top of the file, you have only

using System.Diagnostics;

If you had your project set to Release mode and ran the cleanup task Remove unused using statements (either via Rider or by using dotnet format), that would generate a compile error when you switched back to Debug because the statement had to be eliminated. I noticed that error while cleaning up, so I fixed it.

It wasn't something I had to do, I just noticed it and fixed it.

Not to detract from the statement made, because it's a good example of how improper use of conditional compilation can break stuff.

---

In any case, the idea I had in mind when writing this code was that:

• Debug (DEBUG) has all of the constants enabled
• Constants only add extra code, not remove any code

That means that Debug is a superset of Release.
When we compile for Debug, we build all of the Release unaltered, and then extra.
That means our IDEs, build warnings, etc. pick up all the code during the development phase.

I would also recommend that any conditional code should also inline the namespaces if it introduces additional imports:
e.g.

// Namespace prevents invalid cleanup on 'remove unused using statements' 
Games.BethesdaGameStudios.Services.AddBethesdaGameStudios()

What we must not do is disable code using directives, because that creates additional combinations of flags that would have to be tested in CI. And doing that is in fact not fun to maintain as @erri120 stated, but I am not doing that here. I really do not want to be doing that. The number of combinations remains 2; and one is a superset of the other.

I could also change the directives in the code to be in the form #if (NEXUSMODS_APP_ENABLE_BETHESDA || DEBUG), and remove the debug block from the Build.targets instead. It may perhaps be better as it would clarify intent more explicitly.

[erri120]

I could also change the directives in the code to be in the form #if (NEXUSMODS_APP_ENABLE_BETHESDA || DEBUG), and remove the debug block from the Build.targets instead. It may perhaps be better as it would clarify intent more explicitly.

Just remove the NEXUSMODS_APP_ENABLE_* constants completely and only have DEBUG and RELEASE. Less is better when it comes to preprocessor directives, we gain nothing from having a more complex build system. It might be more flexible to pick and choose, but realistically, we don't need that. We have a list of games we support, that currently only contains Stardew Valley, and that list will only grow.

[Sewer56]

Eh, sure. Gimme a moment.
Will leave .targets file though as a listing of constants.

[halgari]

Thanks for the details @erri120, and I agree, stuff like this was the reason why we ditched Vogen. In that vein, we should proabably do the following:

@Sewer56 so we're all on the same page

1. Setup some sort of way to mark games as "officially supported"
2. Only show "officially supported" games in the "manage game UI", we won't worry about the rest
3. Add a config option so we can turn off that fiter in testing/development