gentables: only unset compiler flags if cross-compilation is detected

[fabiangreffrath]

No description provided.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[derselbst]

The intention behind your change isn't clear.

I don't see how CMAKE_CROSSCOMPILING could ever become true. gentables must always be compiled for the host. It's invoked as a separate process, there is no knowing if the calling cmake process was cross compiling or not.

[fabiangreffrath]

CMAKE_CROSSCOMPILING becomes true if CMake is called with a toolchain file. Following the comments in gentables/CMakeLists.txt, the CC variable gets unset to "force cmake to look for a (working) C compiler, which hopefully will be the host compiler". However, this also resets reasonable CFLAGS for non cross-builds, where host and build environment are the same. Thus, this stunt should only be performed when actually cross-compiling.

[fabiangreffrath]

I don't see how CMAKE_CROSSCOMPILING could ever become true

https://cmake.org/cmake/help/latest/variable/CMAKE_CROSSCOMPILING.html

[derselbst]

CMAKE_CROSSCOMPILING becomes true if CMake is called with a toolchain file.

gentables cmake project is invoked from here:

fluidsynth/src/CMakeLists.txt

Lines 557 to 565 in de1daa4

	ExternalProject_Add(gentables
	DOWNLOAD_COMMAND ""
	SOURCE_DIR ${GENTAB_SDIR}
	BINARY_DIR ${GENTAB_BDIR}
	CONFIGURE_COMMAND
	"${CMAKE_COMMAND}" -DCMAKE_VERBOSE_MAKEFILE=${CMAKE_VERBOSE_MAKEFILE} ${EXPLICIT_HOST_COMPILER_STR} -G "${CMAKE_GENERATOR}" -B "${GENTAB_BDIR}" "${GENTAB_SDIR}"
	BUILD_COMMAND
	"${CMAKE_COMMAND}" --build "${GENTAB_BDIR}"
	INSTALL_COMMAND ${GENTAB_BDIR}/make_tables.exe "${FluidSynth_BINARY_DIR}/"

As you can see, neither a toolchain file, nor CMAKE_SYSTEM_NAME is passed. Hence, I still don't see how CMAKE_CROSSCOMPILING could ever become true in the gentables project.

However, this also resets reasonable CFLAGS for non cross-builds, where host and build environment are the same

Why does this even matter? make_tables is a simple program that generates compile-time known lookup tables as .c file. It's not installed, not distributed, etc. Why would you possibly care about CFLAGS passed to this project?

[fabiangreffrath]

Why does this even matter? make_tables is a simple program that generates compile-time known lookup tables as .c file. It's not installed, not distributed, etc. Why would you possibly care about CFLAGS passed to this project?

A build log checker, some tool that we run as part of our Debian packaging CI, detected and complained that parts of the code were not built with the hardening flags sets during packaging:

https://salsa.debian.org/multimedia-team/fluidsynth/-/jobs/7012907

[derselbst]

A build log checker, some tool that we run as part of our Debian packaging CI, detected and complained that parts of the code were not built with the hardening flags sets during packaging:

I would have appreciated it if this had been your very first comment for this PR, rather than presenting a solution without a problem.

IMO, it is inappropriate to run such a tool on the entire build directory. Instead, you should only run it on the buildroot, so that only those files which have been actually installed into the system and therefore distributed with your package are taken into account.

Your proposed change is inappropriate as it doesn't work and actually breaks cross-compilation, see the Android CI ARM and AARCH64 pipelines. Because of that, I'm rejecting your PR.

[fabiangreffrath]

Alright, I have to admit that my patch was a bit half-baked, as in, well, broken. But, as a compromise, could we agree on a different approach maybe? If the user has the FLUID_HOST_COMPILER variable passed to CMake, could we please agree that we expect him to know what he is doing and omit the resetting of the host compiler flags in this case?

[derselbst]

If the user has the FLUID_HOST_COMPILER variable passed to CMake, could we please agree that we expect him to know what he is doing and omit the resetting of the host compiler flags in this case?

That wouldn't work either, because in a cross compilation scenario, CFLAGS have been specifically set for the cross compiler. If you simply pass them on to the host compiler they can cause build problems.

IMO, the only way to handle this is to introduce yet another cmake variable which would allow you to specify CFLAGS for the host compiler, similar to FLUID_HOST_COMPILER. However, i see no benefit or reason for adding such a change into the upstream repo, since this use-case originates from your debian-specific way to compile fluidsynth. I know from RPM based distros that they run a similar tool to verify if the optflags have been included in the CFLAGS. But they restrict their analysis to the build root, hence they are not facing this problem.

The most straight forward way for you would be to create a patch that just removes all the unset calls. This obviously breaks cross compilation, but if you don't use that, no need to care.

I really hope that one day we'll start to make a first step towards C++11, which would allow these lookup tables to become constexpr and to get rid of this gentables dance altogether.

[fabiangreffrath]

Gretchenfrage: Why do you need to regenerate these tables during each compilation at all? Is there anything non-portable or non-constant about them? Why don't you just save the generated headers and keep the instructions around how to rebuild them if ever necessary?

[derselbst]

As long as the implementation behind those lookup tables doesn't change, they are constant, hopefully identical for all architectures, and one might version control them. However, I consider it wrong to version control autogenerated files, just like I consider it wrong to version control any kind of build artifacts. If I make a change to the underlying logic anywhere in the code, I expect this change to take effect the next time I compile the software, without having to worry about any additional manual steps, which if not done, may silently fool myself. IIRC, someone once reported an issue (either here on GH or via mailing list) complaining about some tunes sounding detuned. It turned out that the packager of fluidsynth he installed it from did in fact pre-generated those tables. And just before that, we made a tiny change to the math behind the lookup tables. They didn't regenerate the tables though, which had caused the problem.