cmake/DaemonGame: set FORK value 1 in current scope, as it is used and only used in current scope

[illwieckz]

Fix Unvanquished/Unvanquished#3355:

• nacl builds are broken Unvanquished/Unvanquished#3355

This is a dormant bug that was already there for a long time (before #1621) , the set(FORK 1 PARENT_SCOPE) can already be found in commit de443dc from Unvanquished/Unvanquished#771 ten years ago.

But because of some luck, the bug wasn't active. Some refactor woke up that bug. I don't know exactly what woken it up and when.

The bug is that the Unvanquished CMakeLists.txt calls the GAMEMODULE() function, which does set(FORK 1 PARENT_SCOPE), meaning FORK is set to 1 in CMakeLists.txt, not in GAMEMODULE(), and GAMEMODULE() test for FORK being 1 to add the nacl-vms target.

The reason why it worked when both cgame and sgame is built is because it had this side effect:

1. CMakeLists.txt calls GAMEMODULE("sgame") that sets FORK to 1 in CMakeListst.txt scope (not in GAMEMODULE() scope), then GAMEMODULE() doesn't add the nacl-vms target because FORK isn't set in GAMEMODULE() scope.
2. CMakeLists.txt calls GAMEMODULE("cgame") and GAMEMODULE() inherit the FORK being previously set to 1 from the CMakeListst.txt scope, then GAMEMODULE() sets FORK to 1 in CMakeListst.txt scope again, then GAMEMODULE() adds the nacl-vms target because FORK being 1 was inherited from CMakeLists.txt as set by the previous GAMEMODULE() call.

[illwieckz]

It possible that what woke the bug up was 328e89e from #1468:

• cmake: remove a warning about scope #1468

As said by @slipher:

NACL_TARGETS is set in Daemon's CMakeLists, but consumed by DaemonGame.cmake which is executed in the context of Unvanquished's CMakeLists. So I expect this to be needed for the variable to show up (including daemon with add_subdirectory is what makes it have a separate scope). Otherwise we would have the same problem as with NACL_VMS_PROJECTS being empty.

Probably the right thing to move the code seting NACL_TARGETS inside DaemonGame.cmake so we don't run this code for building gamelogic when only the engine is built.

Before #1468, some code that belongs to GAMEMODULE() was in Unvanquished/daemon/CMakeListst.txt for no reasons, so telling GAMEMODULE() to set FORK to 1 in Unvanquished/CMakeLists.txt would then make it set for Unvanquished/daemon/CMakeListst.txt if things were luckily done in the right order. I'm not 100% sure it's that because the code moved in #1468 doesn't read or set FORK.

Anyway, this PR #1468 or another one like that (or a combination of this one and some others) is likely what woke up that bug, as the code was using very convoluted ways to do things. Turning this tortured and shady labyrinth into an highway had left some road signs lost in some forgotten corners, now we should be fine.

[illwieckz]

The FORK variable was the yellow marble here: https://www.youtube.com/watch?v=ytV76_ZWprQ

[slipher]

Unvanquished CMakeLists.txt uses FORK

[illwieckz]

Good catch!

Though the Unvanquished CMakeLists.txt uses the FORK value 2 which is set by the cmake command line when running the NaCl build subproject.

The FORK value 1 isn't used outside of GAMEMODULE().

We may rename that FORK=1 to something else at some point to avoid confusion with FORK=2.

[illwieckz]

I renamed the commit accordingly: this is FORK with value 1 which is set in current scope.

[slipher]

I tried and and get this:

CMake Error at /usr/share/cmake-3.25/Modules/ExternalProject.cmake:3985 (add_custom_target):
  add_custom_target cannot create target "nacl-vms" because another target
  with the same name already exists.  The existing target is a custom target
  created in source directory "/unv/Unvanquished".  See documentation for
  policy CMP0002 for more details.
Call Stack (most recent call first):
  daemon/cmake/DaemonGame.cmake:142 (ExternalProject_Add)
  daemon/cmake/DaemonGame.cmake:240 (gameSubProject)
  CMakeLists.txt:231 (GAMEMODULE)

[illwieckz]

OK, I know what to do.

[slipher]

See #1625

[illwieckz]

This should be enough:

-       if (FORK EQUAL 1)
+       if (FORK EQUAL 1 AND NOT TARGET nacl-vms)

I don't get why you set the variable in the parent scope.

[slipher]

This should be enough:

-       if (FORK EQUAL 1)
+       if (FORK EQUAL 1 AND NOT TARGET nacl-vms)

Using TARGET would be a pain because it's different with Saigo.

I don't get why you set the variable in the parent scope.

The point is that FORK needs to survive until the second time GAMEMODULE is called, to see whether it has been called already or not.

[illwieckz]

Using TARGET would be a pain because it's different with Saigo.

The Saigo CMake code creates an nacl-vms target on purpose to be the parent of other per-architecture targets to keep target names reliable and command line build interface consistent…

I don't get why you set the variable in the parent scope.

The point is that FORK needs to survive until the second time GAMEMODULE is called, to see whether it has been called already or not.

On my end FORK=1 is set on every GAMEMODULE call, we only need to know that FORK isn't 2 as far as I know.

[illwieckz]

We better do that instead, so the else() doesn't run because the target is already defined.

	if (NACL_VMS)
		if (TARGET nacl-vms)
			return()
		endif()

Here I renamed FORK=1 with NACL_VMS=ON, I really doubt we need a variable with many values and a variable set in multiple scopes.

[illwieckz]

For information, this:

	if (NACL_VMS)
		if (TARGET nacl-vms)
			return()
		endif()

		# …

		if (BUILD_GAME_NACL)
			if (USE_NACL_SAIGO)
				# …
			else()
				# …
			endif()
		endif()
		
	elseif (FORK EQUAL 2)
		# …
	endif()

means we are already ready to do that:

	if (NACL_VMS)
		if (TARGET nacl-vms)
			return()
		endif()

		# …

		if (BUILD_GAME_NACL)
			if (USE_NACL_SAIGO)
				# …
			else()
				# …
			endif()
		elseif (BUILD_GAME_WASM)
			# …
		endif()
		
	elseif (FORK EQUAL 2)
		# …
		if (BUILD_GAME_NACL)
			if (USE_NACL_SAIGO)
				#…
			else()
				# …
			endif()
		elseif (BUILD_GAME_WASM)
			# …
		endif()
	endif()

I remind that I initially made possible to also build native dll as subproject with this design (it is removed, but it would be as easy to add).

Here NACL_VMS and nacl-vms would be misnamed but that's just a matter of names.

[illwieckz]

Obsoleted by:

• Fix NaCl build breakage #1625