Better support building the runtime separately

[the-horo]

My use case for this is compiling the runtime for x86 on x86_64. -DMULTILIB=ON is not sufficient as I need to create a separate project in order to use a system copy of zlib like I've described in #4742 (comment).

[the-horo]

It looks like cmake is not adding the dependency on LDC_EXE properly. The quick fix is to add back LDC_EXE_FULL as a dependency when building the runtime but I want to figure out why cmake is misbehaving.

[the-horo]

I've reduced it to:

cmake_minimum_required(VERSION 3.27)
project("compiler" LANGUAGES CXX)

set(full "${PROJECT_BINARY_DIR}/bin/mycompiler")

if(TRUE)
    # fails
    add_custom_command(OUTPUT "${full}"
	COMMAND g++ -o "${full}" "${PROJECT_SOURCE_DIR}/mycompiler.cpp"
	DEPENDS mycompiler.cpp
    )
    add_custom_target(mycompiler ALL DEPENDS "${full}")
else()
    # works
    add_executable(mycompiler "mycompiler.cpp")
    set_target_properties(mycompiler PROPERTIES
	RUNTIME_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/bin")
endif()

add_custom_command(OUTPUT foo.o
    COMMAND "${full}" -o foo.o foo.cpp
    DEPENDS mycompiler
    DEPENDS_EXPLICIT_ONLY # this thing breaks
)
add_library(foo STATIC foo.o)
set_target_properties(foo PROPERTIES LINKER_LANGUAGE CXX)

[the-horo]

This seems to be the intended behavior. From add_custom_command docs:

DEPENDS

Specify files on which the command depends. Each argument is converted to a dependency as follows:

1. If the argument is the name of a target (created by the add_custom_target(), add_executable(), or add_library() command) a target-level dependency is created to make sure the target is built before any target using this custom command. Additionally, if the target is an executable or library, a file-level dependency is created to cause the custom command to re-run whenever the target is recompiled.

This wording clearly states that the dependency of add_custom_command aren't for the add_custom_command target, they are only transitively applied to other targets that use the output of the command. To put in bluntly:

What we need:

1. generate foo.o from foo.d (depends on LDC)
2. archive foo.o into libruntime.a

What cmake does:

1. generate foo.o from foo.d
2. archive foo.o into libdruntime.a (depends on LDC)

Since this seems to be the intended behavior the add_custom_command need to be adjusted to properly specify dependencies. We can either drop CMAKE_ADD_CUSTOM_COMMAND_DEPENDS_EXPLICIT_ONLY or specify the dependencies as absolute paths instead of target names. We probably want the latter.

[kinke]

Are you aware of https://wiki.dlang.org/Building_LDC_runtime_libraries? The ldc-build-runtime tool is just sugar around CMake, but I doubt we really need to support cmdline flags in LDC_EXE_FULL; one of the existing, historically named vars like D_EXTRA_FLAGS should do.

[the-horo]

Are you aware of https://wiki.dlang.org/Building_LDC_runtime_libraries? The ldc-build-runtime tool is just sugar around CMake, but I doubt we really need to support cmdline flags in LDC_EXE_FULL; one of the existing, historically named vars like D_EXTRA_FLAGS should do.

I was aware of the tool and the fact that it was mostly a wrapper over cmake so I would prefer to use cmake directly. When I first tried to build the runtime for multilib directly the building itself worked but I hit an issue with the test runners and the druntime integration tests. I think I tried to do -DLDC_EXE_FULL=<ldc2>;-m32 but I didn't try -DLDC_EXE_FULL=<ldc2> -DD_EXTRA_ARGS=-m32 which does work fine for the test runners.

The only thing that doesn't work are the druntime integration tests so we either have pass D_EXTRA_FLAGS to ldmd there too or support that -DLDMD_EXE_FULL contain spaces like I originally proposed. Since the rest of the code uses D_EXTRA_FLAGS I think it's better to stay consistent.

As a conclusion, is it fine if to keep the non-flag related changes like the addition of enable_testing() and to make sure that D_EXTRA_FLAGS are respected in DRuntimeIntegrationTests.cmake?

[the-horo]

I can also improve the code and add support for -DMULTILIB=ON for the druntime tests since only the native build is tested currently.

[the-horo]

My insistence on allowing LDC_EXE_FULL to contain arguments is because this is how everyone else does it, cmake included. It does accomplish the same thing as the current LDC_EXE_FULL and D_EXTRA_FLAGS, i.e. a command and arguments, but it does so in a less error prone way since it's impossible to forget to take into account the necessary command line arguments, which is what happened with the druntime integration tests.

It's also more clear which arguments are required, otherwise the compilation will fail (or do the wrong thing), and which arguments are optional, passed by the user for a more efficient/safe/whatever they desire final binary. In the latter case it's safe to ignore those flags if we're building some intermediary object that we may need for running the tests / generating documentation so we don't always need to respect them.

I'm saying all this not to convince you, but for you to know what my position on the matter is. I won't keep pushing for such changes unless the current D_EXTRA_FLAGS solution will become broken, which it currently isn't.

[kinke]

Yeah I guessed you were after the druntime integration tests, which as you figured, are currently only run for the primary platform. DMD there working with flags is probably just a nice coincidence, there's a MODEL variable for the -m32/-m64 switches (on the few archs supporting them).

If we wanted to include the 32-bit druntime integration tests for MULTILIB builds, we'd need to add some test name suffix to make them unique.

My insistence on allowing LDC_EXE_FULL to contain arguments is because this is how everyone else does it, cmake included. It does accomplish the same thing as the current LDC_EXE_FULL and D_EXTRA_FLAGS, i.e. a command and arguments, but it does so in a less error prone way since it's impossible to forget to take into account the necessary command line arguments.

What I don't like is that we already have a variable for extra flags, and having to take account cmdline options in a compiler var. I trust this is some common concept on Posix, but I don't really like that extra cognitive overhead and CMake special cases. Maybe coz I grew up with Windows, where such space-separated flags are unthinkable. ;)

[the-horo]

If we wanted to include the 32-bit druntime integration tests for MULTILIB builds, we'd need to add some test name suffix to make them unique.

The druntime test makefiles already support ROOT for the output files so we should only need to add a suffix to the test names and be done with it. We would also need the special case the coverage test but it may also be possible to modify it upstream to use separate copies of the test files instead of modifying the same one.

My insistence on allowing LDC_EXE_FULL to contain arguments is because this is how everyone else does it, cmake included. It does accomplish the same thing as the current LDC_EXE_FULL and D_EXTRA_FLAGS, i.e. a command and arguments, but it does so in a less error prone way since it's impossible to forget to take into account the necessary command line arguments.

What I don't like is that we already have a variable for extra flags, and having to take account cmdline options in a compiler var. I trust this is some common concept on Posix, but I don't really like that extra cognitive overhead and CMake special cases. Maybe coz I grew up with Windows, where such space-separated flags are unthinkable. ;)

Yeah, I can see why you wouldn't want your Program Files to be split on windows.

[the-horo]

-set(LDMD_CMD "${LDMD_EXE_FULL} ${D_EXTRA_ARGS}")
+string(REPLACE ";" " " LDMD_CMD "${LDMD_EXE_FULL} ${D_EXTRA_FLAGS}")

1. I was using the wrong variable name.
2. D_EXTRA_FLAGS is a ;-delimited list, not space separated.

[kinke]

And yet we add a add-multilib-section target name 3 lines below. According to

If the argument is the name of a target (created by the add_custom_target(), add_executable(), or add_library() command) a target-level dependency is created to make sure the target is built before any target using this custom command.

this should still make sure the target is created before building any dependent. The (additional) path dependency should take care of the 2nd part wrt. rebuilds IIUC:

Additionally, if the target is an executable or library, a file-level dependency is created to cause the custom command to re-run whenever the target is recompiled.

I guess they actually mean 'if the target is an add_executable or add_library target', so that one has to specify the path to the add_custom_target executable manually for proper rebuilds.

[the-horo]

And yet we add a add-multilib-section target name 3 lines below. According to

If the argument is the name of a target (created by the add_custom_target(), add_executable(), or add_library() command) a target-level dependency is created to make sure the target is built before any target using this custom command.

this should still make sure the target is created before building any dependent. The (additional) path dependency should take care of the 2nd part wrt. rebuilds IIUC:

From looking at build.ninja, the add-multilib-section target is added as a dependency to the final libraries, like lib64/libdruntime-ldc-shared.so.109.1. I think this is fine, as the libraries are compiled with -conf= so they don't actually need the config file to be updated. In this case I think the weird cmake behavior does what we want: "update the config file before declaring the library built", it just does it a little bit later in the process.

Additionally, if the target is an executable or library, a file-level dependency is created to cause the custom command to re-run whenever the target is recompiled.

I guess they actually mean 'if the target is an add_executable or add_library target', so that one has to specify the path to the add_custom_target executable manually for proper rebuilds.

I'm reading this the other way around:

• if the target is an executable or library, cmake adds a file-path dependency automatically => we don't need to specify a full path as a dependency.
• otherwise, cmake does nothing => we need to specify the full path if we need that dependency to be built before this add_custom_command or add_custom_target.

[kinke]

I guess they actually mean 'if the target is an add_executable or add_library target', so that one has to specify the path to the add_custom_target executable manually for proper rebuilds.

I'm reading this the other way around:

I'm pretty sure the 'target' they refer to is the dependency being added, i.e., LDC_EXE (expands to ldc2) here. Which can either be an add_executable or an add_custom_target, depending on LDC_LINK_MANUALLY. So in case the ldc2 executable is a custom target, we additionally add LDC_EXE_FULL so that the runtimes stuff is rebuilt whenever the compiler is.

[the-horo]

Ah, yes, I understand it the same way. Maybe I failed to word it right but, in my defense, it is a unintuitive behavior cmake is exhibiting.

... so that the runtimes stuff is rebuilt whenever the compiler is.

• that the compiler is built, if not present, before the runtime build commands try to invoke it.

[kinke]

Fully agreed wrt. unintuitive (edit: and bad wording on my part - the compiler file dependency via LDC_EXE_PATH should make sure existing runtime build artifacts get stale as soon as the compiler is rebuilt). Probably related: we had CI issues with the make backend, IIRC some targets not waiting for the prerequisites to be finished. Working fine with ninja though.

[kinke]

Okay after another re-read, I think I got your point now - that the LDC_EXE target name dependency does NOT apply to the custom command getting this dependency directly, but only to the dependents of that custom command. So you're absolutely right, the target-name dependency is insufficient, we need the path dependency to make it apply to the custom command directly, and not just for rebuilds. Thanks for digging!

[kinke]

Your use case would be setting D_EXTRA_FLAGS=-m32, right? Just to understand. This step should probably be skipped on Windows [but that divergence would be a bit ugly IMO], to avoid spaces in the path and spaces as flag separators.

AFAICT, the multilib case would be designed to be used with the MODEL make variable:

ldc/runtime/druntime/test/common.mak

Line 33 in 1f0c36e

MODEL_FLAG:=$(if $(findstring $(MODEL),default),,-m$(MODEL))

The MODEL would make it into DFLAGS:

ldc/runtime/druntime/test/common.mak

Line 41 in 1f0c36e

DFLAGS:=$(MODEL_FLAG) $(PIC) -w -I../../src -I../../import -I$(SRC) -defaultlib=$(if $(findstring ldmd2,$(DMD)),druntime-ldc,) -preview=dip1000 $(if $(findstring $(OS),windows),,-L-lpthread -L-lm $(LINKDL))

and CFLAGS_BASE (which we override for non-MSVC targets via CMake cflags_base though):

ldc/runtime/druntime/test/common.mak

Line 34 in 1f0c36e

CFLAGS_BASE:=$(if $(findstring $(OS),windows),/Wall,$(MODEL_FLAG) $(PIC) -Wall)

How do you currently handle the C side for the separate 32-bit build? Setting a CC env var incl. an -m32 flag?

[the-horo]

Your use case would be setting D_EXTRA_FLAGS=-m32, right? Just to understand. This step should probably be skipped on Windows [but that divergence would be a bit ugly IMO], to avoid spaces in the path and spaces as flag separators.

Yes, only -m32 for now. Though it is technically possible, in some future, for this to use other -m<abi> flags for different architectures.

AFAICT, the multilib case would be designed to be used with the MODEL make variable:

ldc/runtime/druntime/test/common.mak

Line 33 in 1f0c36e

MODEL_FLAG:=$(if $(findstring $(MODEL),default),,-m$(MODEL))

The MODEL would make it into DFLAGS:

ldc/runtime/druntime/test/common.mak

Line 41 in 1f0c36e

DFLAGS:=$(MODEL_FLAG) $(PIC) -w -I../../src -I../../import -I$(SRC) -defaultlib=$(if $(findstring ldmd2,$(DMD)),druntime-ldc,) -preview=dip1000 $(if $(findstring $(OS),windows),,-L-lpthread -L-lm $(LINKDL))

and CFLAGS_BASE (which we override for non-MSVC targets via CMake cflags_base though):

ldc/runtime/druntime/test/common.mak

Line 34 in 1f0c36e

CFLAGS_BASE:=$(if $(findstring $(OS),windows),/Wall,$(MODEL_FLAG) $(PIC) -Wall)

How do you currently handle the C side for the separate 32-bit build? Setting a CC env var incl. an -m32 flag?

These variables are set already by the Gentoo helper functions so I didn't have to interact with them but, if I followed the functions right:

• https://github.com/gentoo/gentoo/blob/e1c6cdf0e713cd916c7880296b3af32834fdb9c0/eclass/multilib.eclass#L544 sets CC to contain the -m flag.
• https://github.com/gentoo/gentoo/blob/e1c6cdf0e713cd916c7880296b3af32834fdb9c0/eclass/cmake.eclass#L468-L477 example: set(CMAKE_C_COMPILER "x86_64-pc-linux-gnu-gcc;-m32 -mfpmath=sse")

Here's what the logs contain:

460: x86_64-pc-linux-gnu-gcc -m32 -mfpmath=sse -Wall -Wl,-rpath,/var/tmp/portage/dev-lang/ldc2-1.40.0_beta3-r1/work/ldc-1.40.0-beta3-src_build-abi_x86_32.x86/lib -L/var/tmp/portage/dev-lang/ldc2-1.40.0_beta3-r1/work/ldc-1.40.0-beta3-src_build-abi_x86_32.x86/lib -O3 -o/var/tmp/portage/dev-lang/ldc2-1.40.0_beta3-r1/work/ldc-1.40.0-beta3-src_build-abi_x86_32.x86/druntime-test-shared-release/linkDR  src/linkDR.c /var/tmp/portage/dev-lang/ldc2-1.40.0_beta3-r1/work/ldc-1.40.0-beta3-src_build-abi_x86_32.x86/lib/libdruntime-ldc-shared.so -ldl  -pthread

So yeah, setting CC seems like what's doing the trick, including for the druntime tests that are called through cmake. Note that there isn't a single -m32 flag, -mfpmath=sse is also present so I would like the solution to allow any combination of flags.

Setting MODEL_FLAG=${D_EXTRA_FLAGS} seems like it would do the trick:

• allows for arbitrary flags to be passed through
• doesn't interfere with space-splitting CC

But can you explain something for me? Does gmake on windows not perform space expansion on variables, regardless of where they appear, or is there some special case only for when the variable appears as the first thing in a command. For example:

main: main.cpp
	$(CC) $(CFLAGS) -o main main.cpp

CFLAGS gets split by spaces. CC is also split on linux. Does windows not split CC but splits CFLAGS?

[kinke]

I seem to recall/assume (haven't tested it now!) that on Windows, you are supposed to use explicit quotes, something like make CC="my cc.exe". IIRC, make then uses those quotes as-is, so that $(CC) -c gets expanded to "my cc.exe" -c. This should allow adding the D_EXTRA_FLAGS to LDMD_CMD on Windows too. If a poor soul had to split it up into executable and flags, that'd be a PITA of course, as trivial splitting by space wouldn't work of course.

Edit: Tested it; the cmdline needs extra quoting, but the rest holds: make "CC=\"my cc.exe\"".
Edit2: Nope, make CC="my cc.exe" also works (in a cmd.exe shell), surprisingly. But that might be some special make behavior on Windows (wrt. the executable part of the command); make "CC=my cc.exe" also works.
Edit3: With a flag, this would apparently have to be: make "CC=\"my cc.exe\" -abc".
Edit4: Oh god, there's definitely some magic logic somewhere, this really works too: make "CC=my cc -abc". With a C:\Windows\my cc.exe (copy of calc.exe ;)).

[the-horo]

Amazing! I don't have a windows machine so I trust you on these.

So, what's the final solution we go with?

1. allow LDMD_CMD co contain arguments
2. use MODEL_FLAG to pass D_EXTRA_FLAGS

1 is unintutive on windows because of the comamnd magic, 2 is unintuitive everywhere since the variables don't really seem related.

If the tests are adjusted to also test for 32 bit with -DMULTILIB=ON that would require passing -m32/-m64 somewhere. We could use MODEL here or just override MODEL_FLAG again. I think the final result would look like:

1 + MODEL:

# do_druntime_tests invokes the tests with make MODEL=${1}
do_druntime_tests("")
if(MULTILIB)
  do_druntime_tests("${HOST_BITNESS}")
endif()

1 + MODEL_FLAG:

# do_druntime_tests invokes the tests with make MODEL_FLAG=${1}
do_druntime_tests("")
if(MULTILIB)
  do_druntime_tests("-m${HOST_BITNESS}")
endif()

2 + MODEL_FLAG:

# do_druntime_tests invokes the tests with make MODEL_FLAG="${D_EXTRA_FLAGS} ${1}"
do_druntime_tests("")
if(MULTILIB)
  do_druntime_tests("-m${HOST_BITNESS}")
endif()

[the-horo]

I have to, sometime, change the dmd makefiles to allow taking flags from the command line instead of relying on hacks like using MODEL_FLAG, as Gentoo policy dictates it. If I get to upstream these changes we should be able to do make MY_D_FLAGS=${D_EXTRA_FLAGS} and use either of the model flag logic so maybe it's worth taking this future possibility into consideration.

[kinke]

For the sake of standardization I would recommend that those variables are, instead, named DFLAGS and CFLAGS as that makes their intent very clear and is the standard way of handling flags.

AFAIK, it's common to have such CFLAGS_BASE, in order to only set optional extra/critical flags on demand. Specifying a make variable like this (after make, not as env vars) silently skips all regular assignments of CFLAGS_BASE in common.mak. The full list in CFLAGS etc. can then still be joined together in common.mak, based on (potentially custom) CFLAGS_BASE.

Similarly, common.mak currently also allows one to customize CXXFLAGS_BASE - it just defaults to CFLAGS_BASE in

ldc/runtime/druntime/test/common.mak

Line 56 in 1f0c36e

CXXFLAGS_BASE:=$(CFLAGS_BASE)

[kinke]

Yes, if ldc provides simple variables like RT_CFLAGS and RT_LFLAGS to configure its build, on top of working with the flags cmake is configured with, then the easiest way to pass them along to the druntime tests is having the makefile respect variables like DFLAGS_BASE and CFLAGS_BASE just like you suggest.

[The non-super-consistently named CMake variables come from having a fat umbrella CMake build for LDC, where runtime and Phobos are just a subset of all builds. So we ended up with some ugly names for compiler-only D flags, runtime-only D flags etc. etc. They are usually empty, but designed exactly for such target-specific critical flags and/or custom flags.]

In the worst case I would need to add -DRT_CFLAGS=$(get_abi_CFLAGS) which isn't a deal breaker.

The only potential problem I see with you specifying the C flags explicitly this way is that they would be duplicated when building the runtime libraries (due to Gentoo's CMake hook injecting the critical flags), and potentially also for the make integration tests if CC was still set with flags at test-time. The compiler might complain about some options being specified multiple times.

[the-horo]

The only potential problem I see with you specifying the C flags explicitly this way is that they would be duplicated when building the runtime libraries (due to Gentoo's CMake hook injecting the critical flags), and potentially also for the make integration tests if CC was still set with flags at test-time. The compiler might complain about some options being specified multiple times.

I meant that I would pass-DRT_CFLAGS=-m32 if the cmake code were changed to invoke gmake with CC=${CMAKE_C_COMPILER} to respect the configure-time value. In that case the environment variable is not in effect so $(CC) inside make will be the compiler binary without any arguments so there won't be any duplication with RT_CFLAGS.

[kinke]

Yeah that's the case we can handle. But what about the actual library build?

[the-horo]

Yeah, in that case the flags would be duplicated. This shouldn't be an issue and, even if it is, I can always just patch the cmake files to use a separate variable for passing abi-related CFLAGS and only respect in when invoking make, like CC="${CMAKE_C_COMPILER} ${GENTOO_ABI_CFLAGS}" since this should work on UNIXes.

This is slightly more work for me but, given the choices:

1. don't respect ${CMAKE_C_COMPILER} when calling the makefiles and rely on runtime CC
2. respect ${CMAKE_C_COMPILER}, even if it discards flags

I would prefer for ldc to use 2, because, if it breaks, it would be easier to diagnose even if that entails me possibly carrying one more patch downstream.