Repository navigation
Nonsense cmake configuration messages about "NaCl architectures" #2031
Description
Snippet from the configure output:
-- Detected C++ compiler: MinGW_16.2.0/GCC_16.2.0:c++.exe
-- Enabling compiler intrinsics
-- Enabling compiler custom attributes and operators
-- Available NaCl architectures: amd64
-- Primary NaCl architecture: amd64
-- Available NaCl architectures: i686;amd64
-- Primary NaCl architecture: i686
-- No known NaCl architecture
-- Available NaCl architectures: amd64
-- Primary NaCl architecture: amd64
-- Available NaCl architectures: i686;amd64
-- Primary NaCl architecture: i686
-- No known NaCl architecture
-- Building NaCl targets: amd64;i686
-- Could NOT find Freetype (missing: FREETYPE_LIBRARY FREETYPE_INCLUDE_DIRS)
These messages were added in some of the unreviewed commits from #2018.
This is an amd64 build so it does not make sense to claim that i686 is a native architecture.
No NaCl build subdirectories exist yet so it's not coming from a sub-build.
Activity
illwieckz commentedon Nov 4, 2026
From the missing informations, I guess this uses the default
BUILD_GAME_NACL_TARGETS=allcmake option, so the actual functional bug is that thearmhfnexe is not listed.Both
amd64andi686nexe are meant to be built whenBUILD_GAME_NACL_TARGETS=allis used. The log is consistent with thei686nexe being explicitly set to be built (which is expected whenallis used).The rest (overly verbose log and wrong native wording) is useless noise that will disappear once the root cause of the bug preventing the armhf nexe to be built will be fixed after merging:
The bug was that the native nexe detection was done on all enabled NaCl targets (so
amd64;i686;armhfasallby default), instead of doing it only with thenativeoption. I guess that with running the unneeded detection adding such unwanted log, that overwrote some list and deletedarmhffrom it.We can't prevent some log duplication because this is done once per game (cgame and sgame), but in this case the said duplicated log should have not been printed with default options, to begin with.
As a side note, having done this test while the PR was open for review would have been an acceptable review.
A review does not necessarily requires from reviewers to duplicate the problem solving effort the original author invested.
Outside of security concerns, verifying that the contract is satisfied by testing input and output is also an acceptable kind of review and can be sufficient in many cases.
For what I understood, #2018 lacked review because the review expectation was too high for the actual matter (cmake code setting variables). To avoid more pull requests being stuck in review limbo, we should decrease the review cost when the actual kind of contribution makes it possible. Not everything is crypto or things like that.
illwieckz commentedon Nov 4, 2026
As a side note, on my end the docker release built had built all the nexe, including armhf. That explains why I had not caught this, as my testing was done using the docker release build. I don't know which variation made it works in the docker build but that's not a problem since the bug is meant to be fixed in #2033.